diff --git a/architecture-baseline.toml b/architecture-baseline.toml index 833e8a38d..92803fffa 100644 --- a/architecture-baseline.toml +++ b/architecture-baseline.toml @@ -158,7 +158,7 @@ "crates/libs/api/src/controllers/substrate/kubernetes/store/execution_rows.rs" = 1 "crates/libs/api/src/controllers/substrate/kubernetes/store/pod_placement.rs" = 1 "crates/libs/api/src/controllers/substrate/kubernetes/store/pod_tracking.rs" = 5 -"crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs" = 5 +"crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs" = 8 "crates/libs/api/src/controllers/substrate/kubernetes/store/suspend.rs" = 8 "crates/libs/api/src/controllers/substrate/mod.rs" = 1 "crates/libs/cluster/controller/server/src/drained_node_terminator/store.rs" = 2 @@ -169,7 +169,7 @@ "crates/libs/cluster/controller/server/src/started_sandbox_repair/store.rs" = 3 "crates/libs/cluster/controller/server/src/suspended_execution_reaper/store.rs" = 1 "crates/libs/cluster/controller/server/src/tombstone_retention.rs" = 1 -"crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs" = 10 +"crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs" = 12 "crates/libs/internal/api/server/src/data_plane_sessions/service_route.rs" = 1 "crates/libs/internal/api/server/src/published_state.rs" = 9 "crates/libs/internal/api/server/src/sandbox_quarantine_store.rs" = 3 diff --git a/crates/libs/api/src/controllers/sandbox/ingress/tests/kubernetes.rs b/crates/libs/api/src/controllers/sandbox/ingress/tests/kubernetes.rs index 162725ef1..6fea0fb0d 100644 --- a/crates/libs/api/src/controllers/sandbox/ingress/tests/kubernetes.rs +++ b/crates/libs/api/src/controllers/sandbox/ingress/tests/kubernetes.rs @@ -123,3 +123,64 @@ async fn stored_ingress_access_resumes_paused_execution_before_reporting_ready() } Ok(()) } + +/// Ingress to a `restoring` sandbox whose restore lease has lapsed while its +/// execution still owns the restore (so `observe` reports no owner) reaches +/// `access`, which must not dial the restoring pod: it joins the restore and +/// reports ready only once the sandbox is `started`. +#[tokio::test(flavor = "current_thread")] +async fn stored_ingress_access_joins_a_restore_whose_lease_lapsed() +-> pplx_space_test_env::TestResult { + use pplx_space_common_core::types::AutoLifecycleWakeSource; + use pplx_space_db_test_fixtures::try_pg_test_db; + use pplx_space_node_api_sim::SimWorkerHarness; + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let Some(db) = try_pg_test_db().await? else { + return Ok(()); + }; + let worker = SimWorkerHarness::builder(crate::test_support::kubernetes::WORKER_NODE) + .with_internal_api() + .start() + .await?; + let node = insert_kubernetes_worker_node(&db.pool).await?; + let id = SandboxId::new(); + let mut policy = AutoLifecyclePolicy::disabled(); + policy.wake.on.push(AutoLifecycleWakeSource::Access); + sqlx::query(sql!("INSERT INTO sandboxes (id, user_id, org_id, managed_by, lifecycle, placement, active_node_id, template_id, content_hash, auto_lifecycle_json, network_policy_generation, restore_lease_expires_at, restore_lease_token) + VALUES ($1, '00000000-0000-0000-0000-000000000010', '00000000-0000-0000-0000-000000000001', 'k8s', 'restoring', 'on_node', $2, 'seed', 'hash-seed', $3, 1, NOW() - INTERVAL '1 second', $4)")) + .bind(id.as_uuid()).bind(node).bind(sqlx::types::Json(&policy)).bind(uuid::Uuid::now_v7()).execute(&db.pool).await?; + let LiveExecution { pod, .. } = + insert_live_execution(&db.pool, id, RouteIdentity::Published).await?; + let (substrate, _) = substrate_over_pods(&db.pool, worker.port(), vec![pod])?; + let backend = StoredAccess { + pool: db.pool.clone(), + controller: SandboxController::new(kubernetes(substrate)), + }; + assert!( + !backend.observe(id).await?.restore_owned, + "the lapsed lease leaves ingress with no owner to observe" + ); + let owner_pool = db.pool.clone(); + let owner = tokio::spawn(async move { + tokio::time::sleep(std::time::Duration::from_millis(300)).await; + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'started' WHERE id = $1" + )) + .bind(id.as_uuid()) + .execute(&owner_pool) + .await + }); + + let outcome = backend.access(id).await; + let lifecycle_at_return = fetch_sandbox(&db.pool, id).await?.unwrap().lifecycle; + owner.await??; + + assert_eq!(outcome?, AccessOutcome::Ready); + assert_eq!( + lifecycle_at_return, "started", + "ingress must not reach a restoring pod" + ); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/sandbox/lifecycle.rs b/crates/libs/api/src/controllers/sandbox/lifecycle.rs index 93cbb7283..e0fef71c5 100644 --- a/crates/libs/api/src/controllers/sandbox/lifecycle.rs +++ b/crates/libs/api/src/controllers/sandbox/lifecycle.rs @@ -379,7 +379,10 @@ impl PublishedSuspendFence { /// may continue, but this stale completion must not replace `suspending` with /// `started`. A substrate whose restore-shaped wakes claim `restoring` before /// the node call and leave `restoring -> started` to this CAS (Kubernetes) -/// also fences a finalized `suspended` state. Direct does not: its +/// also fences a finalized `suspended` state. On Kubernetes the restore's +/// owner usually committed `started` already (see +/// `SubstrateCapabilities::caller_completes_claimed_restore`); this CAS then +/// rewrites `started` and completes. Direct does not: its /// worker-local wake leaves its pre-wake lifecycle (`suspended`, or /// `restoring` kept by an earlier lease restore) for this CAS to advance, and /// its lease restore writes `started` before returning. @@ -2168,7 +2171,11 @@ impl SandboxController { } }; if let Some(status) = status { - maybe_update_sandbox_lifecycle(pool, id, status.as_str()).await; + if !(status == SandboxLifecycleStatus::Suspended + && self.substrate.capabilities().suspend_commits_lifecycle) + { + maybe_update_sandbox_lifecycle(pool, id, status.as_str()).await; + } if status == SandboxLifecycleStatus::Started { record_dormant_activation_if_needed(sandbox, metric_identity, action); self.reconcile_open_sandbox_port_exposures_best_effort(pool, id) @@ -2408,6 +2415,9 @@ mod wake_completion_postgres_tests; #[cfg(test)] mod characterization_tests; +#[cfg(all(test, feature = "kubernetes"))] +mod kubernetes_suspend_tests; + // Coverage-gap acknowledgment: `SandboxController::control_runtime_client`'s // ordering invariant (drain check must run before any substrate call that can // implicit-recover a pod) is not unit-testable here because the `Substrate` diff --git a/crates/libs/api/src/controllers/sandbox/lifecycle/kubernetes_suspend_tests.rs b/crates/libs/api/src/controllers/sandbox/lifecycle/kubernetes_suspend_tests.rs new file mode 100644 index 000000000..cfa54c2fe --- /dev/null +++ b/crates/libs/api/src/controllers/sandbox/lifecycle/kubernetes_suspend_tests.rs @@ -0,0 +1,108 @@ +//! The suspend action's lifecycle write on the Kubernetes substrate, whose +//! `suspend_execution` commits `suspended` itself. + +use std::sync::Arc; +use std::sync::atomic::AtomicUsize; + +use pplx_space_common_core::types::{LocalArtifactId, SandboxId}; +use pplx_space_db_test_fixtures::pg_test_db; +use pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity; +use pplx_space_test_env::TestResult; + +use super::*; +use crate::controllers::substrate::kubernetes::tests::{ + deleted_pod_kubernetes_client, insert_k8s_execution, insert_test_sandbox_row, + substrate_with_store_and_client, +}; + +const POD_NAME: &str = "space-sbx-suspend-race-g1"; +const POD_UID: &str = "suspend-race-uid"; + +/// A wake that claims the sandbox right after the substrate commits its +/// suspend keeps the row. The suspend action writes no lifecycle of its own +/// afterwards: a trailing `suspended` would put the woken, routed sandbox +/// back to `suspended` while its new execution stays active, the shape whose +/// pod holds the only copy of the sandbox's state. +/// +/// A deferred trigger stands in for the wake: it moves the row to +/// `restoring` when the transaction that committed `suspended` commits, +/// which is after the substrate's write and before anything the controller +/// does next. +#[tokio::test(flavor = "current_thread")] +async fn suspend_keeps_a_wake_that_claimed_after_the_suspend_committed() -> TestResult { + let db = pg_test_db().await?; + let pool = &db.pool; + let sandbox_id = SandboxId::new(); + insert_test_sandbox_row(pool, sandbox_id, "suspending").await?; + // The node already committed the suspend, so the substrate only retires + // the pod and finalizes the row; it never reaches the node. + sqlx::query(sql!( + "UPDATE sandboxes + SET current_local_artifact_id = $2, + current_local_artifact_kind = 'full', + current_local_artifact_origin = 'suspend' + WHERE id = $1" + )) + .bind(sandbox_id) + .bind(LocalArtifactId::new()) + .execute(pool) + .await?; + insert_k8s_execution(pool, sandbox_id, 1, POD_NAME, POD_UID, None, true).await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions SET state = 'suspended' WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + sqlx::query(sql!( + "CREATE FUNCTION wake_claims_after_suspend() RETURNS trigger AS $$ + BEGIN + UPDATE sandboxes SET lifecycle = 'restoring' WHERE id = NEW.id; + RETURN NULL; + END; + $$ LANGUAGE plpgsql" + )) + .execute(pool) + .await?; + sqlx::query(sql!( + "CREATE CONSTRAINT TRIGGER wake_claims_after_suspend + AFTER UPDATE OF lifecycle ON sandboxes + DEFERRABLE INITIALLY DEFERRED + FOR EACH ROW + WHEN (OLD.lifecycle = 'suspending' AND NEW.lifecycle = 'suspended') + EXECUTE FUNCTION wake_claims_after_suspend()" + )) + .execute(pool) + .await?; + + let mut substrate = substrate_with_store_and_client( + pool.clone(), + deleted_pod_kubernetes_client(POD_NAME, POD_UID, Arc::new(AtomicUsize::new(0))), + ); + substrate.k8s.k8s_blocking_delete_timeout = std::time::Duration::from_millis(500); + let controller = SandboxController::new( + crate::controllers::substrate::Substrate::from_kubernetes(substrate), + ); + let sandbox = crate::controllers::sandbox::fetch_sandbox(pool, sandbox_id) + .await? + .expect("sandbox row exists"); + + controller + .action( + pool, + sandbox_id, + Some(&sandbox), + SandboxApiMetricIdentity::native(SandboxAction::Suspend.as_str()), + SandboxAction::Suspend, + None, + ) + .await?; + + let lifecycle: String = + sqlx::query_scalar(sql!("SELECT lifecycle FROM sandboxes WHERE id = $1")) + .bind(sandbox_id) + .fetch_one(pool) + .await?; + assert_eq!(lifecycle, "restoring"); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/sandbox/wake_completion.rs b/crates/libs/api/src/controllers/sandbox/wake_completion.rs index e2938b05f..c25450c67 100644 --- a/crates/libs/api/src/controllers/sandbox/wake_completion.rs +++ b/crates/libs/api/src/controllers/sandbox/wake_completion.rs @@ -25,6 +25,13 @@ //! Process death during the redrive still loses the write; the sweeper //! remains the backstop, exactly as `CancelGuard` defers to the reconciler //! when it cannot clean up itself. +//! +//! On Kubernetes the restore's owner commits `started` in the substrate under +//! its own lease token before the wake returns +//! (`K8sStore::complete_owned_user_restore`, this module's CAS with the +//! lease-expiry guard replaced by that token). The shield covers the wakes +//! whose owner commit didn't land, and a redrive that finds the row already +//! `started` has nothing left to do. use pplx_space_common_core::sql; use std::sync::{Arc, LazyLock}; diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/adapter/substrate_adapter.rs b/crates/libs/api/src/controllers/substrate/kubernetes/adapter/substrate_adapter.rs index 6b1551c47..ef84438d6 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/adapter/substrate_adapter.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/adapter/substrate_adapter.rs @@ -1497,9 +1497,22 @@ impl SubstrateAdapter for KubernetesSubstrate { .resolve_pod_instance(sandbox_id) .await .map_err(ControllerError::from)? - && let Ok(client) = self.access_kept_worker_client(sandbox_id, &pod).await { - return Ok(client); + if pod.sandbox_restoring { + return self + .join_restore_before_dial( + sandbox_id, + pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity::native( + "runtime_client", + ), + Duration::from_secs(120), + charge, + ) + .await; + } + if let Ok(client) = self.access_kept_worker_client(sandbox_id, &pod).await { + return Ok(client); + } } let sandbox = self @@ -1507,11 +1520,23 @@ impl SubstrateAdapter for KubernetesSubstrate { .require_sandbox(sandbox_id) .await .map_err(ControllerError::from)?; + if is_restoring_lifecycle(&sandbox.lifecycle) { + return self + .join_restore_before_dial( + sandbox_id, + pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity::native( + "runtime_client", + ), + Duration::from_secs(120), + charge, + ) + .await; + } if normalize_lifecycle(&sandbox_before_resolution.lifecycle) == "suspending" && is_suspended_lifecycle(&sandbox.lifecycle) { - let binding = self - .wake_execution_with_metric_identity( + return self + .wake_and_dial_once_started( &sandbox, Duration::from_secs(120), pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity::native( @@ -1519,9 +1544,7 @@ impl SubstrateAdapter for KubernetesSubstrate { ), charge, ) - .await - .map_err(ControllerError::from)?; - return connect_worker(binding.binding.runtime_endpoint).await; + .await; } if is_wake_recovery_lifecycle(&sandbox.lifecycle) && !is_dormant_lifecycle(&sandbox.lifecycle) @@ -1556,6 +1579,12 @@ impl SubstrateAdapter for KubernetesSubstrate { else { return Ok(None); }; + if pod.sandbox_restoring { + // Never dial a restoring pod (see `join_restore_before_dial`). + // This path neither recovers nor waits: its callers are + // best-effort or turn "unreachable" into a retryable 503. + return Ok(None); + } match self.access_kept_worker_client(sandbox_id, &pod).await { Ok(client) => Ok(Some(client)), @@ -1591,16 +1620,14 @@ impl SubstrateAdapter for KubernetesSubstrate { ) -> Result { if is_suspended_lifecycle(&sandbox.lifecycle) { Self::should_allow_access_recovery(sandbox, activity)?; - let binding = self - .wake_execution_with_metric_identity( + return self + .wake_and_dial_once_started( sandbox, timeout, metric_identity, crate::controllers::substrate::RecoveryCharge::Exempt, ) - .await - .map_err(ControllerError::from)?; - return connect_worker(binding.binding.runtime_endpoint).await; + .await; } if let Some(pod) = self @@ -1608,6 +1635,11 @@ impl SubstrateAdapter for KubernetesSubstrate { .await .map_err(ControllerError::from)? { + if pod.sandbox_restoring { + return self + .join_restore_before_dial(sandbox.id, metric_identity, timeout, charge) + .await; + } let pod_ip = pod.ip; // A preserved slow create was reported `started` before its guest // command API was confirmed ready. Force a (bounded) command-ready @@ -1668,6 +1700,11 @@ impl SubstrateAdapter for KubernetesSubstrate { .await .map_err(ControllerError::from)?; Self::should_allow_access_recovery(&sandbox, activity)?; + if is_restoring_lifecycle(&sandbox.lifecycle) { + return self + .join_restore_before_dial(sandbox.id, metric_identity, timeout, charge) + .await; + } let binding = self .ensure_execution_ready_for_access(&sandbox, metric_identity, timeout, charge, None) .await?; diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/adapter/teardown.rs b/crates/libs/api/src/controllers/substrate/kubernetes/adapter/teardown.rs index 762496374..2ea2b75a4 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/adapter/teardown.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/adapter/teardown.rs @@ -597,7 +597,10 @@ impl KubernetesSubstrate { /// Idempotent best-effort delete of each tracked pod name. An already-gone /// pod is the expected case, not a failure. - async fn delete_tracked_pods(&self, pod_names: Vec) -> Result<(), SubstrateError> { + pub(in crate::controllers::substrate::kubernetes) async fn delete_tracked_pods( + &self, + pod_names: Vec, + ) -> Result<(), SubstrateError> { for pod_name in pod_names { if let Err(err) = delete_sandbox_pod(&self.k8s, &pod_name).await && !is_not_found_error(&err) diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests.rs index ddcb71ae4..8bf2cf433 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests.rs @@ -14,6 +14,7 @@ mod create_readiness; mod durable_snapshot; mod hydration_and_suspend; mod pooled_adoption; +mod restoring_access; use std::collections::BTreeMap; diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/create_and_teardown.rs b/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/create_and_teardown.rs index f69a6eb04..83ab72ce4 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/create_and_teardown.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/create_and_teardown.rs @@ -3,8 +3,8 @@ use axum::http::{Method, StatusCode}; use super::super::scripted_test_doubles::{ - KubeLog, ScriptedReply, ScriptedWorkerConfig, kube_status, pod_path, requests_matching, - scripted_kube_client, start_scripted_worker, + KubeLog, ScriptedReply, ScriptedWorkerConfig, kube_status, pod_path, pod_reply, + requests_matching, scripted_kube_client, start_scripted_worker, }; use super::super::tests::{ insert_test_sandbox_row, node_with_network_slots, sandbox_pod_on_node, @@ -12,6 +12,8 @@ use super::super::tests::{ substrate_with_store_client_proxy_port_and_pod, }; use super::*; +use crate::controllers::substrate::{PodIdentityFence, RecordedExecution}; +use pplx_space_common_core::types::{PodName, PodResourceVersion, PodUid}; // ── create_execution ──────────────────────────────────────────────────────── @@ -467,3 +469,179 @@ async fn teardown_propagates_an_apiserver_fault_while_resolving_the_runtime() -> assert_eq!(active_execution_count(&db.pool, sandbox_id).await?, 1); Ok(()) } + +/// The shape the sweeper's lifecycle-only revert leaves with abandoned-restore +/// retirement disabled, the default until every routing replica refuses to +/// route to a `restoring` sandbox: the restore's execution still active. A +/// restore pod that came up after the sweeper's live-pod check may then have +/// served a user request through a replica without that guard. The cleanup +/// that follows the revert retires under the candidate's fence, which sees the +/// pod at a newer resourceVersion and refuses: the pod and its execution are +/// kept, so the sweeper puts the row back to `restoring` rather than losing +/// what the pod served. (That the revert leaves this shape with retirement +/// disabled is pinned in the sweeper's own store tests.) +#[tokio::test(flavor = "current_thread")] +async fn without_retirement_the_sweeper_cleanup_keeps_a_restore_pod_that_came_up_after_its_check() +-> TestResult { + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let pod_name = "space-sbx-late-serve-g1"; + insert_test_sandbox_row(&db.pool, sandbox_id, "suspended").await?; + // What the lifecycle-only revert left: the restore's execution still + // active and the row still placed. + insert_execution( + &db.pool, + sandbox_id, + ExecutionSeed::running(pod_name, "late-serve-uid"), + ) + .await?; + sqlx::query(sql!( + "UPDATE sandboxes SET placement = 'on_node' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + // The pod bound and came up after the check: a newer resourceVersion. + let (client, log) = scripted_kube_client(move |request| match request.method { + Method::DELETE => kube_status(StatusCode::OK, "Success"), + _ => { + let mut pod = (*sandbox_pod_on_node(pod_name, WORKER_NODE, 1, "Running")).clone(); + pod.metadata.uid = Some("late-serve-uid".to_string()); + pod.metadata.resource_version = Some("2".to_string()); + pod_reply(&pod) + } + }); + let substrate = substrate_with_store_and_client(db.pool.clone(), client); + let observed = PodIdentityFence::new(PodName::parse(pod_name)?) + .with_recorded(RecordedExecution::Read { + pod_uid: Some(PodUid::parse("late-serve-uid")?), + route_activation_id: None, + }) + .with_generation(1) + .with_pod_uid(PodUid::parse("late-serve-uid")?) + .with_resource_version(PodResourceVersion::parse("1")?); + + let outcome = substrate + .retire_execution(RetireExecutionRequest::fenced(sandbox_id, observed)) + .await?; + + assert!( + matches!(outcome, RetireExecutionOutcome::PodIdentityChanged { .. }), + "the cleanup must refuse a pod that came up after the check: {outcome:?}" + ); + assert!( + requests_matching(&log, &Method::DELETE).is_empty(), + "a pod that may have served a request must not be deleted" + ); + assert_eq!(active_execution_count(&db.pool, sandbox_id).await?, 1); + Ok(()) +} + +/// The transient-lifecycle sweeper reverts an aged `restoring` row by +/// retiring its execution as `replaced` in the revert's own transaction, and +/// only after checking the candidate's live pod. A restore pod that binds and +/// comes up after that check is the abandoned restore's leftover: the cleanup +/// that follows deletes it rather than letting it recover, the row stays +/// `suspended`, and restore still selects the durable pre-restore snapshot +/// the revert proved. +#[tokio::test(flavor = "current_thread")] +async fn sweeper_revert_cleanup_deletes_a_restore_pod_that_bound_after_its_check() -> TestResult { + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let pod_name = "space-sbx-late-bind-g1"; + let source_uri = "s3://space-test/pre-restore?region=us-east-1"; + insert_test_sandbox_row(&db.pool, sandbox_id, "suspended").await?; + sqlx::query(sql!( + "INSERT INTO snapshots (id, sandbox_id, type, state, timestamp, org_id) + VALUES ($1, $2, 'full', 'ready', NOW() - INTERVAL '10 minutes', + '00000000-0000-0000-0000-000000000001')" + )) + .bind(Uuid::now_v7()) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + sqlx::query(sql!( + "INSERT INTO snapshot_storage_locations (snapshot_id, storage_connection_string, org_id) + SELECT id, $2, org_id FROM snapshots WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .bind(source_uri) + .execute(&db.pool) + .await?; + // What the sweeper's revert left: its execution retired as an abandoned + // restore, the row still reading as placed from when the pod bound, and + // the placement clock the bind moved past the snapshot cleared. + insert_execution( + &db.pool, + sandbox_id, + ExecutionSeed { + state: "replaced", + ..ExecutionSeed::running(pod_name, "late-bind-uid") + }, + ) + .await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions SET active = FALSE WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + sqlx::query(sql!( + "UPDATE sandboxes + SET placement = 'on_node', + placement_changed_at = NULL + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + // The live pod has moved on since the candidate observed it: it bound + // and is Running at a newer resourceVersion. + let (client, log) = scripted_kube_client(move |request| match request.method { + Method::DELETE => kube_status(StatusCode::OK, "Success"), + _ => { + let mut pod = (*sandbox_pod_on_node(pod_name, WORKER_NODE, 1, "Running")).clone(); + pod.metadata.uid = Some("late-bind-uid".to_string()); + pod.metadata.resource_version = Some("2".to_string()); + pod_reply(&pod) + } + }); + let substrate = substrate_with_store_and_client(db.pool.clone(), client); + let observed = PodIdentityFence::new(PodName::parse(pod_name)?) + .with_recorded(RecordedExecution::Read { + pod_uid: Some(PodUid::parse("late-bind-uid")?), + route_activation_id: None, + }) + .with_generation(1) + .with_pod_uid(PodUid::parse("late-bind-uid")?) + .with_resource_version(PodResourceVersion::parse("1")?); + + let outcome = substrate + .retire_execution(RetireExecutionRequest::fenced(sandbox_id, observed)) + .await?; + + assert_eq!(outcome, RetireExecutionOutcome::Retired); + let deletes = requests_matching(&log, &Method::DELETE); + assert!( + deletes + .iter() + .any(|request| request.path == pod_path(pod_name)), + "the late-bound restore pod must be deleted: {deletes:?}" + ); + assert_eq!(sandbox_lifecycle(&db.pool, sandbox_id).await?, "suspended"); + assert_eq!(active_execution_count(&db.pool, sandbox_id).await?, 0); + let sandbox = crate::controllers::sandbox::fetch_sandbox(&db.pool, sandbox_id) + .await? + .expect("sandbox row exists"); + assert_eq!(sandbox.placement, "unplaced"); + assert_eq!( + substrate + .store + .selected_suspend_restore_storage_uri(&sandbox, false) + .await? + .as_deref(), + Some(source_uri), + "the next wake must restore from the durable pre-restore snapshot" + ); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/restoring_access.rs b/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/restoring_access.rs new file mode 100644 index 000000000..e94390ae3 --- /dev/null +++ b/crates/libs/api/src/controllers/substrate/kubernetes/adapter_characterization_tests/restoring_access.rs @@ -0,0 +1,721 @@ +//! No path serving a user request dials a sandbox's pod while the sandbox is +//! `restoring`: `access_runtime_client` and `runtime_client` join the +//! in-flight restore instead, and fail retryably when it doesn't reach +//! `started` in their budget; the non-recovering `reachable_runtime_client` +//! reports the runtime unreachable. + +use axum::http::{Method, StatusCode}; + +use super::super::scripted_test_doubles::{ + KubeLog, ScriptedWorker, ScriptedWorkerConfig, kube_status, scripted_kube_client, + start_scripted_worker, +}; +use super::super::tests::{ + insert_ready_full_snapshot_relative_to_lifecycle, insert_test_sandbox_row, + substrate_with_store_client_proxy_port_and_pod, +}; +use super::*; +use crate::controllers::ControllerRetryAfterCause; + +const POD_UID: &str = "restoring-pod-uid"; +/// The restore's pod runs here, behind a live worker, so a client that +/// dialed it would connect. +const POD_IP: &str = "127.0.0.1"; + +struct Restoring { + substrate: KubernetesSubstrate, + pod_name: String, + sandbox: Sandbox, + lease_token: Uuid, + kube_log: KubeLog, + worker: ScriptedWorker, +} + +/// A `restoring` sandbox whose restore pod is up, routable and dialable: its +/// execution is `running` with the pod's IP. `lease` is how long the restore +/// lease another request holds has left; a lapsed one leaves no owner. +async fn restoring_with_live_pod( + pool: &PgPool, + sandbox_id: SandboxId, + lease: &str, +) -> Result { + insert_test_sandbox_row(pool, sandbox_id, "restoring").await?; + let lease_token = Uuid::now_v7(); + let pod_name = format!("space-sbx-{}-g1", sandbox_id.as_uuid().simple()); + sqlx::query(sql!( + "UPDATE sandboxes + SET restore_lease_expires_at = NOW() + $2::interval, + restore_lease_token = $3 + WHERE id = $1" + )) + .bind(sandbox_id) + .bind(lease) + .bind(lease_token) + .execute(pool) + .await?; + insert_execution( + pool, + sandbox_id, + ExecutionSeed { + pod_ip: Some(POD_IP), + ..ExecutionSeed::running(&pod_name, POD_UID) + }, + ) + .await?; + let worker = start_scripted_worker(ScriptedWorkerConfig::new([])).await?; + let (client, kube_log) = scripted_kube_client(|request| match request.method { + Method::GET => kube_status(StatusCode::NOT_FOUND, "NotFound"), + _ => kube_status(StatusCode::BAD_REQUEST, "BadRequest"), + }); + let substrate = substrate_with_store_client_proxy_port_and_pod( + pool.clone(), + client, + worker.port, + Some(sandbox_pod( + &pod_name, + POD_UID, + sandbox_id, + 1, + "Running", + Some(POD_IP), + )), + ); + let sandbox = crate::controllers::sandbox::fetch_sandbox(pool, sandbox_id) + .await? + .expect("sandbox row exists"); + Ok(Restoring { + pod_name, + substrate, + sandbox, + lease_token, + kube_log, + worker, + }) +} + +/// The restore's owner finishes it after `delay`. +fn complete_restore_after( + pool: &PgPool, + sandbox_id: SandboxId, + delay: Duration, +) -> tokio::task::JoinHandle> { + let pool = pool.clone(); + tokio::spawn(async move { + tokio::time::sleep(delay).await; + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'started' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&pool) + .await?; + Ok(()) + }) +} + +fn access_identity() -> SandboxApiMetricIdentity { + SandboxApiMetricIdentity::native("restoring_access") +} + +fn assert_retryable_restore_in_flight(error: &ControllerError) { + assert_eq!(error.status(), StatusCode::SERVICE_UNAVAILABLE, "{error:?}"); + assert_eq!( + error.retry_after_cause(), + Some(ControllerRetryAfterCause::RestoreInFlight), + "{error:?}" + ); +} + +/// An access request on a restoring sandbox waits for the restore and gets a +/// client only once the sandbox is `started`, although its pod was dialable +/// the whole time. +#[tokio::test(flavor = "current_thread")] +async fn access_on_a_restoring_sandbox_joins_the_restore_before_dialing() -> TestResult { + // The joined client connects to the worker over internal RPC. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + let owner = complete_restore_after(&db.pool, sandbox_id, Duration::from_millis(300)); + + let client = restoring + .substrate + .access_runtime_client( + access_identity(), + &restoring.sandbox, + ExecutionActivityKind::Access, + Duration::from_secs(10), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + let lifecycle_at_return = sandbox_lifecycle(&db.pool, sandbox_id).await?; + owner.await??; + + assert!(client.is_ok(), "{:?}", client.err()); + assert_eq!( + lifecycle_at_return, "started", + "no client may be handed out while the sandbox is restoring" + ); + Ok(()) +} + +/// A request that wakes a `suspended` sandbox and joins an in-process +/// restore run gets the run's activation as soon as the restore's activation +/// completes, which can be before the row is `started` (the run's owner +/// commit may not have landed). The request still gets its client only once +/// the sandbox is `started`, and it keeps waiting while the row reads as +/// unowned. +/// +/// The row has no live owner, so a request that didn't join the run would +/// claim the restore itself and fail at pod creation. +#[tokio::test(flavor = "current_thread")] +async fn a_restore_run_that_completes_before_started_is_dialed_only_once_started() -> TestResult { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "-1 hour").await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions + SET updated_at = NOW() - INTERVAL '1 hour' + WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + let binding = ExecutionBinding { + execution_object_id: restoring.pod_name.clone(), + runtime_endpoint: KubernetesSubstrate::endpoint_for_pod_ip( + PodIp::parse(POD_IP)?, + restoring.worker.port, + )?, + execution_generation: 1, + }; + let release = std::sync::Arc::new(tokio::sync::Notify::new()); + let run_release = release.clone(); + let run_substrate = restoring.substrate.clone(); + let run = tokio::spawn(async move { + run_substrate + .wake_coalescer + .join_or_start( + sandbox_id, + tokio::time::Instant::now() + Duration::from_secs(10), + move || async move { + run_release.notified().await; + Ok(crate::controllers::substrate::ExecutionActivation::joined( + binding, + )) + }, + ) + .await + .is_ok() + }); + tokio::time::sleep(Duration::from_millis(20)).await; + assert_eq!(restoring.substrate.wake_coalescer.live_runs(), 1); + + let request_substrate = restoring.substrate.clone(); + let request_sandbox = restoring.sandbox.clone(); + let request_pool = db.pool.clone(); + let request = tokio::spawn(async move { + // The caller read the row before the restore claimed it. + let mut suspended_view = request_sandbox; + suspended_view.lifecycle = "suspended".to_string(); + let client = request_substrate + .wake_and_dial_once_started( + &suspended_view, + Duration::from_secs(10), + access_identity(), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + let lifecycle_at_return = sandbox_lifecycle(&request_pool, sandbox_id).await; + (client.map(|_| ()), lifecycle_at_return) + }); + // Let the request reach the run and join it, then finish the run. + tokio::time::sleep(Duration::from_millis(300)).await; + release.notify_one(); + let owner = complete_restore_after(&db.pool, sandbox_id, Duration::from_millis(300)); + + let (client, lifecycle_at_return) = request.await?; + owner.await??; + + assert!(run.await?, "the seeded restore run completes"); + assert!(client.is_ok(), "{:?}", client.err()); + assert_eq!( + lifecycle_at_return?, "started", + "no client may be handed out while the sandbox is restoring" + ); + Ok(()) +} + +/// A request that saw the sandbox `restoring` and finds it `started` when +/// it joins dials it, rather than failing a restore that already succeeded. +#[tokio::test(flavor = "current_thread")] +async fn a_restore_that_finished_before_the_join_is_dialed() -> TestResult { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + complete_restore_after(&db.pool, sandbox_id, Duration::ZERO).await??; + + let client = restoring + .substrate + .join_restore_before_dial( + sandbox_id, + access_identity(), + Duration::from_secs(10), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + + assert!(client.is_ok(), "{:?}", client.err()); + Ok(()) +} + +/// An access request whose restore doesn't finish in its budget gets a +/// retryable `RestoreInFlight`, never a client for the restoring pod. +#[tokio::test(flavor = "current_thread")] +async fn access_on_a_restoring_sandbox_is_refused_retryably_when_the_restore_lingers() -> TestResult +{ + // Credentials make the pod connectable, so only the gate can refuse it. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + + let error = restoring + .substrate + .access_runtime_client( + access_identity(), + &restoring.sandbox, + ExecutionActivityKind::Access, + Duration::from_secs(2), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await + .expect_err("a restoring pod must not be dialed"); + + assert_retryable_restore_in_flight(&error); + assert_eq!(sandbox_lifecycle(&db.pool, sandbox_id).await?, "restoring"); + Ok(()) +} + +/// `runtime_client`'s user paths join the restore the same way. +#[tokio::test(flavor = "current_thread")] +async fn runtime_client_on_a_restoring_sandbox_joins_the_restore_before_dialing() -> TestResult { + // The joined client connects to the worker over internal RPC. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + let owner = complete_restore_after(&db.pool, sandbox_id, Duration::from_millis(300)); + + let client = restoring + .substrate + .runtime_client( + sandbox_id, + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + let lifecycle_at_return = sandbox_lifecycle(&db.pool, sandbox_id).await?; + owner.await??; + + assert!(client.is_ok(), "{:?}", client.err()); + assert_eq!(lifecycle_at_return, "started"); + Ok(()) +} + +/// The non-recovering `reachable_runtime_client` neither dials a restoring +/// pod nor waits for its restore: its callers are best-effort (the ECW +/// keepalive refresh) or turn an unreachable runtime into a retryable 503, +/// so it reports the runtime unreachable at once, whether or not an owner is +/// still restoring the sandbox. +#[tokio::test(flavor = "current_thread")] +async fn reachable_runtime_client_finds_no_client_for_a_restoring_sandbox() -> TestResult { + // Credentials make the pod connectable, so only the gate can refuse it. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + for lease in ["120 seconds", "-1 hour"] { + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, lease).await?; + let started = std::time::Instant::now(); + let client = restoring + .substrate + .reachable_runtime_client(sandbox_id) + .await?; + assert!( + client.is_none(), + "{lease}: a restoring pod must not be dialed" + ); + assert!( + started.elapsed() < Duration::from_secs(5), + "{lease}: the probe must not wait out the restore: {:?}", + started.elapsed() + ); + assert_eq!(sandbox_lifecycle(&db.pool, sandbox_id).await?, "restoring"); + } + Ok(()) +} + +/// A drain's evacuation restore owns its `restoring` row through the +/// evacuation marker rather than a lease, so the row reads as unowned. A user +/// request on it waits for the drain to move it to `started` and drives no +/// wake of its own (the scripted worker answers no wake). +#[tokio::test(flavor = "current_thread")] +async fn access_on_an_evacuating_sandbox_waits_for_the_drain() -> TestResult { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "-1 hour").await?; + sqlx::query(sql!( + "UPDATE sandboxes SET evacuation_restore_target = 'started' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + let drain = complete_restore_after(&db.pool, sandbox_id, Duration::from_millis(300)); + + let client = restoring + .substrate + .access_runtime_client( + access_identity(), + &restoring.sandbox, + ExecutionActivityKind::Access, + Duration::from_secs(10), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + let lifecycle_at_return = sandbox_lifecycle(&db.pool, sandbox_id).await?; + drain.await??; + + assert!(client.is_ok(), "{:?}", client.err()); + assert_eq!(lifecycle_at_return, "started"); + Ok(()) +} + +/// A request that joins a restore whose owner then gives up starts no +/// restore of its own: one the owner rolled back to `suspended` is refused +/// retryably, and one that went to a state that can't be woken gets that +/// state's error. Either way no pod is created; a fresh restore is for a +/// caller whose access-wake policy and admission allow it. +#[tokio::test(flavor = "current_thread")] +async fn a_joined_restore_that_its_owner_abandons_starts_no_restore() -> TestResult { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + for abandoned_to in ["suspended", "failed"] { + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + // A durable source, so a fresh restore of the reverted row could + // start. + insert_ready_full_snapshot_relative_to_lifecycle( + &db.pool, + sandbox_id, + "s3://bucket/pre-restore?region=us-east-1", + -60, + Uuid::now_v7(), + ) + .await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions SET created_at = NOW() WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + let owner_substrate = restoring.substrate.clone(); + let owner_pool = db.pool.clone(); + let lease_token = restoring.lease_token; + let owner = tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(300)).await; + if abandoned_to == "suspended" { + owner_substrate + .store + .revert_restore_claim_to_suspended( + sandbox_id, + Some(lease_token), + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) + .await?; + } else { + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'failed' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&owner_pool) + .await?; + } + Ok::<_, BoxError>(()) + }); + + let refused = restoring + .substrate + .access_runtime_client( + access_identity(), + &restoring.sandbox, + ExecutionActivityKind::Access, + Duration::from_secs(10), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await + .expect_err("an abandoned restore serves no client"); + owner.await??; + + if abandoned_to == "suspended" { + assert_retryable_restore_in_flight(&refused); + } else { + assert_eq!(refused.status(), StatusCode::CONFLICT, "{refused:?}"); + } + assert_eq!(sandbox_lifecycle(&db.pool, sandbox_id).await?, abandoned_to); + let pod_writes = restoring + .kube_log + .lock() + .expect("kube log") + .iter() + .filter(|request| request.method != Method::GET) + .count(); + assert_eq!(pod_writes, 0, "{abandoned_to}: no restore may be started"); + } + Ok(()) +} + +/// A request that arrives during a restore that then fails is refused +/// without a client, so when the API rollback retires the restore's execution +/// and deletes its pod, nothing that pod served is lost: it served nothing. +#[tokio::test(flavor = "current_thread")] +async fn a_rolled_back_restore_discards_no_request_it_served() -> TestResult { + // Credentials make the pod connectable, so only the gate can refuse it. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + insert_ready_full_snapshot_relative_to_lifecycle( + &db.pool, + sandbox_id, + "s3://bucket/pre-restore?region=us-east-1", + -60, + Uuid::now_v7(), + ) + .await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions SET created_at = NOW() WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + + let refused = restoring + .substrate + .access_runtime_client( + access_identity(), + &restoring.sandbox, + ExecutionActivityKind::Access, + Duration::from_secs(2), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await + .expect_err("a restoring pod must not be dialed"); + assert_retryable_restore_in_flight(&refused); + + let revert = restoring + .substrate + .store + .revert_restore_claim_to_suspended( + sandbox_id, + Some(restoring.lease_token), + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) + .await?; + + assert_eq!( + revert, + RestoreClaimRevert::ExecutionRetired { + pod_name: restoring.pod_name.clone() + } + ); + assert_eq!(sandbox_lifecycle(&db.pool, sandbox_id).await?, "suspended"); + Ok(()) +} + +/// The substrate's rollback of a failed fresh restore claim retires the +/// restore's execution and deletes its pod only with abandoned-restore +/// retirement enabled. Disabled, the default until every routing replica +/// refuses to route to a `restoring` sandbox, it reverts only the lifecycle +/// and deletes nothing, so a pod an older replica may have routed a request +/// to survives the rollback. +#[tokio::test(flavor = "current_thread")] +async fn a_failed_restore_rollback_deletes_its_pod_only_with_retirement_enabled() -> TestResult { + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + for retirement in [ + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + crate::controllers::substrate::AbandonedRestoreRetirement::Disabled, + ] { + let sandbox_id = SandboxId::new(); + let mut restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + restoring.substrate.k8s.abandoned_restore_retirement = retirement; + insert_ready_full_snapshot_relative_to_lifecycle( + &db.pool, + sandbox_id, + "s3://bucket/pre-restore?region=us-east-1", + -60, + Uuid::now_v7(), + ) + .await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions SET created_at = NOW() WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + + restoring + .substrate + .revert_failed_restore_claim( + sandbox_id, + super::super::store::RestoreClaim::FreshFromSuspended { + restore_lease_token: Some(restoring.lease_token), + }, + None, + ) + .await; + + let deleted = restoring + .kube_log + .lock() + .expect("kube log") + .iter() + .any(|request| request.method == Method::DELETE); + let active: i64 = sqlx::query_scalar(sql!( + "SELECT COUNT(*) FROM sandbox_k8s_executions WHERE sandbox_id = $1 AND active" + )) + .bind(sandbox_id) + .fetch_one(&db.pool) + .await?; + assert_eq!(sandbox_lifecycle(&db.pool, sandbox_id).await?, "suspended"); + assert_eq!( + (deleted, active), + if retirement.is_enabled() { + (true, 0) + } else { + (false, 1) + }, + "{retirement:?}" + ); + } + Ok(()) +} + +/// When the restoring sandbox's pod isn't resolvable yet (its execution is +/// still `creating`), the fallback joins the restore too, rather than +/// materializing a pod of its own: both `access_runtime_client` and +/// `runtime_client` return only once the owner has brought the sandbox to +/// `started`. +#[tokio::test(flavor = "current_thread")] +async fn a_restoring_sandbox_without_a_resolved_pod_joins_the_restore() -> TestResult { + // The joined client connects to the worker over internal RPC. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + for via_access in [true, false] { + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions + SET state = 'creating', pod_uid = NULL, pod_ip = NULL + WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(&db.pool) + .await?; + let pool = db.pool.clone(); + let owner = tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(300)).await; + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions + SET state = 'running', pod_uid = $2, pod_ip = $3::INET + WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .bind(POD_UID) + .bind(POD_IP) + .execute(&pool) + .await?; + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'started' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&pool) + .await?; + Ok::<_, BoxError>(()) + }); + + let client = if via_access { + restoring + .substrate + .access_runtime_client( + access_identity(), + &restoring.sandbox, + ExecutionActivityKind::Access, + Duration::from_secs(10), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await + } else { + restoring + .substrate + .runtime_client( + sandbox_id, + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await + }; + let lifecycle_at_return = sandbox_lifecycle(&db.pool, sandbox_id).await?; + owner.await??; + + assert!( + client.is_ok(), + "via_access={via_access}: {:?}", + client.err() + ); + assert_eq!(lifecycle_at_return, "started", "via_access={via_access}"); + } + Ok(()) +} + +/// The reachability probe that replays a runtime RPC on a fresh client finds +/// no client for a restoring pod. +#[tokio::test(flavor = "current_thread")] +async fn probe_runtime_client_finds_no_client_for_a_restoring_pod() -> TestResult { + // Credentials make the pod connectable, so only the gate can refuse it. + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let sandbox_id = SandboxId::new(); + let restoring = restoring_with_live_pod(&db.pool, sandbox_id, "120 seconds").await?; + assert!( + restoring + .substrate + .probe_runtime_client(sandbox_id) + .await? + .is_none() + ); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/execution.rs b/crates/libs/api/src/controllers/substrate/kubernetes/execution.rs index 4e072c1e4..156c90186 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/execution.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/execution.rs @@ -176,6 +176,37 @@ pub(super) fn ensure_wake_recovery_lifecycle(sandbox: &Sandbox) -> Result<(), Su } } +/// What [`KubernetesSubstrate::reclaim_lapsed_restore`] did. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LapsedRestoreReclaim { + /// This request drove the restore's wake under its own claim. + Reclaimed, + /// An owner holds the restore again; keep joining it. + OwnerReturned, + /// The row isn't this request's to reclaim (a drain owns it, it moved + /// on, or too little budget is left). + Declined, +} + +/// Below this much of its readiness budget, a request joining a restore +/// whose owner lapsed doesn't reclaim it: the claim would take the lease only +/// to fail its wake at once. +const MIN_RESTORE_RECLAIM_WINDOW: Duration = Duration::from_secs(1); + +/// The error for a request whose sandbox left `restoring` other than for +/// `started`: the error a wake would give a state that can't be woken, and +/// otherwise (a rollback to `suspended`, say) a retryable `RestoreInFlight`, +/// so the retry acts on the current state. +fn left_restoring_error(sandbox: &Sandbox) -> SubstrateError { + match ensure_wake_recovery_lifecycle(sandbox) { + Err(error) => error, + Ok(()) => SubstrateError::restore_in_flight(format!( + "sandbox {} went {} before its restore reached started; retry the same sandbox", + sandbox.id, sandbox.lifecycle + )), + } +} + #[derive(Clone)] struct RestoreClaimHandling { preclaimed: Option, @@ -368,6 +399,62 @@ impl KubernetesSubstrate { } } + /// Best-effort rollback of a failed fresh restore claim to `suspended` + /// (see [`K8sStore::revert_restore_claim_to_suspended`]). When the + /// rollback retired the claim's execution, its pod is deleted here too. + /// That delete is best-effort: a pod it misses belongs to no active + /// execution, so the orphaned-restore-pod GC and the next restore's + /// lingering-pod sweep both reap it. + pub(super) async fn revert_failed_restore_claim( + &self, + sandbox_id: SandboxId, + restore_claim: RestoreClaim, + activation_attempt: Option<(SandboxActivationAttemptId, i64)>, + ) { + let outcome = match self + .store + .revert_restore_claim_to_suspended( + sandbox_id, + restore_claim.restore_lease_token(), + activation_attempt, + self.k8s.abandoned_restore_retirement, + ) + .await + { + Ok(outcome) => outcome, + Err(err) => { + tracing::warn!( + sandbox_id = %sandbox_id, + error = %err, + "failed to restore suspended lifecycle after Kubernetes restore failure" + ); + return; + } + }; + pplx_space_metrics::sandbox::restore::claim_reverted(outcome.metric_outcome()); + match outcome { + RestoreClaimRevert::ExecutionRetired { pod_name } => { + // A retired generation is never reused, so the name can't + // belong to a successor. + if let Err(err) = self.delete_tracked_pods(vec![pod_name]).await { + tracing::warn!( + sandbox_id = %sandbox_id, + error = %err, + "failed to delete the pod of an abandoned restore; it has no active \ + execution, so orphan GC or the next restore removes it" + ); + } + } + RestoreClaimRevert::ExecutionLeftActive => tracing::warn!( + sandbox_id = %sandbox_id, + "reverted a failed restore with an execution still active: either restore \ + selection isn't proven to choose a durable pre-restore source, or the claim \ + reused that execution rather than creating it" + ), + RestoreClaimRevert::LifecycleOnly | RestoreClaimRevert::NotReverted => {} + } + } + /// Best-effort release of a restore claim whose deferred admission /// charge was rejected: a fresh suspended claim rolls back to /// `suspended`; a redrive keeps the row `restoring` and only frees the @@ -379,13 +466,9 @@ impl KubernetesSubstrate { ) { let result = match restore_claim { RestoreClaim::FreshFromSuspended { .. } => { - self.store - .revert_lifecycle_after_rollback_best_effort( - sandbox_id, - "suspended", - restore_claim.restore_lease_token(), - ) - .await + return self + .revert_failed_restore_claim(sandbox_id, restore_claim, None) + .await; } RestoreClaim::RedroveRestoring { restore_lease_token: Some(token), @@ -584,6 +667,31 @@ impl KubernetesSubstrate { }) } + /// Redrive the restore of a sandbox whose started recovery a suspend + /// superseded, for an access request. The binding is handed out only once + /// the row is `started` (see `join_restore_before_dial`): if the redrive's + /// owner couldn't commit that, its restore may still be rolled back. + pub(super) async fn redrive_suspended_recovery_for_access( + &self, + sandbox: &Sandbox, + retired_generation: Option, + join_deadline: Instant, + metric_identity: pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity, + charge: RecoveryCharge, + ) -> Result { + self.redrive_started_recovery_after_suspend( + sandbox, + retired_generation, + join_deadline, + metric_identity, + charge, + None, + ) + .await + .map_err(ControllerError::from)?; + self.binding_once_started(sandbox.id, join_deadline).await + } + /// Bounded follower join on the user restore lease. The process-wide /// restore poller batch-observes every pending sandbox; this request owns /// only a watch receiver until the owner publishes a routable outcome, @@ -658,6 +766,223 @@ impl KubernetesSubstrate { } } + /// A request a user is waiting on never dials a sandbox's pod while the + /// sandbox is `restoring`: the restore can still fail and be rolled back, + /// and the rollback retires its execution and deletes the pod, discarding + /// whatever it served. Instead this joins the in-flight restore and, once + /// the sandbox is `started`, returns a client for the execution the row + /// routes to. A row the caller saw `restoring` that is already `started` + /// here is dialed the same way. + /// + /// It joins before it claims, waiting on the shared restore poller rather + /// than polling the row per request. Only a row still `restoring` once its + /// owner's lease and execution ownership have lapsed, and that no drain + /// owns, is reclaimed: this request takes the restoring lease itself and + /// drives the wake under that claim, whose owner commits `started`. A + /// follower whose owner rolled back to `suspended` gets `RestoreInFlight` + /// rather than starting a fresh restore that its caller's access-wake + /// policy and admission never approved. Past `timeout` this fails as + /// `RestoreInFlight` too, a retryable 503; a row that leaves for a state + /// that can't be woken gets that state's error. + pub(super) async fn join_restore_before_dial( + &self, + sandbox_id: SandboxId, + metric_identity: pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity, + timeout: Duration, + charge: RecoveryCharge, + ) -> Result { + let (_, join_deadline) = ready_wait_bounds(timeout); + let sandbox = self + .store + .require_sandbox(sandbox_id) + .await + .map_err(ControllerError::from)?; + match parse_sandbox_lifecycle(&sandbox.lifecycle).map_err(ControllerError::from)? { + SandboxLifecycle::Started => {} + SandboxLifecycle::Restoring => { + let mut backoff = RecoveryJoinBackoff::new(self.recovery_join_backoff); + while let UserRestoreJoin::Claimable = self + .wait_for_user_restore(sandbox_id, join_deadline, None, &mut backoff) + .await + .map_err(ControllerError::from)? + { + let reclaim = self + .reclaim_lapsed_restore( + sandbox_id, + join_deadline, + metric_identity, + charge.clone(), + ) + .await?; + if reclaim != LapsedRestoreReclaim::OwnerReturned { + break; + } + } + } + _ => return Err(ControllerError::from(left_restoring_error(&sandbox))), + } + self.dial_once_started(sandbox_id, join_deadline).await + } + + /// Reclaim a `restoring` row whose owner's lease and execution ownership + /// lapsed, for [`Self::join_restore_before_dial`]. The lease claim matches + /// only a row still `restoring`, unowned and unmarked; this request then + /// drives the wake under that claim, and the wake's owner commits + /// `started`. A row a drain owns, or that moved on, is declined and left + /// to the caller's `started` wait. + async fn reclaim_lapsed_restore( + &self, + sandbox_id: SandboxId, + join_deadline: Instant, + metric_identity: pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity, + charge: RecoveryCharge, + ) -> Result { + if remaining_ready_window(join_deadline) < MIN_RESTORE_RECLAIM_WINDOW { + return Ok(LapsedRestoreReclaim::Declined); + } + let restore_claim = match self + .store + .claim_markerless_restoring_restore_lease(sandbox_id) + .await + { + Ok(Some(restore_claim)) => restore_claim, + Ok(None) | Err(SubstrateError::Conflict { .. }) => { + return Ok(LapsedRestoreReclaim::Declined); + } + Err(SubstrateError::RestoreInFlight { .. }) => { + return Ok(LapsedRestoreReclaim::OwnerReturned); + } + Err(error) => return Err(ControllerError::from(error)), + }; + if let Err(error) = charge.charge_restore_claim().await { + self.release_rejected_restore_claim(sandbox_id, restore_claim) + .await; + return Err(ControllerError::from(error)); + } + let sandbox = self + .store + .require_sandbox(sandbox_id) + .await + .map_err(ControllerError::from)?; + // Only a writer that ignores the lease (the tokenless evacuation + // rollback) can move the row now; and the charge may have used up the + // budget. Either way, hand the restore back rather than drive it. + if !is_restoring_lifecycle(&sandbox.lifecycle) + || remaining_ready_window(join_deadline) < MIN_RESTORE_RECLAIM_WINDOW + { + self.release_rejected_restore_claim(sandbox_id, restore_claim) + .await; + return Ok(LapsedRestoreReclaim::Declined); + } + self.wake_execution_inner( + &sandbox, + remaining_ready_window(join_deadline), + metric_identity, + None, + WakeTarget::started(None), + RestoreClaimHandling::preclaimed_with_cleanup(restore_claim), + ) + .await + .map_err(ControllerError::from)?; + Ok(LapsedRestoreReclaim::Reclaimed) + } + + /// Wake a dormant sandbox for a user request and dial it once it is + /// `started`. A restore-shaped wake leaves the row `restoring` until its + /// owner commits `started` (or, for a joined run, its owner did), so the + /// binding the wake returns isn't dialed directly (see + /// `join_restore_before_dial`). + pub(super) async fn wake_and_dial_once_started( + &self, + sandbox: &Sandbox, + timeout: Duration, + metric_identity: pplx_space_metrics::sandbox::api::SandboxApiMetricIdentity, + charge: RecoveryCharge, + ) -> Result { + let (_, join_deadline) = ready_wait_bounds(timeout); + self.wake_execution_with_metric_identity(sandbox, timeout, metric_identity, charge) + .await + .map_err(ControllerError::from)?; + self.dial_once_started(sandbox.id, join_deadline).await + } + + /// Dial the execution a sandbox routes to once the row is `started`, + /// polling with the recovery-join backoff until `deadline`. Observes before + /// each sleep, so a row that is already `started` is dialed at once. A + /// `restoring` row that reads as unowned is still waited on: a drain's + /// evacuation restore owns its row through the marker, not the lease, and + /// only the drain moves it to `started`. + async fn dial_once_started( + &self, + sandbox_id: SandboxId, + deadline: Instant, + ) -> Result { + let binding = self.binding_once_started(sandbox_id, deadline).await?; + crate::controllers::substrate::node_api_client::connect_worker(binding.runtime_endpoint) + .await + } + + /// The binding [`Self::dial_once_started`] dials. + async fn binding_once_started( + &self, + sandbox_id: SandboxId, + deadline: Instant, + ) -> Result { + let mut backoff = RecoveryJoinBackoff::new(self.recovery_join_backoff); + loop { + match self + .store + .observe_user_restore(sandbox_id) + .await + .map_err(ControllerError::from)? + { + UserRestoreObservation::Routable { + generation, + pod_name, + pod_ip, + } => { + return self + .binding_for_routable_pod(generation, pod_name, &pod_ip) + .map_err(ControllerError::from); + } + UserRestoreObservation::InFlight { .. } => {} + UserRestoreObservation::Claimable => { + let sandbox = self + .store + .require_sandbox(sandbox_id) + .await + .map_err(ControllerError::from)?; + if normalize_lifecycle(&sandbox.lifecycle) == "started" { + // Moved on since the observation: observe again now. + continue; + } + if !is_restoring_lifecycle(&sandbox.lifecycle) { + return Err(ControllerError::from(left_restoring_error(&sandbox))); + } + } + UserRestoreObservation::StartedUnroutable => { + return Err(ControllerError::from(SubstrateError::restore_in_flight( + format!( + "sandbox {sandbox_id} is started without a routable execution; \ + retry the same sandbox" + ), + ))); + } + } + if tokio::time::timeout_at(deadline, tokio::time::sleep(backoff.next_delay())) + .await + .is_err() + { + return Err(ControllerError::from(SubstrateError::restore_in_flight( + format!( + "sandbox {sandbox_id}'s restore didn't reach started in time; \ + retry the same sandbox" + ), + ))); + } + } + } + /// Claim or join started recovery. Followers wait without spending /// admission; owners charge before Pod creation and publish completion. /// The access path takes no caller-selected `suspend_restore_source_node`. @@ -698,17 +1023,14 @@ impl KubernetesSubstrate { retired_generation, } => { return self - .redrive_started_recovery_after_suspend( + .redrive_suspended_recovery_for_access( &sandbox, retired_generation, join_deadline, metric_identity, charge, - None, ) - .await - .map(|activation| activation.binding) - .map_err(ControllerError::from); + .await; } } } else { @@ -860,17 +1182,14 @@ impl KubernetesSubstrate { .await; } return self - .redrive_started_recovery_after_suspend( + .redrive_suspended_recovery_for_access( &sandbox, Some(generation), join_deadline, metric_identity, RecoveryCharge::Exempt, - None, ) - .await - .map(|activation| activation.binding) - .map_err(ControllerError::from); + .await; } self.publish_started_recovery_result( @@ -1909,22 +2228,14 @@ impl KubernetesSubstrate { result.as_ref().err(), wake_dispatch, ) - && let Err(update_err) = self - .store - .revert_lifecycle_after_rollback_best_effort( - sandbox_id, - "suspended", - prepared - .restore_claim - .and_then(RestoreClaim::restore_lease_token), - ) - .await + && let Some(restore_claim) = prepared.restore_claim { - tracing::warn!( - sandbox_id = %sandbox_id, - error = %update_err, - "failed to restore suspended lifecycle after Kubernetes wake failure" - ); + self.revert_failed_restore_claim( + sandbox_id, + restore_claim, + activation_assignment.assigned(), + ) + .await; } if cleanup_on_failure && !restore_failure_should_revert_to_suspended( @@ -1957,6 +2268,48 @@ impl KubernetesSubstrate { ) .await; } + // The owner of a user restore commits `restoring -> started` itself, + // under its own lease, before handing out the binding: a user request + // never dials a `restoring` pod (see `join_restore_before_dial`), and + // on the access path no lifecycle-controller completion follows. A + // lost or failed CAS leaves the row to the controller's completion or + // the node's published state, which those requests then wait for. + if result.is_ok() + && target_state == SandboxLifecycle::Started + && evacuation_placement.is_none() + && let Some(restore_lease_token) = prepared + .restore_claim + .and_then(RestoreClaim::restore_lease_token) + { + match self + .store + .complete_owned_user_restore(sandbox_id, restore_lease_token) + .await + { + // Reported here, in the restore's detached task, so a caller + // that hung up can't drop it; the node's published state + // finds the row `started` and reports nothing. The lifecycle + // controller's actions report their own completion too, as + // for `WakeCompleted`. + Ok(true) => SandboxEvent::new( + sandbox_id, + SandboxEventType::RestoreCompleted, + SandboxEventSource::Substrate, + ) + .emit_with( + SandboxEventDetails::default() + .reason("restore_owner_completed") + .with_from_state("restoring") + .with_to_state("started"), + ), + Ok(false) => {} + Err(error) => tracing::warn!( + sandbox_id = %sandbox_id, + %error, + "failed to commit started after an owned restore" + ), + } + } result.map(|(binding, _)| ExecutionActivation::owned(binding)) } } diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/mod.rs b/crates/libs/api/src/controllers/substrate/kubernetes/mod.rs index e459d95a5..62996cb3b 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/mod.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/mod.rs @@ -70,8 +70,9 @@ use cancel_rate_tracker::CancelRateTracker; use restore_waiters::RestoreWaiters; use store::{ ActiveExecutionState, K8sStore, NodeLifecycleState, NodeStateWrite, PlacementClock, - PreparedWakeSandbox, RestoreClaim, SandboxExecutionRow, StartedRecoveryClaim, - StartedRecoveryCompletion, StartedRecoveryObservation, UserRestoreObservation, + PreparedWakeSandbox, RestoreClaim, RestoreClaimRevert, SandboxExecutionRow, + StartedRecoveryClaim, StartedRecoveryCompletion, StartedRecoveryObservation, + UserRestoreObservation, }; use crate::{ @@ -370,6 +371,10 @@ struct ResolvedPodInstance { ip: PodIp, pod_uid: Option, generation: i64, + /// The sandbox row read with the execution was `restoring`. A path that + /// serves a user request must not dial such a pod (see + /// `KubernetesSubstrate::join_restore_before_dial`). + sandbox_restoring: bool, } impl ResolvedPodInstance { @@ -419,6 +424,9 @@ pub struct KubernetesState { /// plane does not publish `suspended` while kubelet/containerd teardown for /// the old sandbox pod can still run later. pub k8s_blocking_delete_timeout: Duration, + /// Whether a failed restore claim's rollback retires its execution + /// (`SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED`, default off). + pub abandoned_restore_retirement: super::AbandonedRestoreRetirement, pub caches: Arc, } @@ -471,6 +479,7 @@ impl KubernetesState { create_command_ready_timeout, create_data_plane_ready_timeout, k8s_blocking_delete_timeout: DEFAULT_K8S_BLOCKING_DELETE_TIMEOUT, + abandoned_restore_retirement: super::AbandonedRestoreRetirement::from_env(), sandbox_proxy_port: DEFAULT_SANDBOX_PROXY_PORT, node_api_port: DEFAULT_SANDBOX_PROXY_PORT, sandbox_pod_config: SandboxPodConfigValues::from_env()?, @@ -554,6 +563,10 @@ fn is_suspended_lifecycle(lifecycle: &str) -> bool { normalize_lifecycle(lifecycle) == "suspended" } +fn is_restoring_lifecycle(lifecycle: &str) -> bool { + normalize_lifecycle(lifecycle) == "restoring" +} + fn is_recoverable_live_lifecycle(lifecycle: &str) -> bool { matches!( normalize_lifecycle(lifecycle).as_str(), diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/pod_ops/read.rs b/crates/libs/api/src/controllers/substrate/kubernetes/pod_ops/read.rs index f1eb5dd49..be84cfbca 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/pod_ops/read.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/pod_ops/read.rs @@ -196,7 +196,12 @@ impl KubernetesSubstrate { let Some(row) = self.store.active_execution_status(sandbox_id).await? else { return Ok(None); }; - if is_suspended_lifecycle(&row.lifecycle) || execution_pod_lookup_in_flight(row.state) { + // A restoring pod is not reachable for a request: see + // `join_restore_before_dial`. + if is_suspended_lifecycle(&row.lifecycle) + || is_restoring_lifecycle(&row.lifecycle) + || execution_pod_lookup_in_flight(row.state) + { return Ok(None); } let Some(pod) = self @@ -466,6 +471,7 @@ impl KubernetesSubstrate { ip, pod_uid: pod_uid.as_deref().and_then(|uid| PodUid::parse(uid).ok()), generation: row.generation, + sandbox_restoring: is_restoring_lifecycle(&row.lifecycle), })) } } diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store.rs index 8da2d92a2..7a3da6dc2 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/store.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store.rs @@ -642,6 +642,39 @@ impl RestoreClaim { } } +/// What [`K8sStore::revert_restore_claim_to_suspended`] did with a failed +/// fresh restore claim. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum RestoreClaimRevert { + /// The row reverted to `suspended` and the execution the claim created + /// was retired as `replaced`, so its pod, `pod_name`, is unadoptable. + ExecutionRetired { pod_name: String }, + /// The row reverted to `suspended` with no active execution left. + LifecycleOnly, + /// The row reverted to `suspended` with an execution still active, + /// leaving a same-generation pod on a `suspended` row: either the claim's + /// own execution, because restore selection isn't proven to choose a + /// durable pre-restore source, or one the claim reused rather than + /// created, whose pod may hold state of its own. + ExecutionLeftActive, + /// The row is no longer this claim's `restoring` row; nothing changed. + NotReverted, +} + +impl RestoreClaimRevert { + pub(super) const fn metric_outcome( + &self, + ) -> pplx_space_metrics::sandbox::restore::RevertOutcome { + use pplx_space_metrics::sandbox::restore::RevertOutcome; + match self { + Self::ExecutionRetired { .. } => RevertOutcome::ExecutionRetired, + Self::LifecycleOnly => RevertOutcome::LifecycleOnly, + Self::ExecutionLeftActive => RevertOutcome::ExecutionLeftActive, + Self::NotReverted => RevertOutcome::NotReverted, + } + } +} + #[derive(sqlx::FromRow)] struct RestoreClaimRow { lifecycle: String, diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/execution_rows.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/execution_rows.rs index 98ca38ffd..64bd34161 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/store/execution_rows.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/execution_rows.rs @@ -948,7 +948,7 @@ impl K8sStore { /// The activation-attempt identity fences retries that reuse a generation. /// `restore_lease_token` additionally ownership-fences restore takeover /// (with the same `None`-is-unguarded contract as - /// [`Self::revert_lifecycle_after_rollback_best_effort`]). A dropped leader + /// [`Self::revert_restore_claim_to_suspended`]). A dropped leader /// can outlive its lease; once a follower adopts the still-`creating` /// generation, either changed fence makes the delayed retirement a no-op. /// diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests.rs index 7478b38fb..c3b5f25e6 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests.rs @@ -13,10 +13,19 @@ use pplx_space_test_env::TestResult; mod cleanup_recovery_lifecycle_matrix_tests; mod lifecycle_writer_tests; mod materialization_record_tests; +mod owned_restore_completion_tests; +mod restore_revert_tests; +mod started_recovery_observation_tests; mod stopped_evacuation_claim_tests; mod stopped_restore_selector_tests; mod substrate_scope_tests; mod suspend_lifecycle_matrix_tests; +use owned_restore_completion_tests::owned_restore_completion_requires_its_lease_and_a_routable_execution; +use restore_revert_tests::{ + failed_restore_revert_retires_only_its_own_proven_execution, + without_retirement_a_failed_restore_revert_moves_only_the_lifecycle, +}; +use started_recovery_observation_tests::started_recovery_followers_succeed_only_on_a_started_sandbox; use stopped_evacuation_claim_tests::{ a_fresh_stopped_claim_is_not_rolled_back_on_a_clean_failure, evacuation_claim_takes_only_a_released_stopped_evacuation, @@ -1433,6 +1442,10 @@ async fn k8s_store_postgres_behaviors() -> TestResult { clean_redrive_failure_releases_restore_lease_for_immediate_retry(&fixture).await?; stale_restore_rollback_is_guarded_by_lease_token(&fixture).await?; restore_rollback_requires_fresh_claim_and_clean_failure(&fixture).await?; + failed_restore_revert_retires_only_its_own_proven_execution(&fixture).await?; + without_retirement_a_failed_restore_revert_moves_only_the_lifecycle(&fixture).await?; + owned_restore_completion_requires_its_lease_and_a_routable_execution(&fixture).await?; + started_recovery_followers_succeed_only_on_a_started_sandbox(&fixture).await?; prepare_wake_preserves_restore_source_boundary(&fixture).await?; prepare_wake_sandbox_restores_suspended_lifecycle(&fixture).await?; restored_generation_accepts_late_pod_metadata(&fixture).await?; @@ -7781,15 +7794,12 @@ async fn begin_restore_lifecycle_preserves_retry_and_marker_claim_rules( .expect_err("started sandbox should not enter restore lifecycle"); assert!(matches!(err, SubstrateError::Conflict { .. })); - // Test fixture setup: a fresh `started` row from `insert_sandbox` - // is never in a terminal lifecycle, so the rollback-best-effort - // helper is functionally equivalent to a plain UPDATE here. We - // reuse it rather than splitting into a separate test-only setter - // — keeping a single lifecycle-write API surface. - fixture - .store - .revert_lifecycle_after_rollback_best_effort(sandbox_id, "suspended", None) - .await?; + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'suspended' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&fixture.pool) + .await?; let claim = fixture .store .begin_restore_lifecycle(sandbox_id, false) @@ -8247,8 +8257,8 @@ async fn clean_redrive_failure_releases_restore_lease_for_immediate_retry( /// A stale clean-failure rollback (delayed past the lease TTL, carrying an /// OLD token) must not flip a row that a retry has already re-claimed with a /// NEW token back to `suspended` — that would orphan the newer live restore. -/// The token-guarded `revert_lifecycle_after_rollback_best_effort` is a CAS -/// on the claim's token; `None` (evacuation / test setup) stays unguarded. +/// The token-guarded `revert_restore_claim_to_suspended` is a CAS on the +/// claim's token; `None` (the tokenless evacuation claim) is unguarded. async fn stale_restore_rollback_is_guarded_by_lease_token(fixture: &PostgresStore) -> TestResult { let id = SandboxId::new(); insert_sandbox(&fixture.pool, id, "restoring").await?; @@ -8269,7 +8279,12 @@ async fn stale_restore_rollback_is_guarded_by_lease_token(fixture: &PostgresStor let old_token = Uuid::now_v7(); fixture .store - .revert_lifecycle_after_rollback_best_effort(id, "suspended", Some(old_token)) + .revert_restore_claim_to_suspended( + id, + Some(old_token), + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) .await?; // A stale-token rollback must not flip the newer retry to `suspended` // nor clear its lease — the redaction asserts the token is exactly the @@ -8292,7 +8307,12 @@ async fn stale_restore_rollback_is_guarded_by_lease_token(fixture: &PostgresStor // the clear-trigger drops the lease as the row leaves `restoring`). fixture .store - .revert_lifecycle_after_rollback_best_effort(id, "suspended", Some(new_token)) + .revert_restore_claim_to_suspended( + id, + Some(new_token), + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) .await?; // A matching-token rollback reverts, and leaving `restoring` clears the // lease (no run-varying value survives here, so a plain snapshot pins @@ -8335,7 +8355,12 @@ async fn restore_rollback_requires_fresh_claim_and_clean_failure( ) { fixture .store - .revert_lifecycle_after_rollback_best_effort(fresh_clean_id, "suspended", None) + .revert_restore_claim_to_suspended( + fresh_clean_id, + None, + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) .await?; } let lifecycle: String = @@ -8415,7 +8440,12 @@ async fn restore_rollback_requires_fresh_claim_and_clean_failure( ) { fixture .store - .revert_lifecycle_after_rollback_best_effort(redrove_clean_id, "suspended", None) + .revert_restore_claim_to_suspended( + redrove_clean_id, + None, + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) .await?; } let lifecycle: String = @@ -8447,7 +8477,12 @@ async fn restore_rollback_requires_fresh_claim_and_clean_failure( ) { fixture .store - .revert_lifecycle_after_rollback_best_effort(fresh_timeout_id, "suspended", None) + .revert_restore_claim_to_suspended( + fresh_timeout_id, + None, + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) .await?; } let lifecycle: String = @@ -8492,7 +8527,12 @@ async fn restore_rollback_requires_fresh_claim_and_clean_failure( ) { fixture .store - .revert_lifecycle_after_rollback_best_effort(redrove_timeout_id, "suspended", None) + .revert_restore_claim_to_suspended( + redrove_timeout_id, + None, + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) .await?; } let lifecycle: String = diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/owned_restore_completion_tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/owned_restore_completion_tests.rs new file mode 100644 index 000000000..b4fd722df --- /dev/null +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/owned_restore_completion_tests.rs @@ -0,0 +1,129 @@ +//! Postgres behaviors for the restore owner's own `restoring -> started` +//! commit (`K8sStore::complete_owned_user_restore`). +//! +//! A sibling module rather than more lines in the parent, which is already far +//! past the repository's file-size guidance. Driven from +//! `k8s_store_postgres_behaviors` against the parent's fixture. + +use pplx_space_common_core::types::SandboxId; + +use super::*; + +/// What stands between the owner's wake and its commit. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Shape { + /// The owner's restore is live behind a running execution with a pod IP. + Routable, + /// A newer claim holds the row under another lease token. + AnotherOwner, + /// A rollback moved the row back to `suspended`. + RolledBack, + /// The execution has no pod IP yet. + Unroutable, +} + +/// The owner of a user restore commits `started` only for its own lease, on +/// a row still `restoring` with a routable execution. Once it has, the API +/// rollback that would abandon the restore finds nothing to revert. +pub(super) async fn owned_restore_completion_requires_its_lease_and_a_routable_execution( + fixture: &PostgresStore, +) -> TestResult { + let mut mismatches = Vec::new(); + for shape in [ + Shape::Routable, + Shape::AnotherOwner, + Shape::RolledBack, + Shape::Unroutable, + ] { + let sandbox_id = SandboxId::new(); + insert_sandbox(&fixture.pool, sandbox_id, "suspended").await?; + let token = user_fresh_restore_token( + fixture + .store + .begin_restore_lifecycle(sandbox_id, false) + .await?, + ); + sqlx::query(sql!( + "INSERT INTO sandbox_k8s_executions + (sandbox_id, generation, pod_name, pod_namespace, pod_uid, state, active, + pod_ip) + VALUES ($1, 1, $2, 'space', 'uid-owned-g1', 'running', TRUE, + CASE WHEN $3 THEN NULL ELSE '10.80.0.7'::inet END)" + )) + .bind(sandbox_id) + .bind(format!("space-sbx-{}-g1", sandbox_id.as_uuid().simple())) + .bind(shape == Shape::Unroutable) + .execute(&fixture.pool) + .await?; + match shape { + Shape::AnotherOwner => { + sqlx::query(sql!( + "UPDATE sandboxes SET restore_lease_token = $2 WHERE id = $1" + )) + .bind(sandbox_id) + .bind(Uuid::now_v7()) + .execute(&fixture.pool) + .await?; + } + Shape::RolledBack => { + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'suspended' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&fixture.pool) + .await?; + } + Shape::Routable | Shape::Unroutable => {} + } + let lifecycle_before: String = + sqlx::query_scalar(sql!("SELECT lifecycle FROM sandboxes WHERE id = $1")) + .bind(sandbox_id) + .fetch_one(&fixture.pool) + .await?; + + let completed = fixture + .store + .complete_owned_user_restore(sandbox_id, token) + .await?; + let reverted = fixture + .store + .revert_restore_claim_to_suspended( + sandbox_id, + Some(token), + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) + .await?; + let (lifecycle, placement): (String, String) = sqlx::query_as(sql!( + "SELECT lifecycle, placement FROM sandboxes WHERE id = $1" + )) + .bind(sandbox_id) + .fetch_one(&fixture.pool) + .await?; + + let expected_completed = shape == Shape::Routable; + if completed != expected_completed { + mismatches.push(format!( + "{shape:?}: completed {completed}, expected {expected_completed}" + )); + } + if expected_completed { + if (lifecycle.as_str(), placement.as_str()) != ("started", "on_node") { + mismatches.push(format!( + "{shape:?}: row is {lifecycle}/{placement} after the commit" + )); + } + if reverted != RestoreClaimRevert::NotReverted { + mismatches.push(format!( + "{shape:?}: a rollback after the commit reported {reverted:?}" + )); + } + } else if lifecycle == "started" { + mismatches.push(format!( + "{shape:?}: the row moved to started from {lifecycle_before}" + )); + } + } + assert!(mismatches.is_empty(), "{}", mismatches.join("\n")); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/restore_revert_tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/restore_revert_tests.rs new file mode 100644 index 000000000..d262ad2a6 --- /dev/null +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/restore_revert_tests.rs @@ -0,0 +1,477 @@ +//! Postgres behaviors for rolling a failed fresh restore claim back to +//! `suspended` (`K8sStore::revert_restore_claim_to_suspended`). +//! +//! A sibling module rather than more lines in the parent, which is already far +//! past the repository's file-size guidance. Driven from +//! `k8s_store_postgres_behaviors` against the parent's fixture. + +use pplx_space_common_core::types::{ + LocalArtifactId, NodeId, SandboxActivationAttemptId, SandboxId, +}; + +use super::*; + +/// Which execution row the sandbox carries when the revert runs. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Execution { + /// The restore claim created it and its pod materialized (UID recorded, + /// placed on a node), then the restore failed before dispatch. + CreatedByClaim, + /// It was already active when the claim reused it, so this restore didn't + /// create it. + PredatesClaim, + /// The claim failed before any execution existed. + Absent, +} + +/// The activation attempt the rollback names as its claim's. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ClaimAttempt { + /// The claim failed before it was assigned one. + Unassigned, + /// The attempt the execution carries. + Assigned, + /// Another attempt: a redrive took the execution over since. + Superseded, +} + +struct Case { + name: &'static str, + marker: bool, + /// A ready, stored pre-restore snapshot (of the marker's artifact on a + /// marker row), which is what proves restore selection stays durable. + durable_source: bool, + execution: Execution, + /// A node converged the row to `started` before the rollback ran. + converged_to_started: bool, + /// Revert with the claim's lease token, or tokenless as an evacuation + /// claim does. + with_token: bool, + attempt: ClaimAttempt, + expected: Expected, +} + +/// What the revert should report; the retired pod's name is per sandbox. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Expected { + ExecutionRetired, + LifecycleOnly, + ExecutionLeftActive, + NotReverted, +} + +#[derive(Debug, PartialEq, Eq, sqlx::FromRow)] +struct RevertedRow { + lifecycle: String, + placement: String, + active_node_id: Option, + target_node_id: Option, + current_local_artifact_id: Option, + execution_active: Option, + execution_state: Option, +} + +async fn reverted_row(pool: &PgPool, sandbox_id: SandboxId) -> TestResult { + Ok(sqlx::query_as(sql!( + "SELECT s.lifecycle, s.placement, s.active_node_id, s.target_node_id, + s.current_local_artifact_id, + e.active AS execution_active, e.state AS execution_state + FROM sandboxes s + LEFT JOIN sandbox_k8s_executions e ON e.sandbox_id = s.id + WHERE s.id = $1" + )) + .bind(sandbox_id) + .fetch_one(pool) + .await?) +} + +const PRE_RESTORE_URI: &str = "s3://bucket/pre-restore?region=us-east-1"; + +fn execution_pod_name(sandbox_id: SandboxId) -> String { + format!("space-sbx-{}-g1", sandbox_id.as_uuid().simple()) +} + +/// `created` offsets the row's `created_at` and `updated_at` from now. An +/// older row needs both: a recently updated active execution would block the +/// restore claim itself. +async fn insert_execution( + pool: &PgPool, + sandbox_id: SandboxId, + created: &str, + activation_attempt_id: SandboxActivationAttemptId, +) -> TestResult { + sqlx::query(sql!( + "INSERT INTO sandbox_k8s_executions + (sandbox_id, generation, pod_name, pod_namespace, pod_uid, state, active, + created_at, updated_at, activation_attempt_id, activation_attempt_expires_at, + activation_attempt_target_state) + VALUES ($1, 1, $3, 'space', 'uid-revert-g1', 'created', TRUE, + NOW() + $2::interval, NOW() + $2::interval, $4, + NOW() + INTERVAL '5 minutes', 'started')" + )) + .bind(sandbox_id) + .bind(created) + .bind(execution_pod_name(sandbox_id)) + .bind(activation_attempt_id) + .execute(pool) + .await?; + Ok(()) +} + +/// A failed fresh restore claim's rollback records the abandonment on the +/// execution the claim created, retiring it as `replaced` with the row's +/// placement, while keeping the wake source (`target_node_id`, the artifact +/// marker). It retires nothing it can't prove disposable: an execution the +/// claim didn't create, or one whose sandbox has no proven durable restore +/// source, stays active; and a row that left `restoring` isn't touched. +pub(super) async fn failed_restore_revert_retires_only_its_own_proven_execution( + fixture: &PostgresStore, +) -> TestResult { + let cases = [ + Case { + name: "claimed execution, durable markerless source", + marker: false, + durable_source: true, + execution: Execution::CreatedByClaim, + converged_to_started: false, + with_token: true, + attempt: ClaimAttempt::Unassigned, + expected: Expected::ExecutionRetired, + }, + Case { + name: "claimed execution, durable marker source", + marker: true, + durable_source: true, + execution: Execution::CreatedByClaim, + converged_to_started: false, + with_token: true, + attempt: ClaimAttempt::Assigned, + expected: Expected::ExecutionRetired, + }, + Case { + name: "claimed execution, tokenless claim with its attempt", + marker: false, + durable_source: true, + execution: Execution::CreatedByClaim, + converged_to_started: false, + with_token: false, + attempt: ClaimAttempt::Assigned, + expected: Expected::ExecutionRetired, + }, + Case { + name: "claimed execution, tokenless claim with no attempt", + marker: false, + durable_source: true, + execution: Execution::CreatedByClaim, + converged_to_started: false, + with_token: false, + attempt: ClaimAttempt::Unassigned, + expected: Expected::ExecutionLeftActive, + }, + Case { + name: "execution taken over by a tokenless redrive", + marker: false, + durable_source: true, + execution: Execution::CreatedByClaim, + converged_to_started: false, + with_token: false, + attempt: ClaimAttempt::Superseded, + expected: Expected::ExecutionLeftActive, + }, + Case { + name: "claimed execution, no durable source", + marker: false, + durable_source: false, + execution: Execution::CreatedByClaim, + converged_to_started: false, + with_token: true, + attempt: ClaimAttempt::Unassigned, + expected: Expected::ExecutionLeftActive, + }, + Case { + name: "execution predates the claim", + marker: false, + durable_source: true, + execution: Execution::PredatesClaim, + converged_to_started: false, + with_token: true, + attempt: ClaimAttempt::Unassigned, + expected: Expected::ExecutionLeftActive, + }, + Case { + name: "no execution, durable source", + marker: false, + durable_source: true, + execution: Execution::Absent, + converged_to_started: false, + with_token: true, + attempt: ClaimAttempt::Unassigned, + expected: Expected::LifecycleOnly, + }, + Case { + name: "row converged to started first", + marker: false, + durable_source: true, + execution: Execution::CreatedByClaim, + converged_to_started: true, + with_token: false, + attempt: ClaimAttempt::Assigned, + expected: Expected::NotReverted, + }, + ]; + + let mut mismatches = Vec::new(); + for case in cases { + let sandbox_id = SandboxId::new(); + let node_id = NodeId::new(); + insert_sandbox(&fixture.pool, sandbox_id, "suspended").await?; + sqlx::query(sql!( + "INSERT INTO nodes (id, address, role, managed_by, state) + VALUES ($1, '10.0.0.11', 'worker', 'k8s', 'ready')" + )) + .bind(node_id) + .execute(&fixture.pool) + .await?; + let execution_attempt = SandboxActivationAttemptId::new(); + let claim_attempt = match case.attempt { + ClaimAttempt::Unassigned => None, + ClaimAttempt::Assigned => Some((execution_attempt, 1)), + ClaimAttempt::Superseded => Some((SandboxActivationAttemptId::new(), 1)), + }; + let marker = case.marker.then(LocalArtifactId::new); + sqlx::query(sql!( + "UPDATE sandboxes + SET target_node_id = $2, + current_local_artifact_id = $3, + current_local_artifact_kind = CASE WHEN $3::uuid IS NULL THEN NULL + ELSE 'full' END, + current_local_artifact_origin = CASE WHEN $3::uuid IS NULL THEN NULL + ELSE 'suspend' END + WHERE id = $1" + )) + .bind(sandbox_id) + .bind(node_id) + .bind(marker) + .execute(&fixture.pool) + .await?; + if case.durable_source { + insert_ready_full_snapshot_relative_to_lifecycle( + &fixture.pool, + sandbox_id, + PRE_RESTORE_URI, + -1, + marker, + ) + .await?; + } + if case.execution == Execution::PredatesClaim { + insert_execution(&fixture.pool, sandbox_id, "-1 hour", execution_attempt).await?; + } + + let token = user_fresh_restore_token( + fixture + .store + .begin_restore_lifecycle(sandbox_id, false) + .await?, + ); + + if case.execution == Execution::CreatedByClaim { + insert_execution(&fixture.pool, sandbox_id, "0 seconds", execution_attempt).await?; + } + // The pod bound, so the row reads as placed on that node, as of now: + // after the pre-restore snapshot was taken. + sqlx::query(sql!( + "UPDATE sandboxes + SET placement = 'on_node', + active_node_id = $2, + placement_changed_at = NOW() + WHERE id = $1" + )) + .bind(sandbox_id) + .bind(node_id) + .execute(&fixture.pool) + .await?; + if case.converged_to_started { + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'started' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&fixture.pool) + .await?; + } + let before = reverted_row(&fixture.pool, sandbox_id).await?; + + let outcome = fixture + .store + .revert_restore_claim_to_suspended( + sandbox_id, + case.with_token.then_some(token), + claim_attempt, + crate::controllers::substrate::AbandonedRestoreRetirement::Enabled, + ) + .await?; + + let (expected_outcome, expected_row) = match case.expected { + Expected::ExecutionRetired => ( + RestoreClaimRevert::ExecutionRetired { + pod_name: execution_pod_name(sandbox_id), + }, + RevertedRow { + lifecycle: "suspended".to_string(), + placement: "unplaced".to_string(), + active_node_id: None, + execution_active: Some(false), + execution_state: Some("replaced".to_string()), + ..before + }, + ), + Expected::LifecycleOnly => ( + RestoreClaimRevert::LifecycleOnly, + RevertedRow { + lifecycle: "suspended".to_string(), + ..before + }, + ), + Expected::ExecutionLeftActive => ( + RestoreClaimRevert::ExecutionLeftActive, + RevertedRow { + lifecycle: "suspended".to_string(), + ..before + }, + ), + Expected::NotReverted => (RestoreClaimRevert::NotReverted, before), + }; + // The expected rows keep `before`'s wake source: `target_node_id` + // and the artifact marker. + let actual = reverted_row(&fixture.pool, sandbox_id).await?; + // A reverted row with a durable source still restores from it, + // although the bind moved its placement past that snapshot. + let selected = if case.expected == Expected::NotReverted { + None + } else { + let sandbox = crate::controllers::sandbox::fetch_sandbox(&fixture.pool, sandbox_id) + .await? + .expect("sandbox row exists"); + fixture + .store + .selected_suspend_restore_storage_uri(&sandbox, false) + .await? + }; + // A reverted, proven row whose active execution is gone has its + // placement clock cleared, so it selects its durable source. One that + // keeps an execution the claim didn't create keeps the clock: nothing + // proves that execution's pod holds no state of its own. + let clock_cleared = case.durable_source + && (case.expected == Expected::ExecutionRetired + || (case.expected == Expected::LifecycleOnly + && case.execution == Execution::Absent)); + let expected_selected = clock_cleared.then_some(PRE_RESTORE_URI); + // ...and is durably restorable at once, which is what lets a fenced + // release of the abandoned pod's runtime go ahead. + let durable = + pplx_space_sandbox_store::durably_restorable(&fixture.pool, sandbox_id).await?; + if outcome != expected_outcome + || actual != expected_row + || selected.as_deref() != expected_selected + || durable != clock_cleared + { + mismatches.push(format!( + "{}: {outcome:?}, {actual:?}, selected {selected:?}, durable {durable}", + case.name + )); + } + } + assert!(mismatches.is_empty(), "{mismatches:#?}"); + Ok(()) +} + +/// With abandoned-restore retirement disabled (the default until every +/// routing replica refuses to route to a `restoring` sandbox), a failed +/// fresh restore claim's rollback moves only the lifecycle, even with a +/// proven source: the execution the claim created stays active and the row +/// stays placed with its placement clock, so its pod isn't deleted under a +/// request an older replica may have routed to it. +pub(super) async fn without_retirement_a_failed_restore_revert_moves_only_the_lifecycle( + fixture: &PostgresStore, +) -> TestResult { + let sandbox_id = SandboxId::new(); + let node_id = NodeId::new(); + insert_sandbox(&fixture.pool, sandbox_id, "suspended").await?; + sqlx::query(sql!( + "INSERT INTO nodes (id, address, role, managed_by, state) + VALUES ($1, '10.0.0.12', 'worker', 'k8s', 'ready')" + )) + .bind(node_id) + .execute(&fixture.pool) + .await?; + sqlx::query(sql!( + "UPDATE sandboxes SET target_node_id = $2 WHERE id = $1" + )) + .bind(sandbox_id) + .bind(node_id) + .execute(&fixture.pool) + .await?; + insert_ready_full_snapshot_relative_to_lifecycle( + &fixture.pool, + sandbox_id, + PRE_RESTORE_URI, + -1, + None, + ) + .await?; + let token = user_fresh_restore_token( + fixture + .store + .begin_restore_lifecycle(sandbox_id, false) + .await?, + ); + insert_execution( + &fixture.pool, + sandbox_id, + "0 seconds", + SandboxActivationAttemptId::new(), + ) + .await?; + sqlx::query(sql!( + "UPDATE sandboxes + SET placement = 'on_node', + active_node_id = $2, + placement_changed_at = NOW() + WHERE id = $1" + )) + .bind(sandbox_id) + .bind(node_id) + .execute(&fixture.pool) + .await?; + let before = reverted_row(&fixture.pool, sandbox_id).await?; + let clock = || async { + sqlx::query_scalar::<_, Option>(sql!( + "SELECT placement_changed_at FROM sandboxes WHERE id = $1" + )) + .bind(sandbox_id) + .fetch_one(&fixture.pool) + .await + }; + let clock_before = clock().await?; + + let outcome = fixture + .store + .revert_restore_claim_to_suspended( + sandbox_id, + Some(token), + None, + crate::controllers::substrate::AbandonedRestoreRetirement::Disabled, + ) + .await?; + + assert_eq!(outcome, RestoreClaimRevert::ExecutionLeftActive); + assert_eq!( + reverted_row(&fixture.pool, sandbox_id).await?, + RevertedRow { + lifecycle: "suspended".to_string(), + ..before + } + ); + assert_eq!(clock().await?, clock_before); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/started_recovery_observation_tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/started_recovery_observation_tests.rs new file mode 100644 index 000000000..d9ec80daf --- /dev/null +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/started_recovery_observation_tests.rs @@ -0,0 +1,53 @@ +//! Postgres behaviors for a started-recovery follower's observation +//! (`K8sStore::observe_started_recovery`). +//! +//! A sibling module rather than more lines in the parent, which is already far +//! past the repository's file-size guidance. Driven from +//! `k8s_store_postgres_behaviors` against the parent's fixture. + +use pplx_space_common_core::types::SandboxId; + +use super::*; + +/// A follower of started recovery at generation 1 reports success only for a +/// running execution on a `started` sandbox. If a suspend and a restore landed +/// in between, the generation it finds running belongs to that restore, and +/// while the sandbox is still `restoring` the follower keeps waiting: handing +/// it out would route a user request to a restore that can still be rolled +/// back. +pub(super) async fn started_recovery_followers_succeed_only_on_a_started_sandbox( + fixture: &PostgresStore, +) -> TestResult { + let mut mismatches = Vec::new(); + for (lifecycle, expected_success, expected_pending) in [ + ("started", true, false), + ("restoring", false, true), + ("suspended", false, false), + ] { + let sandbox_id = SandboxId::new(); + insert_sandbox(&fixture.pool, sandbox_id, lifecycle).await?; + // The generation a later restore created, already running. + sqlx::query(sql!( + "INSERT INTO sandbox_k8s_executions + (sandbox_id, generation, pod_name, pod_namespace, pod_uid, state, active, pod_ip) + VALUES ($1, 2, $2, 'space', 'uid-observed-g2', 'running', TRUE, '10.80.0.9')" + )) + .bind(sandbox_id) + .bind(format!("space-sbx-{}-g2", sandbox_id.as_uuid().simple())) + .execute(&fixture.pool) + .await?; + + let observation = fixture + .store + .observe_started_recovery(sandbox_id, 1) + .await?; + + let succeeded = matches!(observation, StartedRecoveryObservation::Succeeded { .. }); + let pending = matches!(observation, StartedRecoveryObservation::Pending); + if (succeeded, pending) != (expected_success, expected_pending) { + mismatches.push(format!("{lifecycle}: observed {observation:?}")); + } + } + assert!(mismatches.is_empty(), "{}", mismatches.join("\n")); + Ok(()) +} diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/stopped_evacuation_claim_tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/stopped_evacuation_claim_tests.rs index ca5241733..bcaf79798 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/stopped_evacuation_claim_tests.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/k8s_store_postgres_tests/stopped_evacuation_claim_tests.rs @@ -244,10 +244,12 @@ pub(super) async fn a_fresh_stopped_claim_is_not_rolled_back_on_a_clean_failure( is no lease for the failure path to release" ); - fixture - .store - .revert_lifecycle_after_rollback_best_effort(sandbox_id, "stopped", None) - .await?; + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'stopped' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; assert_eq!( claim_row(pool, sandbox_id).await?, StoppedClaimRow { diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs index b730258cd..378101433 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/restore_lease.rs @@ -210,57 +210,206 @@ impl K8sStore { .collect() } - /// Best-effort lifecycle revert for the restore/wake rollback paths in - /// [`super::super::execution`]. Reverts the row to `lifecycle` (in practice - /// `"suspended"`) when a Kubernetes restore or wake fails mid-flight. - /// CAS-guarded against `('failed', 'quarantined', 'deleted', 'cleanup_required')` so - /// a rollback can't overwrite a terminal lifecycle the - /// transient-lifecycle sweeper just claimed (the sweeper decided the - /// operation was wedged; the rollback's "revert to my last known - /// state" intent is wrong once the row has been retired). + /// Revert a failed fresh restore claim from `restoring` to `suspended`. /// - /// **Returns Ok regardless of whether the write applied.** The - /// rollback is best-effort by design — `failed` is the correct - /// resting state for a swept row, and callers don't act on the - /// no-write case. The function name encodes this contract; do - /// **not** call this from paths that depend on "the row is now in - /// `lifecycle` after this returns." Use a dedicated CAS-guarded - /// UPDATE inline if you need to detect race-loss (see the - /// post-restore / post-wake CAS in - /// `crates/libs/api/src/controllers/sandbox/lifecycle.rs` for the - /// pattern). - /// Best-effort revert of a sandbox's lifecycle (used by the restore - /// rollback path to put a failed restore back to `suspended`). + /// The revert is the one writer that knows the restore was abandoned, so + /// it records that on the execution the claim created: in the same + /// transaction, that execution is retired as `replaced` and the row is + /// unplaced. A pod the claim materialized then belongs to no active + /// execution, so no later restore adopts it (a new claim allocates a newer + /// generation, whose lingering-pod sweep deletes this one), and a backup + /// its runtime captures registers against a retired generation. Left + /// active instead, the execution keeps a same-generation pod adoptable on + /// a `suspended` row that no reconciler revisits, and that row looks like + /// one whose pod holds the only copy of the sandbox's state. /// - /// `restore_lease_token` token-guards the revert: when `Some`, the UPDATE - /// only matches if the row still holds that exact lease token. This closes - /// a stale-rollback race the restore lease introduced — a fresh attempt's - /// clean-failure rollback could be delayed past the lease TTL, by which - /// point a retry has claimed a *new* token and is actively restoring; - /// without the guard the stale rollback would flip the newer restore back - /// to `suspended` (and the clear-trigger would drop the newer lease), - /// orphaning live work. `None` skips the guard (evacuation reverts, which - /// are marker-gated and tokenless, and test-only lifecycle setup). - pub(in crate::controllers::substrate::kubernetes) async fn revert_lifecycle_after_rollback_best_effort( + /// The retirement needs the suspended-revert proof: restore selection on + /// the reverted row must still choose a ready, durably stored pre-restore + /// source, the state the sandbox had when it was suspended. The retired + /// runtime served no user request: the access, runtime-client, ingress + /// and data-plane route paths never dial a `restoring` sandbox's pod, but + /// join its restore or fail retryably, once every routing replica runs + /// them; until then retirement stays disabled. Without the proof, or when the + /// active execution can't be shown + /// to be this claim's (one that predates the claim was reused, not + /// created by it), only the lifecycle reverts and the execution is left as + /// it was. + /// + /// `restore_lease_token` binds the revert to the claim that owns the row: + /// a rollback delayed past the lease TTL must not flip a retry that holds + /// a newer token back to `suspended` (the lease-clear trigger would also + /// drop that retry's lease). `None` is the tokenless evacuation claim. + /// Either way the row must still be `restoring`; a row another writer + /// moved on, including one a node converged to `started`, is left alone. + /// + /// `activation_attempt` is the attempt and generation this claim was + /// assigned, if it got that far. The execution is retired only if it + /// still carries them, because a redrive of the same `restoring` row + /// reuses the execution under a new attempt. A claim with no attempt is + /// identified by its lease token alone, so a tokenless one retires + /// nothing: a delayed evacuation rollback can't tell its execution from a + /// later evacuation redrive's. + /// + /// With `retirement` disabled, only the lifecycle reverts, whatever the + /// proof says: the execution, placement and placement clock stay as the + /// restore left them (see [`AbandonedRestoreRetirement`]). + pub(in crate::controllers::substrate::kubernetes) async fn revert_restore_claim_to_suspended( &self, sandbox_id: SandboxId, - lifecycle: &str, restore_lease_token: Option, - ) -> Result<(), SubstrateError> { - sqlx::query(sql!( - "UPDATE sandboxes - SET lifecycle = $1 - WHERE id = $2 - AND lifecycle NOT IN ('failed', 'quarantined', 'deleted', 'cleanup_required') - AND ($3::uuid IS NULL OR restore_lease_token = $3)" + activation_attempt: Option<(SandboxActivationAttemptId, i64)>, + retirement: crate::controllers::substrate::AbandonedRestoreRetirement, + ) -> Result { + let mut tx = self + .pool + .begin() + .await + .db_context("failed to start restore revert transaction")?; + // Lock first, in its own statement: a later statement in this + // transaction then reads the snapshots and executions as of after the + // lock, so a writer that committed while this waited is seen. + let locked: Option = sqlx::query_scalar(sql!( + "SELECT TRUE + FROM sandboxes + WHERE id = $1 + AND lifecycle = 'restoring' + AND ($2::uuid IS NULL OR restore_lease_token = $2) + FOR UPDATE" )) - .bind(lifecycle) .bind(sandbox_id) .bind(restore_lease_token) - .execute(&self.pool) + .fetch_optional(&mut *tx) .await - .db_context_with(|| format!("failed to revert sandbox lifecycle to {lifecycle}"))?; - Ok(()) + .db_context("failed to lock sandbox for restore revert")?; + if locked.is_none() { + tx.commit() + .await + .db_context("failed to commit restore revert transaction")?; + return Ok(RestoreClaimRevert::NotReverted); + } + + let proof = restore_retry_suspended_revert_snapshot_exists("s"); + // Under the row lock no other claim can replace the execution, and + // `lifecycle_changed_at` still reads this claim's `restoring` + // transition, so an active execution created since, carrying this + // claim's attempt (or, with no attempt, under this claim's lease + // token), is the one it created. + let query = format!( + "SELECT claimed.generation, + claimed.pod_name, + EXISTS ( + SELECT 1 + FROM sandbox_k8s_executions e + WHERE e.sandbox_id = s.id + AND e.active + ), + {proof} + FROM sandboxes s + LEFT JOIN LATERAL ( + SELECT e.generation, e.pod_name + FROM sandbox_k8s_executions e + WHERE e.sandbox_id = s.id + AND e.active + AND e.created_at >= s.lifecycle_changed_at + AND CASE + WHEN $3::uuid IS NOT NULL + THEN e.activation_attempt_id = $3 + AND e.generation = $4 + ELSE $2::uuid IS NOT NULL + END + ) claimed ON TRUE + WHERE s.id = $1" + ); + let (claimed_generation, claimed_pod, any_active, source_proven): ( + Option, + Option, + bool, + bool, + ) = sqlx::query_as(sqlx::AssertSqlSafe(sql!(query.as_str()))) + .bind(sandbox_id) + .bind(restore_lease_token) + .bind(activation_attempt.map(|(attempt, _)| attempt)) + .bind(activation_attempt.map(|(_, generation)| generation)) + .fetch_one(&mut *tx) + .await + .db_context("failed to read restore revert evidence")?; + + let retire = source_proven && retirement.is_enabled(); + let outcome = match (claimed_generation.zip(claimed_pod), retire) { + (Some((generation, pod_name)), true) => { + if !self + .finish_active_execution_generation_in( + &mut tx, sandbox_id, generation, "replaced", + ) + .await? + { + // Not expected under the row lock; fail closed rather + // than revert a row whose execution this claim can't + // retire. + tx.rollback() + .await + .db_context("failed to roll back restore revert transaction")?; + return Ok(RestoreClaimRevert::NotReverted); + } + // `target_node_id` and the artifact marker name the wake + // source and stay; the lifecycle triggers keep the marker on + // `suspended`. `placement_changed_at` is cleared, as the + // markerless restoring recovery in `pod_tracking.rs` does for + // its rows: a + // restore whose pod bound moved it past the proven + // pre-restore source, and both the suspended selector and + // `durably_restorable` take a markerless snapshot only when + // it is newer than the last placement. + sqlx::query(sql!( + "UPDATE sandboxes + SET lifecycle = 'suspended', + placement = 'unplaced', + active_node_id = NULL, + placement_changed_at = NULL + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&mut *tx) + .await + .db_context("failed to revert sandbox lifecycle to suspended")?; + RestoreClaimRevert::ExecutionRetired { pod_name } + } + (None, true) if !any_active => { + // Proven, and no execution at all: the same placement-clock + // clear as above. An execution the claim reused rather than + // created keeps the clock, since nothing proves its pod + // holds no state of its own. + sqlx::query(sql!( + "UPDATE sandboxes + SET lifecycle = 'suspended', + placement_changed_at = NULL + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&mut *tx) + .await + .db_context("failed to revert sandbox lifecycle to suspended")?; + RestoreClaimRevert::LifecycleOnly + } + (claimed, _) => { + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'suspended' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&mut *tx) + .await + .db_context("failed to revert sandbox lifecycle to suspended")?; + if claimed.is_some() || any_active { + RestoreClaimRevert::ExecutionLeftActive + } else { + RestoreClaimRevert::LifecycleOnly + } + } + }; + tx.commit() + .await + .db_context("failed to commit restore revert transaction")?; + Ok(outcome) } pub(in crate::controllers::substrate::kubernetes) async fn begin_restore_lifecycle( @@ -490,7 +639,7 @@ impl K8sStore { } } - async fn claim_markerless_restoring_restore_lease( + pub(in crate::controllers::substrate::kubernetes) async fn claim_markerless_restoring_restore_lease( &self, sandbox_id: SandboxId, ) -> Result, SubstrateError> { @@ -571,6 +720,53 @@ impl K8sStore { }) } + /// Commit `restoring -> started` for the restore this caller owns, once + /// its wake has succeeded. Returns whether the row moved. + /// + /// This is the cancelled-wake redrive's fenced completion CAS + /// (`wake_completion.rs`): the row must still be `restoring` with a + /// routable active execution behind it. Where the redrive waits for the + /// lease to expire, so that no live attempt is overridden, this requires + /// the lease to be the caller's own: the owner is that live attempt. A + /// row a rollback, the sweeper, a suspend or a delete has moved on matches + /// nothing (leaving `restoring` also clears the token, through the + /// `sandboxes_clear_restore_lease` trigger), and once it is `started` + /// neither the API rollback nor the sweeper's revert (both + /// `restoring`-guarded) can move it back. + /// + /// Without this, a Kubernetes restore driven from an access path, which + /// runs no lifecycle-controller completion, stays `restoring` until the + /// node's published state converges it, and a user request may not dial + /// it until then. + pub(in crate::controllers::substrate::kubernetes) async fn complete_owned_user_restore( + &self, + sandbox_id: SandboxId, + restore_lease_token: Uuid, + ) -> Result { + let routable = + pplx_space_common_svc::sql::sandbox_routability::routable_execution_or_non_k8s( + "sandboxes", + "$3", + ); + let query = format!( + "UPDATE sandboxes + SET lifecycle = 'started', placement = 'on_node' + WHERE id = $1 + AND lifecycle = 'restoring' + AND restore_lease_token = $2 + AND {routable}" + ); + let completed = sqlx::query(sqlx::AssertSqlSafe(sql!(query.as_str()))) + .bind(sandbox_id.as_uuid()) + .bind(restore_lease_token) + .bind(pplx_space_common_core::types::SubstrateKind::Kubernetes) + .execute(&self.pool) + .await + .db_context("failed to complete owned sandbox restore")? + .rows_affected(); + Ok(completed == 1) + } + pub(in crate::controllers::substrate::kubernetes) async fn release_user_restore_lease( &self, sandbox_id: SandboxId, diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/store/started_recovery.rs b/crates/libs/api/src/controllers/substrate/kubernetes/store/started_recovery.rs index 630b24d99..7f0afbafc 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/store/started_recovery.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/store/started_recovery.rs @@ -556,11 +556,17 @@ impl K8sStore { sandbox_id: SandboxId, generation: i64, ) -> Result { - let row = sqlx::query_as::<_, (i64, String, String, Option)>(sql!( - "SELECT generation, state, pod_name, host(pod_ip) - FROM sandbox_k8s_executions - WHERE sandbox_id = $1 AND generation >= $2 AND active - AND state IN ($3, $4, $5)" + // The sandbox's lifecycle decides what a running execution means: a + // suspend and a restore can land while a follower waits, and the + // generation it then finds running is that restore's. Until the row + // is `started` that restore can still be rolled back, so it is no + // recovery to hand out (see `join_restore_before_dial`). + let row = sqlx::query_as::<_, (i64, String, String, Option, String)>(sql!( + "SELECT e.generation, e.state, e.pod_name, host(e.pod_ip), s.lifecycle + FROM sandbox_k8s_executions e + JOIN sandboxes s ON s.id = e.sandbox_id + WHERE e.sandbox_id = $1 AND e.generation >= $2 AND e.active + AND e.state IN ($3, $4, $5)" )) .bind(sandbox_id.as_uuid()) .bind(generation) @@ -571,7 +577,7 @@ impl K8sStore { .await .db_context("failed to observe started sandbox recovery")?; - let Some((generation, state, pod_name, pod_ip)) = row else { + let Some((generation, state, pod_name, pod_ip, lifecycle)) = row else { return Ok(StartedRecoveryObservation::Failed); }; let state = @@ -581,14 +587,25 @@ impl K8sStore { message: format!("failed to parse started recovery execution state: {err}"), })?; + let lifecycle = + lifecycle + .parse::() + .map_err(|err| SubstrateError::Failed { + message: format!("failed to parse started recovery sandbox lifecycle: {err}"), + })?; Ok(match (state, pod_ip) { - (ActiveExecutionState::Running, Some(pod_ip)) => { + (ActiveExecutionState::Running, Some(pod_ip)) + if lifecycle == SandboxLifecycle::Started => + { StartedRecoveryObservation::Succeeded { generation, pod_name, pod_ip, } } + (ActiveExecutionState::Running, _) if lifecycle == SandboxLifecycle::Restoring => { + StartedRecoveryObservation::Pending + } (ActiveExecutionState::Creating | ActiveExecutionState::Recovering, _) => { StartedRecoveryObservation::Pending } diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/test_substrate.rs b/crates/libs/api/src/controllers/substrate/kubernetes/test_substrate.rs index d9a56f775..55810626c 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/test_substrate.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/test_substrate.rs @@ -11,6 +11,7 @@ use kube::runtime::{reflector, watcher::Event}; use super::execution::RecoveryJoinBackoffPolicy; use super::*; +use crate::controllers::substrate::AbandonedRestoreRetirement; /// The namespace a substrate built here keeps its sandbox pods and their /// `sandbox_k8s_executions` rows in. @@ -26,8 +27,9 @@ impl KubernetesSubstrate { /// It dials a pod's sandbox proxy on `sandbox_proxy_port`, and a node's /// node-api on the port its node-api pod advertises, or on /// `sandbox_proxy_port` when that pod advertises none. Its create, - /// data-plane, delete and cancel-rate budgets are the ones - /// [`Self::try_from_env`] computes with none of their variables set. The + /// data-plane, delete and cancel-rate budgets, and its abandoned-restore + /// retirement switch (off), are the ones [`Self::try_from_env`] computes + /// with none of their variables set. The /// node-api client still reads its transport and bearer tokens from the /// process environment, as in production. pub fn with_seeded_caches( @@ -40,6 +42,7 @@ impl KubernetesSubstrate { substrate.k8s.create_command_ready_timeout = parse_create_command_ready_timeout(None); substrate.k8s.create_data_plane_ready_timeout = parse_create_data_plane_ready_timeout(None); substrate.k8s.k8s_blocking_delete_timeout = DEFAULT_K8S_BLOCKING_DELETE_TIMEOUT; + substrate.k8s.abandoned_restore_retirement = AbandonedRestoreRetirement::Disabled; substrate.cancel_tracker = CancelRateTracker::with_default_budget(); substrate } @@ -81,8 +84,9 @@ pub(super) fn seeded_substrate( } /// A substrate over `pool` and `client` with the given caches, one-second -/// create and delete budgets, a cancel-rate budget of 64 a second, and the -/// node-group placement gate closed. +/// create and delete budgets, a cancel-rate budget of 64 a second, +/// abandoned-restore retirement enabled, and the node-group placement gate +/// closed. pub(super) fn assemble_substrate( pool: PgPool, client: Client, @@ -103,6 +107,7 @@ pub(super) fn assemble_substrate( create_command_ready_timeout: Duration::from_secs(1), create_data_plane_ready_timeout: Duration::from_secs(1), k8s_blocking_delete_timeout: Duration::from_secs(1), + abandoned_restore_retirement: AbandonedRestoreRetirement::Enabled, caches, }, restore_waiters: RestoreWaiters::new( diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/tests.rs b/crates/libs/api/src/controllers/substrate/kubernetes/tests.rs index 2755ede56..fdf9c801d 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/tests.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/tests.rs @@ -5441,6 +5441,172 @@ mod kubernetes_tests { Ok(()) } + /// The owner of a user restore commits `restoring -> started` itself + /// before it returns, so the request it serves never dials a `restoring` + /// pod and no node publication has to arrive first. + #[tokio::test(flavor = "current_thread")] + async fn an_owned_restore_commits_started_before_it_returns() -> Result<(), BoxError> { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let pool = db.pool.clone(); + let sandbox_id = SandboxId::new(); + seed_restoring_sandbox_with_active_lease(&pool, sandbox_id, Duration::from_millis(200)) + .await?; + let sandbox = fetch_sandbox(&pool, sandbox_id) + .await? + .expect("inserted sandbox must be visible"); + let sim_worker = start_sim_worker().await?; + let substrate = substrate_with_store_client_and_proxy_port( + pool.clone(), + running_pod_kubernetes_client(Arc::new(Mutex::new(Vec::new())), "127.0.0.1"), + sim_worker.port(), + ); + + let binding = substrate + .wake_execution_with_metric_identity( + &sandbox, + Duration::from_secs(2), + SandboxApiMetricIdentity::ecw("connect"), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await?; + + assert_eq!(binding.completion, ActivationCompletion::Owned); + let lifecycle: String = + sqlx::query_scalar(sql!("SELECT lifecycle FROM sandboxes WHERE id = $1")) + .bind(sandbox_id) + .fetch_one(&pool) + .await?; + assert_eq!(lifecycle, "started"); + Ok(()) + } + + /// A user request on a `restoring` row whose owner lapsed reclaims the + /// restore under its own claim, drives it to `started` and only then + /// gets a client: it never dials the restoring pod. + #[tokio::test(flavor = "current_thread")] + async fn a_request_reclaims_a_lapsed_restore_and_dials_it_once_started() -> Result<(), BoxError> + { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let pool = db.pool.clone(); + let sandbox_id = SandboxId::new(); + seed_restoring_sandbox_with_active_lease(&pool, sandbox_id, Duration::from_millis(1)) + .await?; + tokio::time::sleep(Duration::from_millis(50)).await; + let captured_pod_creates = Arc::new(Mutex::new(Vec::new())); + let sim_worker = start_sim_worker().await?; + let substrate = substrate_with_store_client_and_proxy_port( + pool.clone(), + running_pod_kubernetes_client(Arc::clone(&captured_pod_creates), "127.0.0.1"), + sim_worker.port(), + ); + + let client = substrate + .join_restore_before_dial( + sandbox_id, + SandboxApiMetricIdentity::native("restoring_access"), + Duration::from_secs(5), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + + assert!(client.is_ok(), "{:?}", client.err()); + let lifecycle: String = + sqlx::query_scalar(sql!("SELECT lifecycle FROM sandboxes WHERE id = $1")) + .bind(sandbox_id) + .fetch_one(&pool) + .await?; + assert_eq!(lifecycle, "started"); + assert_eq!( + captured_pod_creates + .lock() + .expect("capture mutex poisoned") + .len(), + 1, + "the reclaim drives one restore" + ); + Ok(()) + } + + /// A started recovery a suspend superseded redrives the sandbox's + /// restore. If the restore's owner can't commit `started` (here a + /// synthetic database failure on that write), the row stays `restoring` + /// and the access request gets no binding for the restored pod: a later + /// revert may still retire it. + #[tokio::test(flavor = "current_thread")] + async fn a_redriven_restore_hands_out_no_binding_until_it_is_started() -> Result<(), BoxError> { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let db = pplx_space_db_test_fixtures::pg_test_db().await?; + let pool = db.pool.clone(); + let sandbox_id = SandboxId::new(); + seed_restoring_sandbox_with_active_lease(&pool, sandbox_id, Duration::from_millis(1)) + .await?; + sqlx::query(sql!( + "UPDATE sandboxes SET lifecycle = 'suspended' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&pool) + .await?; + sqlx::query(sql!( + "CREATE FUNCTION test_refuse_restored_started() RETURNS trigger AS $$ + BEGIN + IF OLD.lifecycle = 'restoring' AND NEW.lifecycle = 'started' THEN + RAISE EXCEPTION 'synthetic started-commit failure'; + END IF; + RETURN NEW; + END + $$ LANGUAGE plpgsql" + )) + .execute(&pool) + .await?; + sqlx::query(sql!( + "CREATE TRIGGER test_refuse_restored_started BEFORE UPDATE ON sandboxes + FOR EACH ROW EXECUTE FUNCTION test_refuse_restored_started()" + )) + .execute(&pool) + .await?; + let sandbox = fetch_sandbox(&pool, sandbox_id) + .await? + .expect("inserted sandbox must be visible"); + let sim_worker = start_sim_worker().await?; + let substrate = substrate_with_store_client_and_proxy_port( + pool.clone(), + running_pod_kubernetes_client(Arc::new(Mutex::new(Vec::new())), "127.0.0.1"), + sim_worker.port(), + ); + + let binding = substrate + .redrive_suspended_recovery_for_access( + &sandbox, + Some(1), + tokio::time::Instant::now() + Duration::from_secs(3), + SandboxApiMetricIdentity::native("restoring_access"), + crate::controllers::substrate::RecoveryCharge::Exempt, + ) + .await; + + let error = binding.expect_err("a restoring execution must not be handed out"); + assert_eq!( + error.retry_after_cause(), + Some(crate::controllers::ControllerRetryAfterCause::RestoreInFlight), + "{error:?}" + ); + let lifecycle: String = + sqlx::query_scalar(sql!("SELECT lifecycle FROM sandboxes WHERE id = $1")) + .bind(sandbox_id) + .fetch_one(&pool) + .await?; + assert_eq!(lifecycle, "restoring"); + Ok(()) + } + #[tokio::test(flavor = "current_thread")] async fn stale_restore_owner_cannot_materialize_after_lease_reclaim() -> Result<(), BoxError> { // Arrange. diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/tests/native_pod_failover.rs b/crates/libs/api/src/controllers/substrate/kubernetes/tests/native_pod_failover.rs index 320eac2b7..69a299281 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/tests/native_pod_failover.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/tests/native_pod_failover.rs @@ -52,6 +52,7 @@ fn stale_instance() -> ResolvedPodInstance { ip: PodIp::parse("127.0.0.2").unwrap(), pod_uid: Some(PodUid::parse("space-sbx-old-uid").unwrap()), generation: 1, + sandbox_restoring: false, } } @@ -83,10 +84,10 @@ struct MovedPod { _db: pplx_space_db_test_fixtures::TestDb, } -async fn moved_pod() -> TestResult { +async fn moved_pod(lifecycle: &str) -> TestResult { let db = pplx_space_db_test_fixtures::pg_test_db().await?; let sandbox_id = SandboxId::new(); - insert_test_sandbox_row(&db.pool, sandbox_id, "started").await?; + insert_test_sandbox_row(&db.pool, sandbox_id, lifecycle).await?; let pod_name = "space-sbx-new"; insert_k8s_execution( &db.pool, @@ -131,7 +132,7 @@ async fn a_native_request_follows_a_moved_pod_after_a_one_second_dial() -> TestR if !crate::test_support::enter_sim_worker_process() { return Ok(()); } - let moved = moved_pod().await?; + let moved = moved_pod("started").await?; let started = Instant::now(); let client = native_access::scope( @@ -161,6 +162,30 @@ async fn a_native_request_follows_a_moved_pod_after_a_one_second_dial() -> TestR Ok(()) } +/// A replacement whose sandbox is `restoring` serves no user request: the +/// first dial's error goes back to the caller, whose recovery joins the +/// restore. +#[tokio::test(flavor = "current_thread")] +async fn a_native_request_does_not_follow_a_moved_pod_into_a_restore() -> TestResult { + if !crate::test_support::enter_sim_worker_process() { + return Ok(()); + } + let moved = moved_pod("restoring").await?; + + let result = native_access::scope( + moved + .substrate + .access_kept_worker_client(moved.sandbox_id, &stale_instance()), + ) + .await; + moved.shutdown.cancel(); + + let error = result.expect_err("the restoring replacement is not dialed"); + assert_eq!(error.status(), axum::http::StatusCode::SERVICE_UNAVAILABLE); + assert_eq!(moved.substrate.pod_worker_channels.len(), 0); + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn a_native_request_to_a_removed_pod_returns_after_its_first_dial() -> TestResult { if !crate::test_support::enter_sim_worker_process() { @@ -206,7 +231,7 @@ async fn any_other_caller_waits_out_the_full_budget_on_the_old_address() -> Test if !crate::test_support::enter_sim_worker_process() { return Ok(()); } - let moved = moved_pod().await?; + let moved = moved_pod("started").await?; let started = Instant::now(); let result = moved diff --git a/crates/libs/api/src/controllers/substrate/kubernetes/wait_ready.rs b/crates/libs/api/src/controllers/substrate/kubernetes/wait_ready.rs index 7db99edee..b0e744f84 100644 --- a/crates/libs/api/src/controllers/substrate/kubernetes/wait_ready.rs +++ b/crates/libs/api/src/controllers/substrate/kubernetes/wait_ready.rs @@ -94,6 +94,11 @@ impl KubernetesSubstrate { } Err(_) => return Err(first_error), }; + if current.sandbox_restoring { + // A replacement mid-restore isn't dialed for a user request (see + // `join_restore_before_dial`); the caller's recovery joins it. + return Err(first_error); + } if !current.same_instance(pod) { pplx_space_metrics::grpc::client_channel_cache("worker-node-api", "pod_moved"); } diff --git a/crates/libs/api/src/controllers/substrate/mod.rs b/crates/libs/api/src/controllers/substrate/mod.rs index ed10cb989..a1ff3a4ee 100644 --- a/crates/libs/api/src/controllers/substrate/mod.rs +++ b/crates/libs/api/src/controllers/substrate/mod.rs @@ -208,6 +208,52 @@ impl ActivationCompletion { } } +/// Whether a restore rollback retires the abandoned restore's execution, and +/// so makes its pod disposable: the API's rollback of a failed restore claim +/// and the transient-lifecycle sweeper's revert of an aged `restoring` row. +/// +/// Retiring is safe only once no user request can have reached a `restoring` +/// sandbox's pod, which every routing replica (API and internal API) running +/// the restoring gate guarantees. A replica from before that gate still +/// routes to a reachable restore pod, and a retirement would then delete +/// what it served. So retirement ships disabled, behind +/// [`Self::ENV`], and is enabled per deployment once every routing replica +/// runs the gate; rolling back to older routing binaries disables it first. +/// Disabled, both writers revert only the lifecycle, as they did before. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum AbandonedRestoreRetirement { + /// Revert only the lifecycle; the restore's execution stays active. + #[default] + Disabled, + /// Retire the abandoned restore's execution in the revert. + Enabled, +} + +impl AbandonedRestoreRetirement { + /// The switch both the API and the cluster controller read + /// ([`Self::from_env`]): `1`, `true`, `yes`, `on` or `enabled`, in any + /// case, turns retirement on; unset or anything else keeps it disabled. + pub const ENV: &'static str = "SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED"; + + pub fn from_enabled(enabled: bool) -> Self { + if enabled { + Self::Enabled + } else { + Self::Disabled + } + } + + /// The switch as both components read it, so they agree on what turns + /// it on. + pub fn from_env() -> Self { + Self::from_enabled(pplx_space_common_core::env::env_flag_var(Self::ENV, false)) + } + + pub fn is_enabled(self) -> bool { + self == Self::Enabled + } +} + /// Exact operation lease to release after the controller publishes its lifecycle. /// A later owner or execution must never be released by this completion. #[derive(Debug, Clone, Copy)] @@ -1057,6 +1103,13 @@ pub struct SubstrateCapabilities { /// row at completion as a newer, finalized suspend and fence it, and must /// shield that CAS against request cancellation. /// + /// On Kubernetes the owner of a user restore, the wake holding its lease + /// token, also commits `started` itself before it returns, so a user + /// request never dials a `restoring` pod and the access path, which runs + /// no completion CAS, doesn't wait on the node's published state. The + /// caller's CAS then finds the row `started` and completes as before; it + /// still covers every wake whose owner commit didn't land. + /// /// `false` does not mean the substrate's rows are never `restoring`. /// Direct's worker-local wake leaves its pre-wake lifecycle (`suspended`, /// or `restoring` kept by an earlier lease restore) for the completion CAS @@ -1095,6 +1148,15 @@ pub struct SubstrateCapabilities { /// reconcilers unconditionally. The lease client comes from the process /// environment, not from the substrate. pub contends_for_leader_lease: bool, + /// A successful [`SubstrateAdapter::suspend_execution`] has already + /// committed `lifecycle = 'suspended'`, under the fence of the execution + /// it suspended. The caller then writes no lifecycle of its own: an + /// unfenced trailing `suspended` write would land after a wake that + /// claimed the row in between and put a live, routed sandbox back to + /// `suspended` with its execution still active. + /// + /// Direct's suspend leaves the lifecycle to that caller write. + pub suspend_commits_lifecycle: bool, } impl SubstrateCapabilities { @@ -1106,6 +1168,7 @@ impl SubstrateCapabilities { supports_node_pool_placement: false, supports_node_drain: false, contends_for_leader_lease: false, + suspend_commits_lifecycle: false, }; pub const KUBERNETES: Self = Self { @@ -1116,6 +1179,7 @@ impl SubstrateCapabilities { supports_node_pool_placement: true, supports_node_drain: true, contends_for_leader_lease: true, + suspend_commits_lifecycle: true, }; } diff --git a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/classify.rs b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/classify.rs index 6a1ada724..f4e173142 100644 --- a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/classify.rs +++ b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/classify.rs @@ -163,10 +163,17 @@ pub enum SkipClass { /// current execution still materializing its UID, and a same-generation pod /// a restore re-drive could resume and adopt. Pod names embed the /// generation and are unique, so no second same-generation pod can coexist - /// with the current one. A lingering active execution on a `suspended` row - /// also lands here; retiring it is the suspend subsystem's job, not this - /// loop's. + /// with the current one. ActiveRuntime, + /// The pod's generation equals the active execution's on a `suspended` + /// row. A restore revert now retires its claim's execution in the same + /// transaction, so this shape is left by a revert that predates that or + /// couldn't prove a durable restore source, or by a suspend write that + /// raced a wake. Nothing on the row says which, and in the last case the + /// pod holds the only copy of the sandbox's state, so it's never reaped + /// here. Tallied apart from [`Self::ActiveRuntime`] so its count sizes + /// that backlog; the next restore of the sandbox clears it. + SuspendedSameGeneration, /// The owning row exists but isn't Kubernetes-managed, or holds a /// lifecycle value this loop doesn't recognize. It has an owner this loop /// can't reason about, so leave the pod alone. @@ -189,6 +196,7 @@ impl SkipClass { Self::RowRestoring => "row_restoring", Self::RowTransient => "row_transient", Self::ActiveRuntime => "active_runtime", + Self::SuspendedSameGeneration => "suspended_same_generation", Self::UnrecognizedRow => "unrecognized_row", Self::RowQuarantined => "row_quarantined", Self::NoActiveExecution => "no_active_execution", @@ -215,10 +223,11 @@ impl SkipClass { /// generation, so a same-generation pod can still be adopted by a restore that /// resumes that generation — deleting it unlocked would race that adoption and /// could destroy a live or recoverable runtime. Such a pod is the current -/// execution or is still materializing its UID; on a `suspended` row it may be -/// the carrier a failed revert teardown left behind, which the suspend -/// subsystem owns. Its writers clear `active` only once that pod is gone or a -/// durable snapshot exists, which is what makes this loop's +/// execution or is still materializing its UID; on a `suspended` row it is +/// [`SkipClass::SuspendedSameGeneration`]. The writers that clear `active` on +/// a `suspended` row do so only once that pod is gone or restore selection is +/// proven to pick a durable snapshot (a restore revert retires its claim's +/// execution as `replaced` under that proof), which is what makes this loop's /// no-active-execution reap of a `suspended` row safe; a writer that cleared /// `active` with the carrier still holding the only copy would break that. /// @@ -285,6 +294,11 @@ pub fn classify_disposition( Some(active_generation) if pod_generation < active_generation => { Disposition::Orphaned(OrphanReason::RowReverted) } + Some(active_generation) + if lifecycle == L::Suspended && pod_generation == active_generation => + { + Disposition::Skip(SkipClass::SuspendedSameGeneration) + } // At or above the active generation — the current execution, // still materializing, or a same-generation pod a re-drive could // resume and adopt. Not provably superseded, so never reap it diff --git a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/mod.rs b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/mod.rs index 8eac7bcc4..749cb3ffb 100644 --- a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/mod.rs +++ b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/mod.rs @@ -47,14 +47,12 @@ //! registers the capture with `BeginArtifactBackup`, which refuses it either //! way, so a reap can't register a snapshot the sandbox's next restore would //! select. A restore whose unmaterialized claim the request abandoned (the -//! re-drive cancellation guard) retired its execution as `replaced`, and a pod -//! below the active generation has a newer one, so the refusal says the -//! execution was superseded and the node releases the VM and discards the -//! capture. A restore the transient-lifecycle -//! sweeper reverted cleared the row's node columns instead, so the capture is -//! refused for having no source node; nothing tells the node to release, and -//! the pod stays terminating (reported as `teardown_stuck`). A successful -//! DELETE therefore means that +//! re-drive cancellation guard), or that the API's rollback or the +//! transient-lifecycle sweeper reverted with abandoned-restore retirement +//! enabled, retired its execution as `replaced`, +//! and a pod below the active generation has a newer one. Either way the +//! refusal says the execution was superseded, and the node releases the VM and +//! discards the capture. A successful DELETE therefore means that //! teardown has begun (the pod is terminating), not that the VM is gone: it's //! reported as `requested_orphaned_restore_pod_delete`, and the reap is only //! confirmed (`reaped_orphaned_restore_pod`, `cleanup_completed`) once the pod @@ -99,16 +97,21 @@ //! //! ## The one orphan this loop deliberately does NOT handle //! -//! A same-generation pod on a `suspended` row — the revert's `retire_execution` -//! teardown failed before clearing `active` — is left to the suspend subsystem. -//! Reaping it safely means retiring the stale execution first (to defeat the -//! adoption race) *and* proving the pod holds no committed state that exists -//! only locally: a restored sandbox that later idle-auto-suspends presents -//! identically, and its carrier pod may hold the only copy of the suspended VM -//! state. That proof needs suspend-domain snapshot-recency and marker -//! machinery, not a pod GC, and is tracked as a separate follow-up. Until then -//! such a pod is cleaned by the next restore, or reaped here once the -//! generation advances past it. +//! A same-generation pod on a `suspended` row whose execution is still active +//! is left alone. With abandoned-restore retirement enabled +//! (`SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED`, off by default), both +//! restore reverts (the API's rollback and the transient-lifecycle sweeper's) +//! retire their claim's execution as `replaced` in the same transaction, once +//! restore selection is proven to choose a durable pre-restore source, which +//! turns their leftover pod into the no-active-execution orphan this loop +//! reaps. What remains in this shape is left by a revert with retirement +//! disabled, or that predates it, or couldn't prove the source, or by a +//! suspend write that raced a wake and put a live, routed sandbox back to +//! `suspended`. The row can't tell them apart, and in the last case the pod +//! holds the only copy of the sandbox's state, so this loop only tallies them +//! (`SkipClass::SuspendedSameGeneration`). The next restore of the sandbox +//! clears such a pod, or this loop reaps it once the generation advances past +//! it. //! //! Safeguards, in order: //! - **Restore-only.** Only pods carrying a restore annotation are considered; diff --git a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/reconciler.rs b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/reconciler.rs index 03488ca1f..31d1f83c9 100644 --- a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/reconciler.rs +++ b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/reconciler.rs @@ -1075,6 +1075,11 @@ pub async fn run( delete_failed = report.delete_failed, rate_limited = report.rate_limited, skipped = report.skipped_total(), + suspended_same_generation = report + .skipped + .get(&SkipClass::SuspendedSameGeneration) + .copied() + .unwrap_or(0), terminating = report.terminating, missing_identity = report.missing_identity, deferred = report.deferred, diff --git a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/tests.rs b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/tests.rs index d5439aa9e..f2092e7c2 100644 --- a/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/tests.rs +++ b/crates/libs/cluster/controller/server/src/orphaned_restore_pod_gc/tests.rs @@ -525,7 +525,6 @@ fn current_generation_pod_is_skipped() { SandboxLifecycle::Started, SandboxLifecycle::Paused, SandboxLifecycle::Failed, - SandboxLifecycle::Suspended, ] { let s = state(lifecycle, Some(POD_GEN)); assert_eq!( @@ -538,16 +537,16 @@ fn current_generation_pod_is_skipped() { #[test] fn same_generation_pod_is_never_reaped_even_when_row_reverted() { - // A same-generation pod on a reverted/dormant row whose execution was NOT - // retired (a failed teardown left it active) is left alone: a restore - // resuming that generation could still adopt this pod, so deleting it - // unlocked would race that adoption. Retiring the stale active execution is - // the suspended-execution convergence path's job; once it clears `active`, - // this loop reaps the pod as no-active-execution. + // A same-generation pod on a `suspended` row whose execution is still + // active is left alone: a restore resuming that generation could still + // adopt it, and a suspend that raced a wake leaves this same shape with + // the only copy of the sandbox's state in the pod. It's tallied apart from + // the live runtimes so the backlog is visible; once a revert retires the + // execution, this loop reaps the pod as no-active-execution. let s = state(SandboxLifecycle::Suspended, Some(POD_GEN)); assert_eq!( classify_disposition(Some(&s), POD_GEN), - Disposition::Skip(SkipClass::ActiveRuntime) + Disposition::Skip(SkipClass::SuspendedSameGeneration) ); } diff --git a/crates/libs/cluster/controller/server/src/suspended_execution_reaper/mod.rs b/crates/libs/cluster/controller/server/src/suspended_execution_reaper/mod.rs index 5ee4c0987..a323563a9 100644 --- a/crates/libs/cluster/controller/server/src/suspended_execution_reaper/mod.rs +++ b/crates/libs/cluster/controller/server/src/suspended_execution_reaper/mod.rs @@ -12,18 +12,24 @@ //! `placement = 'unplaced', active_node_id = NULL` //! (leaving `target_node_id` pinned as the local-snapshot affinity hint). //! -//! The node-side idle auto-suspend path performs none of that bookkeeping: -//! the node suspends the VM locally and publishes `status = 'suspended'`, -//! and the internal API's published-state convergence promotes -//! `sandboxes.lifecycle` to `'suspended'` — and stops there. The execution -//! row stays `active = TRUE, state = 'running'` with a recorded pod IP, -//! the sandbox stays `placement = 'on_node'` with `active_node_id` set, -//! and the pod's container exits (`Succeeded`) and is later deleted by pod -//! cleanup. The control plane is left claiming a live execution on a pod -//! that no longer exists, indefinitely. The stale bookkeeping feeds the -//! wake/reattach path a long-dead pod name and stale pod IP, and makes -//! forensics/operator queries keyed on `active_node_id` see thousands of -//! "active" sandboxes pinned to long-gone nodes. +//! Node-side idle auto-suspend reaches the same end state through internal +//! API's suspended projections, which retire the active execution in the +//! statement that moves the row to `suspended`. A row can still be left +//! `suspended` with an active execution by a writer that doesn't do that +//! bookkeeping, and nothing else revisits a dormant row. Its pod's container +//! exits (`Succeeded`) and is later deleted by pod cleanup, while the control +//! plane keeps claiming a live execution on a pod that no longer exists. The +//! stale bookkeeping feeds the wake/reattach path a long-dead pod name and +//! stale pod IP, and makes forensics/operator queries keyed on +//! `active_node_id` see "active" sandboxes pinned to long-gone nodes. +//! +//! A *present* pod on such a row is out of scope. A same-generation restore +//! pod there may be an abandoned restore's leftover or a live sandbox a racing +//! suspend write put back to `suspended`, and only the latter's pod holds the +//! sandbox's state. With abandoned-restore retirement enabled, the restore +//! reverts record abandonment durably instead, by retiring their claim's +//! execution as `replaced`, which leaves the pod to the orphaned-restore-pod +//! GC. //! //! ## Safety contract with the wake path //! diff --git a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/lifecycle_writer_tests.rs b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/lifecycle_writer_tests.rs index b6c8fb303..7319538ba 100644 --- a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/lifecycle_writer_tests.rs +++ b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/lifecycle_writer_tests.rs @@ -30,10 +30,15 @@ use std::sync::Arc; use std::time::Duration; -use pplx_space_api::controllers::substrate::Substrate; use pplx_space_api::controllers::substrate::direct::DirectSubstrate; +use pplx_space_api::controllers::substrate::{ + AbandonedRestoreRetirement, PodIdentityFence, RecordedExecution, RetireFence, Substrate, +}; use pplx_space_common_core::sql; -use pplx_space_common_core::types::{NodeId, PositiveDuration, SandboxId, SandboxLifecycle}; +use pplx_space_common_core::types::{ + NodeId, PodName, PodUid, PositiveDuration, SandboxActivationAttemptId, SandboxId, + SandboxLifecycle, +}; use pplx_space_db_test_fixtures::{pg_test_db, pg_test_db_with_max_connections}; use pplx_space_placement::StoredSandboxLifecycle; use pplx_space_placement::test_support::stored_lifecycle_matrix; @@ -43,7 +48,8 @@ use sqlx::PgPool; use super::{SWEEPABLE_LIFECYCLES, SubstrateSweepSink, SweepSink}; use crate::lifecycle_writer_test_support::{ ExecutionSeed, SandboxSeed, bounded, insert_execution, insert_sandbox, insert_snapshot, - kubernetes_substrate, pin_local_artifact, single_backend_pool, wait_until_blocked, + kubernetes_substrate, pin_local_artifact, pod_name, pod_uid, single_backend_pool, + wait_until_blocked, }; fn age() -> PositiveDuration { @@ -58,6 +64,7 @@ fn direct_sink(pool: &PgPool) -> SubstrateSweepSink { pool.clone(), Arc::new(Substrate::from_direct(DirectSubstrate::new(pool.clone()))), ) + .with_abandoned_restore_retirement(AbandonedRestoreRetirement::Enabled) } /// The Postgres `sandboxes` columns a sweeper write moves. @@ -525,7 +532,12 @@ async fn revert_cas_moves_only_restoring_rows() -> TestResult { insert_snapshot(pool, sandbox_id, "ready", -60).await?; let claimed = sink - .revert_to_suspended(sandbox_id, SandboxLifecycle::Restoring, age()) + .revert_to_suspended( + sandbox_id, + SandboxLifecycle::Restoring, + age(), + &RetireFence::NoExecution, + ) .await? .is_some(); @@ -566,7 +578,12 @@ async fn revert_cas_moves_only_restoring_rows() -> TestResult { .await?; } let claimed = sink - .revert_to_suspended(sandbox_id, SandboxLifecycle::Restoring, age()) + .revert_to_suspended( + sandbox_id, + SandboxLifecycle::Restoring, + age(), + &RetireFence::NoExecution, + ) .await?; assert!(claimed.is_none(), "{case}"); assert_eq!( @@ -578,6 +595,386 @@ async fn revert_cas_moves_only_restoring_rows() -> TestResult { Ok(()) } +/// The revert CAS retires the restore's execution as `replaced` in its own +/// transaction, and claims nothing unless the execution still matches every +/// part of the identity the candidate select recorded: a revert that can't +/// record the abandonment on the right execution would strand that execution +/// active on a `suspended` row. A candidate observed with no execution reverts +/// only while none exists, and one that observed nothing never reverts. +/// +/// The execution row sits under the direct sink's `space` row (see the +/// module doc): the retirement matches the execution by sandbox and identity, +/// whatever substrate labels the row. +#[tokio::test(flavor = "current_thread")] +async fn revert_cas_retires_only_the_observed_execution() -> TestResult { + let db = pg_test_db().await?; + let pool = &db.pool; + let sink = direct_sink(pool); + + let observed = |sandbox_id: SandboxId| { + PodIdentityFence::new(PodName::parse(pod_name(sandbox_id, 1)).expect("pod name")) + .with_recorded(RecordedExecution::Read { + pod_uid: Some(PodUid::parse(pod_uid(sandbox_id, 1)).expect("pod uid")), + route_activation_id: None, + }) + .with_generation(1) + }; + struct Case { + name: &'static str, + retires: bool, + fence: fn(PodIdentityFence) -> RetireFence, + /// Whether the execution row records its pod UID. One that records + /// none is what only the unread-fence refusal can tell apart. + records_pod_uid: bool, + } + fn case(name: &'static str, retires: bool, fence: fn(PodIdentityFence) -> RetireFence) -> Case { + Case { + name, + retires, + fence, + records_pod_uid: true, + } + } + let cases = [ + case("the observed execution", true, RetireFence::Pod), + case("another generation", false, |fence| { + RetireFence::Pod(fence.with_generation(2)) + }), + case("another pod", false, |fence| { + let mut fence = fence; + fence.pod_name = PodName::parse("space-sbx-elsewhere-g1").expect("pod name"); + RetireFence::Pod(fence) + }), + case("another pod uid", false, |fence| { + RetireFence::Pod(fence.with_recorded(RecordedExecution::Read { + pod_uid: Some(PodUid::parse("uid-of-a-replacement").expect("pod uid")), + route_activation_id: None, + })) + }), + case("another activation attempt", false, |fence| { + RetireFence::Pod(fence.with_activation_attempt(SandboxActivationAttemptId::new())) + }), + case("another route activation", false, |fence| { + let pod_uid = fence.recorded.pod_uid().cloned(); + RetireFence::Pod(fence.with_recorded(RecordedExecution::Read { + pod_uid, + route_activation_id: Some(SandboxActivationAttemptId::new()), + })) + }), + Case { + records_pod_uid: false, + ..case("an unread row", false, |fence| { + RetireFence::Pod(fence.with_recorded(RecordedExecution::Unread)) + }) + }, + case("no execution observed", false, |_| RetireFence::NoExecution), + case("nothing observed", false, |_| RetireFence::Unobserved), + ]; + + let mut mismatches = Vec::new(); + for Case { + name, + retires, + fence, + records_pod_uid, + } in cases + { + let sandbox_id = SandboxId::new(); + insert_row(pool, sandbox_id, &Row::bare("restoring")).await?; + insert_snapshot(pool, sandbox_id, "ready", -60).await?; + insert_execution(pool, sandbox_id, ExecutionSeed::RUNNING).await?; + sqlx::query(sql!( + "UPDATE sandboxes SET placement = 'on_node' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + if !records_pod_uid { + sqlx::query(sql!( + "UPDATE sandbox_k8s_executions SET pod_uid = NULL WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + } + + let claimed = sink + .revert_to_suspended( + sandbox_id, + SandboxLifecycle::Restoring, + age(), + &fence(observed(sandbox_id)), + ) + .await? + .is_some(); + + let (lifecycle, placement, active, state): (String, String, bool, String) = + sqlx::query_as(sql!( + "SELECT s.lifecycle, s.placement, e.active, e.state + FROM sandboxes s + JOIN sandbox_k8s_executions e ON e.sandbox_id = s.id + WHERE s.id = $1" + )) + .bind(sandbox_id) + .fetch_one(pool) + .await?; + // A retiring revert also unplaces the row, so it doesn't stay placed + // if the best-effort cleanup that follows never runs. + let expected = if retires { + ("suspended", "unplaced", false, "replaced") + } else { + ("restoring", "on_node", true, "running") + }; + if claimed != retires + || ( + lifecycle.as_str(), + placement.as_str(), + active, + state.as_str(), + ) != expected + { + mismatches.push(format!( + "{name}: claimed {claimed}, row ({lifecycle}, {placement}, {active}, {state})" + )); + } + } + assert!(mismatches.is_empty(), "{mismatches:#?}"); + Ok(()) +} + +/// With abandoned-restore retirement disabled (the default), the revert CAS +/// moves only the lifecycle, as before retirement existed: the restore's +/// execution stays active, the row stays placed and keeps its placement +/// clock, and the cleanup that follows retires the execution under the +/// candidate's fence, which refuses a pod that came up after the check. +#[tokio::test(flavor = "current_thread")] +async fn without_retirement_the_revert_moves_only_the_lifecycle() -> TestResult { + let db = pg_test_db().await?; + let pool = &db.pool; + let sink = + direct_sink(pool).with_abandoned_restore_retirement(AbandonedRestoreRetirement::Disabled); + + let sandbox_id = SandboxId::new(); + insert_row(pool, sandbox_id, &Row::bare("restoring")).await?; + insert_snapshot(pool, sandbox_id, "ready", -60).await?; + insert_execution(pool, sandbox_id, ExecutionSeed::RUNNING).await?; + sqlx::query(sql!( + "UPDATE sandboxes + SET placement = 'on_node', + placement_changed_at = lifecycle_changed_at + INTERVAL '30 seconds' + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + let placement_clock = || async { + sqlx::query_scalar::<_, Option>(sql!( + "SELECT placement_changed_at FROM sandboxes WHERE id = $1" + )) + .bind(sandbox_id) + .fetch_one(pool) + .await + }; + let clock_before = placement_clock().await?; + let fence = RetireFence::Pod( + PodIdentityFence::new(PodName::parse(pod_name(sandbox_id, 1)).expect("pod name")) + .with_recorded(RecordedExecution::Read { + pod_uid: Some(PodUid::parse(pod_uid(sandbox_id, 1)).expect("pod uid")), + route_activation_id: None, + }) + .with_generation(1), + ); + + let reverted = sink + .revert_to_suspended(sandbox_id, SandboxLifecycle::Restoring, age(), &fence) + .await? + .expect("the revert CAS claims"); + + let (lifecycle, placement, active, state): (String, String, bool, String) = + sqlx::query_as(sql!( + "SELECT s.lifecycle, s.placement, e.active, e.state + FROM sandboxes s + JOIN sandbox_k8s_executions e ON e.sandbox_id = s.id + WHERE s.id = $1" + )) + .bind(sandbox_id) + .fetch_one(pool) + .await?; + assert_eq!( + ( + lifecycle.as_str(), + placement.as_str(), + active, + state.as_str() + ), + ("suspended", "on_node", true, "running") + ); + assert_eq!(placement_clock().await?, clock_before); + assert_eq!(reverted.prior_placement_changed_at, clock_before); + Ok(()) +} + +/// A placed-then-reverted restore is durably restorable as soon as the revert +/// lands. Its pod's bind moved `placement_changed_at` past the pre-restore +/// snapshot, and `durably_restorable`, which gates the fenced release of the +/// abandoned pod's runtime, takes a markerless snapshot only when it is newer +/// than the last placement. The revert clears the clock, as the API store's +/// markerless restoring recovery does; left in place, the release would wait +/// for the next suspend and the pod would count against `teardown_stuck`. +#[tokio::test(flavor = "current_thread")] +async fn placed_then_reverted_restore_is_durably_restorable_at_once() -> TestResult { + let db = pg_test_db().await?; + let pool = &db.pool; + let sink = direct_sink(pool); + + let sandbox_id = SandboxId::new(); + insert_row(pool, sandbox_id, &Row::bare("restoring")).await?; + insert_snapshot(pool, sandbox_id, "ready", -60).await?; + // The restore's pod bound after the pre-restore snapshot was taken. + sqlx::query(sql!( + "UPDATE sandboxes + SET placement_changed_at = lifecycle_changed_at + INTERVAL '30 seconds' + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + + let reverted = sink + .revert_to_suspended( + sandbox_id, + SandboxLifecycle::Restoring, + age(), + &RetireFence::NoExecution, + ) + .await? + .expect("the revert CAS claims"); + + assert_eq!(row(pool, sandbox_id).await?.lifecycle, "suspended"); + assert!( + pplx_space_sandbox_store::durably_restorable(pool, sandbox_id).await?, + "a fenced release must not wait for the next suspend" + ); + assert!(reverted.prior_placement_changed_at.is_some()); + Ok(()) +} + +/// A restore's owner commits `restoring -> started` under its own lease as +/// soon as its wake succeeds, before its request dials the runtime (the API's +/// `complete_owned_user_restore`). A revert the sweeper decided on before that +/// commit claims nothing after it, so it can't retire the runtime the owner's +/// request is using, even with the observed execution's exact fence. +#[tokio::test(flavor = "current_thread")] +async fn revert_after_the_owner_commits_started_claims_nothing() -> TestResult { + let db = pg_test_db().await?; + let pool = &db.pool; + let sink = direct_sink(pool); + + let sandbox_id = SandboxId::new(); + insert_row(pool, sandbox_id, &Row::bare("restoring").leased()).await?; + insert_snapshot(pool, sandbox_id, "ready", -60).await?; + insert_execution(pool, sandbox_id, ExecutionSeed::RUNNING).await?; + sqlx::query(sql!( + "UPDATE sandboxes SET placement = 'on_node' WHERE id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + // The sweep observed the restore's execution before the owner finished. + let fence = RetireFence::Pod( + PodIdentityFence::new(PodName::parse(pod_name(sandbox_id, 1)).expect("pod name")) + .with_recorded(RecordedExecution::Read { + pod_uid: Some(PodUid::parse(pod_uid(sandbox_id, 1)).expect("pod uid")), + route_activation_id: None, + }) + .with_generation(1), + ); + // The owner's commit: under the row's own lease token, from `restoring`. + // `complete_owned_user_restore` is private to the API's Kubernetes store + // (and tested there); what this pins is the sweeper side: a `started` row. + let committed = sqlx::query(sql!( + "UPDATE sandboxes + SET lifecycle = 'started', placement = 'on_node' + WHERE id = $1 + AND lifecycle = 'restoring' + AND restore_lease_token IS NOT NULL" + )) + .bind(sandbox_id) + .execute(pool) + .await? + .rows_affected(); + assert_eq!(committed, 1); + + let claimed = sink + .revert_to_suspended(sandbox_id, SandboxLifecycle::Restoring, age(), &fence) + .await?; + + assert!(claimed.is_none(), "the revert must not claim a started row"); + assert_eq!(row(pool, sandbox_id).await?.lifecycle, "started"); + let (active, state): (bool, String) = sqlx::query_as(sql!( + "SELECT active, state FROM sandbox_k8s_executions WHERE sandbox_id = $1" + )) + .bind(sandbox_id) + .fetch_one(pool) + .await?; + assert_eq!((active, state.as_str()), (true, "running")); + Ok(()) +} + +/// The revert undo puts back the exact `placement_changed_at` the revert +/// cleared, so the `restoring` row's restore retry window reads the same +/// placement clock it did before the sweep. +#[tokio::test(flavor = "current_thread")] +async fn revert_undo_restores_the_placement_clock() -> TestResult { + let db = pg_test_db().await?; + let pool = &db.pool; + let sink = direct_sink(pool); + + let sandbox_id = SandboxId::new(); + insert_row(pool, sandbox_id, &Row::bare("restoring").leased()).await?; + insert_snapshot(pool, sandbox_id, "ready", -60).await?; + sqlx::query(sql!( + "UPDATE sandboxes + SET placement_changed_at = lifecycle_changed_at + INTERVAL '30 seconds' + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(pool) + .await?; + let placement_clock = || async { + sqlx::query_scalar::<_, Option>(sql!( + "SELECT placement_changed_at FROM sandboxes WHERE id = $1" + )) + .bind(sandbox_id) + .fetch_one(pool) + .await + }; + let original = placement_clock().await?; + + let reverted = sink + .revert_to_suspended( + sandbox_id, + SandboxLifecycle::Restoring, + age(), + &RetireFence::NoExecution, + ) + .await? + .expect("the revert CAS claims"); + assert_eq!( + placement_clock().await?, + None, + "the revert clears the clock" + ); + + assert!( + sink.undo_revert_to_suspended(sandbox_id, SandboxLifecycle::Restoring, &reverted, false) + .await? + ); + assert_eq!(row(pool, sandbox_id).await?.lifecycle, "restoring"); + assert_eq!(placement_clock().await?, original); + Ok(()) +} + /// The revert undo moves a row back only out of `suspended` and only while /// the revert's write is the row's last, restoring the lifecycle and the /// fork-preservation flag it is handed but not the lease. @@ -598,7 +995,12 @@ async fn revert_undo_reverts_only_its_own_suspended_write() -> TestResult { .await?; insert_snapshot(pool, sandbox_id, "ready", -60).await?; let token = sink - .revert_to_suspended(sandbox_id, SandboxLifecycle::Restoring, age()) + .revert_to_suspended( + sandbox_id, + SandboxLifecycle::Restoring, + age(), + &RetireFence::NoExecution, + ) .await? .expect("the accepting revert CAS claims"); relabel_keeping_write_token(pool, sandbox_id, stored).await?; diff --git a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/mod.rs b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/mod.rs index 37ee4deed..f6a57af14 100644 --- a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/mod.rs +++ b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/mod.rs @@ -119,7 +119,8 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use pplx_space_api::controllers::substrate::{ - CommandReadyProbe, PodFenceMismatch, RetireExecutionOutcome, RetireFence, Substrate, + AbandonedRestoreRetirement, CommandReadyProbe, PodFenceMismatch, RetireExecutionOutcome, + RetireFence, Substrate, }; use pplx_space_common_core::types::{PositiveDuration, SandboxId, SandboxLifecycle}; use pplx_space_events::{ @@ -682,6 +683,12 @@ pub struct ReconcilerConfig { /// `restoring` row is failed. See /// [`DEFAULT_UNRESTORABLE_RESTORING_AGE_SECS`]. pub unrestorable_restoring_min_placement_age: PositiveDuration, + /// Whether the revert to `suspended` retires the abandoned restore's + /// execution (`SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED`, default off; + /// see [`AbandonedRestoreRetirement`]). Disabled, the revert only moves + /// the lifecycle and the cleanup retires the execution afterwards, + /// refusing a pod that came up since the candidate was observed. + pub abandoned_restore_retirement: AbandonedRestoreRetirement, } /// Per-lifecycle override resolution. @@ -730,6 +737,7 @@ impl Default for ReconcilerConfig { unrestorable_restoring_min_placement_age: PositiveDuration::new_panic( Duration::from_secs(DEFAULT_UNRESTORABLE_RESTORING_AGE_SECS), ), + abandoned_restore_retirement: AbandonedRestoreRetirement::Disabled, } } } @@ -781,6 +789,7 @@ impl ReconcilerConfig { unrestorable_restoring_enabled: true, unrestorable_restoring_min_placement_age: default .unrestorable_restoring_min_placement_age, + abandoned_restore_retirement: AbandonedRestoreRetirement::from_env(), } } @@ -837,6 +846,15 @@ fn env_lifecycle_age(var: &str) -> LifecycleAge { } } +/// A revert to `suspended` the CAS claimed, with what undoing it needs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RevertedToSuspended { + /// Names the exact lifecycle write the revert made. + pub write_token: String, + /// The row's `placement_changed_at` before the revert cleared it. + pub prior_placement_changed_at: Option, +} + /// A `failed` row with an active Kubernetes execution that the /// failed-cleanup pass wants to retire, plus the node to probe first. #[derive(Debug, Clone, PartialEq, Eq)] @@ -937,14 +955,35 @@ pub trait SweepSink: Send + Sync + 'static { /// implementation hard-pins `lifecycle = 'restoring'`: every other /// transient lifecycle's matching backup is a stale prior-cycle copy /// (see `classify::decide`), so the CAS must claim nothing even if - /// classification drifts. Returns the token naming the lifecycle write - /// this call made, or `None` if it claimed nothing. + /// classification drifts. + /// + /// With abandoned-restore retirement enabled, the same transaction + /// retires the restore's execution as `replaced`, matching `fence`'s + /// recorded identity (or, for a candidate observed with no execution, + /// requires that none exists). That retirement is what records the + /// restore as abandoned, so the revert claims nothing unless it lands + /// too. Disabled (the default, see + /// [`ReconcilerConfig::abandoned_restore_retirement`]), the revert moves + /// only the lifecycle, and the cleanup retires the execution afterwards + /// under `fence`. + /// + /// A retiring revert also clears `placement_changed_at`, as the API + /// store's markerless restoring recovery does. A restore whose pod bound + /// moved it past the pre-restore snapshot, and both the suspended restore + /// selector and `durably_restorable` take a markerless snapshot only when + /// it is newer than the last placement: left in place, the proven source isn't + /// selected, and a fenced release of the abandoned pod's runtime is held + /// until the next suspend. + /// + /// Returns what the write needs to be undone, or `None` if it claimed + /// nothing. fn revert_to_suspended( &self, sandbox_id: SandboxId, lifecycle: SandboxLifecycle, age: PositiveDuration, - ) -> impl std::future::Future, sqlx::Error>> + Send; + fence: &RetireFence, + ) -> impl std::future::Future, sqlx::Error>> + Send; /// CAS one provably-unrecoverable row from `lifecycle` to `'failed'`, /// with the same evacuation-exemption guards as the candidate select. @@ -1020,21 +1059,34 @@ pub trait SweepSink: Send + Sync + 'static { fence: RetireFence, ) -> impl std::future::Future> + Send; - /// Undo a revert this sweep just made, when the retire it was supposed to - /// enable turned out to be refused. + /// Undo a revert this sweep just made, when the cleanup it was supposed + /// to enable turned out to be refused. + /// + /// With abandoned-restore retirement disabled (the default), the revert + /// moved only the lifecycle and the cleanup retires the execution under + /// the candidate's fence. A pod that came up after the pre-check, which a + /// Pending pod going Running is enough for, fails that fence and leaves a + /// `suspended` row owning a live execution, which no reconciler revisits; + /// the restore the sweep judged dead is running after all, so putting the + /// lifecycle back is simply correct. + /// + /// With retirement enabled, the revert retired the restore's execution in + /// its own transaction, so the cleanup refuses only when an execution it + /// never observed is active by the time it runs. Putting the lifecycle + /// back lets the next tick judge the row afresh; the retired execution + /// stays `replaced`, since a restore never re-adopts a retired + /// generation. A pod that merely came up after the pre-check isn't a + /// refusal then: the cleanup deletes it as the abandoned restore's + /// leftover (see `candidate_pod_fence_with_observation`). /// - /// The pre-check narrows that window but cannot close it: a pod that - /// recovers between the question and the CAS — a Pending pod going Running - /// is enough, since that alone changes the resourceVersion — leaves a - /// `suspended` row owning a live execution, and no reconciler revisits a - /// suspended row. Putting the lifecycle back is also simply correct in - /// that case: the restore the sweep judged dead is running after all. /// CAS-guarded on the row still being `suspended`, so a genuine suspend - /// that landed since is not clobbered. - /// `write_token` is what the revert CAS returned, naming that exact - /// lifecycle write. Undoing on the lifecycle value alone would reach past - /// this sweep's own transition and drag a later, legitimate suspend back - /// to `restoring`. + /// that landed since is not clobbered. `reverted` is what the revert CAS + /// returned. Its write token names that + /// exact lifecycle write: undoing on the lifecycle value alone would reach + /// past this sweep's own transition and drag a later, legitimate suspend + /// back to `restoring`. Its prior `placement_changed_at` is put back, so + /// the restored `restoring` row keeps the placement clock its restore + /// retry window reads. /// /// `preserve_local_artifact` is the candidate's `fork_restore`, restored /// alongside the lifecycle because leaving `restoring` cleared it. Without @@ -1044,7 +1096,7 @@ pub trait SweepSink: Send + Sync + 'static { &self, sandbox_id: SandboxId, lifecycle: SandboxLifecycle, - write_token: &str, + reverted: &RevertedToSuspended, preserve_local_artifact: bool, ) -> impl std::future::Future> + Send; @@ -1508,10 +1560,11 @@ impl Reconciler { report: &mut TickReport, ) { // Ask before committing (a UID backfill aside, this changes nothing). - // The revert below moves the sandbox to - // `suspended`, which no reconciler revisits, so a fence lost between - // the candidate select and here must be discovered *now* — after the - // CAS, the only remedy is a dormant row owning a live execution. + // The revert below moves the sandbox to `suspended`, which no + // reconciler revisits. Its transaction re-checks the execution row's + // recorded identity, but only this check can see the live pod: a pod + // replaced or moved on since the candidate select must stop the + // revert here. match self .sink .retire_fence_mismatch(candidate.sandbox_id, &candidate.pod_fence) @@ -1541,9 +1594,14 @@ impl Reconciler { } } - let write_token = match self + let reverted = match self .sink - .revert_to_suspended(candidate.sandbox_id, candidate.lifecycle, age) + .revert_to_suspended( + candidate.sandbox_id, + candidate.lifecycle, + age, + &candidate.pod_fence, + ) .await { Ok(claimed) => claimed, @@ -1556,7 +1614,7 @@ impl Reconciler { return; } }; - let Some(write_token) = write_token else { + let Some(reverted) = reverted else { OutcomeEvent::aged_transient( AgedTransientOutcome::CasLost, candidate.lifecycle.as_str(), @@ -1586,18 +1644,16 @@ impl Reconciler { "recoverable_snapshot", ); - // A claimed revert leaves the wedged restore's execution shape - // behind: an active `sandbox_k8s_executions` row, `placement = - // 'placed'`, and possibly a still-Running pod. - // `suspended` is a contract that none of those exist — the normal - // suspend completion clears them all in one transaction — and no - // reconciler collects them on a dormant row (the orphaned-sandbox - // reconciler excludes dormant lifecycles; the failed-cleanup pass - // only lists `failed`). Retire the execution the same way the fail - // path does: it tears down the pod while preserving node-local state, - // finishes the execution row, and unplaces the sandbox. Best-effort: - // the row is already `suspended`, - // so a failure here is logged loudly and leaves only a leak. + // The revert already retired the restore's execution, so what's left + // of the wedged restore is its pod and the row's placement: + // `suspended` is a contract that neither remains, and no reconciler + // collects them on a dormant row (the orphaned-sandbox reconciler + // excludes dormant lifecycles; the failed-cleanup pass only lists + // `failed`). The retire finds no active execution, deletes the pods + // recorded on inactive rows and unplaces the sandbox. Best-effort: a + // pod it misses belongs to no active execution, which the + // orphaned-restore-pod GC and the next restore's lingering-pod sweep + // both reap. emit_revert_cleanup_event( candidate.sandbox_id, SandboxEventType::CleanupStarted, @@ -1648,16 +1704,17 @@ impl Reconciler { ); return; } - // The revert is already durable and the execution it was meant - // to retire is still live, so leaving the row `suspended` - // strands it where nothing looks again. Put the lifecycle back - // and let the next tick judge it afresh. + // The revert retired the restore's execution, so the cleanup + // refused because an execution it never observed is active + // now. Leaving the row `suspended` would strand that one where + // nothing looks again. Put the lifecycle back, if the revert is + // still the row's last write, and let the next tick judge it. match self .sink .undo_revert_to_suspended( candidate.sandbox_id, candidate.lifecycle, - &write_token, + &reverted, candidate.fork_restore, ) .await @@ -1682,8 +1739,8 @@ impl Reconciler { sandbox_id = %candidate.sandbox_id, error = %err, lifecycle = %candidate.lifecycle, - "could not undo a revert whose retire was refused; the row is \ - suspended with a live execution and no reconciler revisits it" + "could not undo a revert whose cleanup was refused; the row is \ + suspended with an active execution and no reconciler revisits it" ), } } @@ -1694,11 +1751,15 @@ impl Reconciler { ) .sandbox_id(candidate.sandbox_id) .error(&err) - .detail( + .detail(if self.config.abandoned_restore_retirement.is_enabled() { + "pod cleanup failed after revert to suspended; the execution is \ + retired, so orphan GC or the next restore removes the pod, but the \ + row keeps its placement, which nothing revisits on a suspended row" + } else { "execution retire failed after revert to suspended; stale execution, \ placement, and pod persist until the node terminates (no reconciler \ - retries them on a suspended row)", - ) + retries them on a suspended row)" + }) .warn(); report.cleanup_failures += 1; emit_revert_cleanup_event( @@ -2592,6 +2653,7 @@ pub async fn run_with_substrate( substrate: Arc, shutdown_token: CancellationToken, ) { - let sink = SubstrateSweepSink::new(pool, substrate); + let sink = SubstrateSweepSink::new(pool, substrate) + .with_abandoned_restore_retirement(config.abandoned_restore_retirement); run(config, sink, shutdown_token).await; } diff --git a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs index 20f111e12..6733e7be4 100644 --- a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs +++ b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/store.rs @@ -26,7 +26,8 @@ use sqlx::PgPool; use super::classify::{ExecutionRoutability, ProbeObservation}; use super::probe::{ProbeNodeError, SweepCandidate}; -use super::{CommandReadyPendingCandidate, FailedCleanupCandidate, SweepSink}; +use super::{CommandReadyPendingCandidate, FailedCleanupCandidate, RevertedToSuspended, SweepSink}; +use pplx_space_api::controllers::substrate::AbandonedRestoreRetirement; /// Max preserved `command_ready_pending` rows resolved per tick. Each costs one /// non-recovering guest probe (bounded by the configured probe timeout), so the @@ -169,9 +170,11 @@ const LIFECYCLE_WRITE_TOKEN: &str = "to_char(lifecycle_changed_at, 'YYYY-MM-DD H /// Put a reverted row back, bound to the revert's own write. /// -/// Restores `preserve_local_artifact_on_restore_failure` alongside the -/// lifecycle: leaving `restoring` fired `sandboxes_clear_restore_lease`, which -/// clears it, and a fork restore that came back without its preservation +/// Restores the `placement_changed_at` a retiring revert cleared (a clock +/// still set, by a lifecycle-only revert or a pod bind since, is kept), and +/// `preserve_local_artifact_on_restore_failure` alongside the lifecycle: +/// leaving `restoring` fired `sandboxes_clear_restore_lease`, which clears +/// it, and a fork restore that came back without its preservation /// contract could discard the local artifact it was supposed to keep. The /// lease itself is deliberately *not* restored — another actor may hold one by /// now. @@ -179,7 +182,8 @@ pub(crate) fn undo_revert_to_suspended_query() -> String { format!( "UPDATE sandboxes SET lifecycle = $3, - preserve_local_artifact_on_restore_failure = $5 + preserve_local_artifact_on_restore_failure = $5, + placement_changed_at = COALESCE(placement_changed_at, $6) WHERE id = $1 AND managed_by = $2 AND lifecycle = 'suspended' @@ -329,10 +333,26 @@ pub(crate) struct AgedTransientRow { /// upgraded with what the substrate currently sees. /// /// The persisted UID cannot tell a pod that *moved*: a Pending pod becoming -/// Running keeps its UID, and that is precisely the recovery this sweep must -/// not destroy — its evidence is a node probe, and a pod coming up after that -/// probe invalidates it. A resourceVersion observed here, before the probe and -/// the CAS, is what the substrate re-checks at teardown to notice. +/// Running keeps its UID, and the sweep's evidence that the sandbox has no +/// live runtime is a node probe, which a pod coming up after it invalidates. +/// A resourceVersion observed here, before the probe and the CAS, is what the +/// substrate re-checks to notice. +/// +/// The revert to `suspended` re-checks it once, before its CAS +/// (`retire_fence_mismatch`). With abandoned-restore retirement disabled (the +/// default), the cleanup then retires the still-active execution under this +/// fence and refuses a pod that came up since, and the revert is undone. +/// Enabled, the revert retires the execution in the CAS's own transaction, +/// and a restore pod that binds after that check is the abandoned +/// restore's leftover: the restore's lease has expired, its row aged past the +/// sweep's window, and the revert's snapshot proof says restore selection +/// picks a durable pre-restore source. So the cleanup deletes it, the row +/// stays `suspended`, and the next wake restores from that source. The row +/// never reached `started`, and no user request reached its VM: the API's +/// access, runtime-client, ingress and data-plane route paths never dial a +/// `restoring` sandbox's pod, so nothing it holds is newer than the source. +/// That holds only once every routing replica runs those paths, which is why +/// retirement stays disabled until then (see `AbandonedRestoreRetirement`). /// /// The observation may only *confirm* the row's persisted UID, never replace /// it. The row is read by one query and the pods by a later one, so a row that @@ -603,7 +623,32 @@ pub(crate) fn converge_restoring_query() -> String { /// * the suspended-revert-safe restore snapshot predicate, so a late artifact /// published by a failed restore branch can't become the snapshot a later /// suspended restore selects. +/// +/// It also clears `placement_changed_at` (see +/// [`SweepSink::revert_to_suspended`]). pub(crate) fn revert_to_suspended_query() -> String { + let ready_snapshot = restore_retry_suspended_revert_snapshot_exists("sandboxes"); + format!( + "UPDATE sandboxes + SET lifecycle = 'suspended', + placement_changed_at = NULL + WHERE managed_by = $1 + AND lifecycle = $2 + AND lifecycle = 'restoring' + AND lifecycle_changed_at < NOW() - make_interval(secs => $3::BIGINT) + AND (restore_lease_expires_at IS NULL + OR restore_lease_expires_at < NOW() - make_interval(secs => $3::BIGINT)) + AND ({ready_snapshot}) + AND id = $4 + RETURNING {LIFECYCLE_WRITE_TOKEN}" + ) +} + +/// [`revert_to_suspended_query`] with abandoned-restore retirement disabled: +/// the same guards, moving only the lifecycle. The execution, placement and +/// placement clock stay, and the cleanup that follows retires the execution +/// under the candidate's fence, refusing a pod that came up since. +pub(crate) fn revert_to_suspended_lifecycle_only_query() -> String { let ready_snapshot = restore_retry_suspended_revert_snapshot_exists("sandboxes"); format!( "UPDATE sandboxes @@ -620,6 +665,80 @@ pub(crate) fn revert_to_suspended_query() -> String { ) } +/// Retire the reverted restore's execution as `replaced` inside the revert +/// CAS's transaction, matching the identity the candidate select recorded +/// (see [`SweepSink::revert_to_suspended`]). A crash, error or refusal +/// between a revert and a separate retire would leave a `suspended` row with +/// its same-generation execution still active, which nothing revisits. +/// +/// Returns `false`, and the caller rolls the revert back, when the active +/// execution is no longer the one the candidate observed. +async fn retire_reverted_restore_execution( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + sandbox_id: SandboxId, + fence: &PodIdentityFence, +) -> Result { + if fence.recorded == RecordedExecution::Unread { + return Ok(false); + } + let retired = sqlx::query(sql!( + "UPDATE sandbox_k8s_executions + SET active = FALSE, + state = 'replaced', + pod_ip = NULL, + terminated_at = COALESCE(terminated_at, NOW()), + updated_at = NOW() + WHERE sandbox_id = $1 + AND active + AND pod_name = $2 + AND generation IS NOT DISTINCT FROM $3 + AND activation_attempt_id IS NOT DISTINCT FROM $4 + AND NULLIF(BTRIM(pod_uid), '') IS NOT DISTINCT FROM $5::TEXT + AND route_activation_id IS NOT DISTINCT FROM $6" + )) + .bind(sandbox_id) + .bind(fence.pod_name.as_str()) + .bind(fence.generation) + .bind(fence.activation_attempt_id) + .bind(fence.recorded.pod_uid().map(PodUid::as_str)) + .bind(fence.recorded.route_activation_id()) + .execute(&mut **tx) + .await? + .rows_affected() + == 1; + if retired { + // The retired execution no longer places the row. The cleanup that + // follows unplaces it too, but it is best-effort, and a `suspended` + // row left reading as placed is one nothing revisits. + sqlx::query(sql!( + "UPDATE sandboxes + SET placement = 'unplaced', + active_node_id = NULL + WHERE id = $1" + )) + .bind(sandbox_id) + .execute(&mut **tx) + .await?; + // The node state the open exposures described died with the runtime. + sqlx::query(sql!( + "UPDATE sandbox_port_exposures + SET apply_state = 'pending', + last_applied_node_id = NULL, + last_error = NULL, + last_bridge_status = NULL, + last_applied_bridge_generation = NULL, + last_observed_at = NULL, + updated_at = NOW() + WHERE sandbox_id = $1 + AND desired_state = 'open'" + )) + .bind(sandbox_id) + .execute(&mut **tx) + .await?; + } + Ok(retired) +} + /// Shared predicate for the unrestorable-`restoring` sweep: a markerless /// `restoring` row that the restore planner **provably cannot restore from /// anywhere** — the exact wedge the aged-transient sweep above cannot reach, @@ -737,6 +856,8 @@ pub struct SubstrateSweepSink { /// replaced node-api pod gets a fresh connection. Probing is sequential, /// so the `tokio::sync::Mutex` is uncontended. probe_clients: tokio::sync::Mutex>, + /// See [`super::ReconcilerConfig::abandoned_restore_retirement`]. + abandoned_restore_retirement: AbandonedRestoreRetirement, } #[derive(Clone)] @@ -768,9 +889,20 @@ impl SubstrateSweepSink { pool, substrate, probe_clients: tokio::sync::Mutex::new(HashMap::new()), + abandoned_restore_retirement: AbandonedRestoreRetirement::Disabled, } } + /// Whether [`SweepSink::revert_to_suspended`] retires the abandoned + /// restore's execution. Disabled by default. + pub fn with_abandoned_restore_retirement( + mut self, + retirement: AbandonedRestoreRetirement, + ) -> Self { + self.abandoned_restore_retirement = retirement; + self + } + async fn probe_client( &self, node_name: &str, @@ -1061,7 +1193,7 @@ impl SweepSink for SubstrateSweepSink { &self, sandbox_id: SandboxId, lifecycle: SandboxLifecycle, - write_token: &str, + reverted: &RevertedToSuspended, preserve_local_artifact: bool, ) -> Result { let managed_by = self.substrate.managed_by(); @@ -1075,8 +1207,9 @@ impl SweepSink for SubstrateSweepSink { .bind(sandbox_id) .bind(managed_by) .bind(lifecycle.to_string()) - .bind(write_token) + .bind(&reverted.write_token) .bind(preserve_local_artifact) + .bind(reverted.prior_placement_changed_at) .fetch_optional(&self.pool) .await?; Ok(restored.is_some()) @@ -1087,17 +1220,84 @@ impl SweepSink for SubstrateSweepSink { sandbox_id: SandboxId, lifecycle: SandboxLifecycle, age: PositiveDuration, - ) -> Result, sqlx::Error> { + fence: &RetireFence, + ) -> Result, sqlx::Error> { let managed_by = self.substrate.managed_by(); - let claimed = - sqlx::query_scalar::<_, String>(sqlx::AssertSqlSafe(sql!(revert_to_suspended_query()))) - .bind(managed_by) - .bind(lifecycle.to_string()) - .bind(Self::age_secs_bind(age)) + let mut tx = self.pool.begin().await?; + // Take the sandbox row lock that every execution claim also takes, so + // the execution check below can't race a new claim, and read the + // placement clock the CAS clears while holding it. + let prior_placement_changed_at: Option> = + sqlx::query_scalar(sql!( + "SELECT placement_changed_at + FROM sandboxes + WHERE id = $1 + AND managed_by = $2 + FOR UPDATE" + )) + .bind(sandbox_id) + .bind(managed_by) + .fetch_optional(&mut *tx) + .await?; + let Some(prior_placement_changed_at) = prior_placement_changed_at else { + tx.rollback().await?; + return Ok(None); + }; + let retire = self.abandoned_restore_retirement.is_enabled(); + let query = if retire { + revert_to_suspended_query() + } else { + revert_to_suspended_lifecycle_only_query() + }; + let claimed = sqlx::query_scalar::<_, String>(sqlx::AssertSqlSafe(sql!(query.as_str()))) + .bind(managed_by) + .bind(lifecycle.to_string()) + .bind(Self::age_secs_bind(age)) + .bind(sandbox_id) + .fetch_optional(&mut *tx) + .await?; + let Some(write_token) = claimed else { + tx.rollback().await?; + return Ok(None); + }; + if !retire { + // The cleanup retires the execution afterwards, under the fence. + tx.commit().await?; + return Ok(Some(RevertedToSuspended { + write_token, + prior_placement_changed_at, + })); + } + let execution_settled = match fence { + RetireFence::Pod(pod_fence) => { + retire_reverted_restore_execution(&mut tx, sandbox_id, pod_fence).await? + } + RetireFence::NoExecution => { + !sqlx::query_scalar::<_, bool>(sql!( + "SELECT EXISTS ( + SELECT 1 + FROM sandbox_k8s_executions + WHERE sandbox_id = $1 + AND active + )" + )) .bind(sandbox_id) - .fetch_optional(&self.pool) - .await?; - Ok(claimed) + .fetch_one(&mut *tx) + .await? + } + // The candidate select always observes one of the two above; a + // caller that observed nothing can't name the execution to retire. + RetireFence::Unobserved => false, + }; + if !execution_settled { + tx.rollback().await?; + return Ok(None); + } + tx.commit().await?; + Ok(Some(RevertedToSuspended { + write_token, + prior_placement_changed_at, + })) } async fn fail_aged_candidate( diff --git a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/tests.rs b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/tests.rs index b4ec774b3..0d7d99e30 100644 --- a/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/tests.rs +++ b/crates/libs/cluster/controller/server/src/transient_lifecycle_sweeper/tests.rs @@ -511,7 +511,7 @@ impl SweepSink for FakeSink { &self, sandbox_id: SandboxId, _lifecycle: SandboxLifecycle, - _write_token: &str, + _reverted: &RevertedToSuspended, _preserve_local_artifact: bool, ) -> Result { let mut state = self.0.lock().unwrap(); @@ -524,10 +524,16 @@ impl SweepSink for FakeSink { sandbox_id: SandboxId, _lifecycle: SandboxLifecycle, _age: PositiveDuration, - ) -> Result, sqlx::Error> { + _fence: &RetireFence, + ) -> Result, sqlx::Error> { let mut state = self.0.lock().unwrap(); state.revert_calls.push(sandbox_id); - Ok((!state.cas_unclaimed.contains(&sandbox_id)).then(|| "revert-write".to_string())) + Ok( + (!state.cas_unclaimed.contains(&sandbox_id)).then(|| RevertedToSuspended { + write_token: "revert-write".to_string(), + prior_placement_changed_at: None, + }), + ) } async fn fail_aged_candidate( @@ -829,6 +835,7 @@ fn config_with_all_overrides(age: PositiveDuration) -> ReconcilerConfig { .unwrap(), unrestorable_restoring_enabled: true, unrestorable_restoring_min_placement_age: age, + abandoned_restore_retirement: Default::default(), } } @@ -4116,6 +4123,7 @@ mod postgres_backed { .bind(SandboxLifecycle::Restoring.to_string()) .bind(token) .bind(false) + .bind(None::) .fetch_optional(pool) .await }; @@ -4189,6 +4197,7 @@ mod postgres_backed { .bind(SandboxLifecycle::Restoring.to_string()) .bind(write_token) .bind(true) + .bind(None::) .fetch_optional(pool) .await?; assert!(restored.is_some(), "the undo must claim its own write"); diff --git a/crates/libs/internal/api/server/src/data_plane_sessions.rs b/crates/libs/internal/api/server/src/data_plane_sessions.rs index 24da5b395..5f29b7eb2 100644 --- a/crates/libs/internal/api/server/src/data_plane_sessions.rs +++ b/crates/libs/internal/api/server/src/data_plane_sessions.rs @@ -95,6 +95,23 @@ fn activation_may_repair(ingress: bool, error: &Status) -> bool { ) } +/// No data-plane route resolves to a sandbox that is `restoring`: its +/// restore can still fail and be rolled back, and the rollback retires the +/// execution and deletes its pod, discarding whatever it served. The +/// resolvers turn this refusal into a wait on the restore through the +/// activation owner, then retry, and return it (retryable `Unavailable`) +/// when the restore didn't reach `started` in their budget. +fn refuse_restoring_route(lifecycle: SandboxLifecycle) -> Result<(), Status> { + if lifecycle == SandboxLifecycle::Restoring { + Err(sandbox_service_route_not_ready_status( + Code::Unavailable, + "sandbox is restoring; retry once the restore completes", + )) + } else { + Ok(()) + } +} + fn require_started_ingress(lifecycle: SandboxLifecycle) -> Result<(), Status> { if lifecycle == SandboxLifecycle::Started { Ok(()) diff --git a/crates/libs/internal/api/server/src/data_plane_sessions/route_rows.rs b/crates/libs/internal/api/server/src/data_plane_sessions/route_rows.rs index b5f09a458..3b106fbbe 100644 --- a/crates/libs/internal/api/server/src/data_plane_sessions/route_rows.rs +++ b/crates/libs/internal/api/server/src/data_plane_sessions/route_rows.rs @@ -74,7 +74,13 @@ pub(super) async fn update_route_row( backend_endpoint: &str, backend_transport: &str, ) -> Result { - sqlx::query_as::<_, SessionRouteRow>(sql!( + // The update re-reads the sandbox, and is rolled back if it refuses + // what it read, so a refused refresh changes no route. + let mut tx = pool + .begin() + .await + .map_err(|error| Status::internal(format!("update data-plane session route: {error}")))?; + let row = sqlx::query_as::<_, SessionRouteRow>(sql!( "UPDATE data_plane_sessions d SET current_node_id = $2, backend_endpoint = $3, @@ -138,10 +144,18 @@ pub(super) async fn update_route_row( .bind(current_node_id) .bind(backend_endpoint) .bind(backend_transport) - .fetch_optional(pool) + .fetch_optional(&mut *tx) .await .map_err(|error| Status::internal(format!("update data-plane session route: {error}")))? - .ok_or_else(|| Status::not_found("data-plane session not found")) + .ok_or_else(|| Status::not_found("data-plane session not found"))?; + // A suspend and a restore may have committed since the caller's first + // read, and the refreshed identity is then the restoring replacement's. + // Dropping the transaction rolls the route change back. + refuse_restoring_route(row.sandbox_lifecycle)?; + tx.commit() + .await + .map_err(|error| Status::internal(format!("update data-plane session route: {error}")))?; + Ok(row) } /// Explicit registered endpoints win; database INET host values need IPv6 brackets. diff --git a/crates/libs/internal/api/server/src/data_plane_sessions/service_route.rs b/crates/libs/internal/api/server/src/data_plane_sessions/service_route.rs index a9c8e6778..002a19cb7 100644 --- a/crates/libs/internal/api/server/src/data_plane_sessions/service_route.rs +++ b/crates/libs/internal/api/server/src/data_plane_sessions/service_route.rs @@ -67,11 +67,16 @@ pub(crate) async fn unstable_resolve_sandbox_service_route( None }; let ingress = transport == SandboxServiceRouteTransport::ServicePortIngress; + let lifecycle: Result = row + .lifecycle + .parse() + .map_err(|error| Status::internal(format!("invalid sandbox lifecycle: {error}"))); + let restoring = matches!(lifecycle, Ok(SandboxLifecycle::Restoring)); let result = async { + let lifecycle = lifecycle?; + refuse_restoring_route(lifecycle)?; if transport == SandboxServiceRouteTransport::ServicePortIngress { - require_started_ingress(row.lifecycle.parse().map_err(|error| { - Status::internal(format!("invalid sandbox lifecycle: {error}")) - })?)?; + require_started_ingress(lifecycle)?; } let current_node_id = row.active_node_id.ok_or_else(|| { sandbox_service_route_not_ready_status( @@ -197,17 +202,23 @@ pub(crate) async fn unstable_resolve_sandbox_service_route( Ok(response) => return Ok(response), Err(error) => error, }; - let Some(owner) = - activation.filter(|_| !activated && activation_may_repair(ingress, &error)) + let Some(owner) = activation + .filter(|_| !activated && (restoring || activation_may_repair(ingress, &error))) else { return Err(error); }; - owner + // A restoring row waits on its restore here; if that fails, the + // caller still gets the retryable refusal rather than the waiter's + // error. + if let Err(prepare_error) = owner .prepare( sandbox_id, deadline.saturating_duration_since(tokio::time::Instant::now()), ) - .await?; + .await + { + return Err(if restoring { error } else { prepare_error }); + } activated = true; // Re-read identity and authorization after activation; never replay bytes. } diff --git a/crates/libs/internal/api/server/src/data_plane_sessions/session_route.rs b/crates/libs/internal/api/server/src/data_plane_sessions/session_route.rs index b624359bf..7a0d59ffe 100644 --- a/crates/libs/internal/api/server/src/data_plane_sessions/session_route.rs +++ b/crates/libs/internal/api/server/src/data_plane_sessions/session_route.rs @@ -78,7 +78,9 @@ pub(crate) async fn resolve_session_route( Ok(SessionKind::Port | SessionKind::Protocol) ); let sandbox_id = row.sandbox_id; + let restoring = row.sandbox_lifecycle == SandboxLifecycle::Restoring; let result = async { + refuse_restoring_route(row.sandbox_lifecycle)?; if matches!( row.kind.parse::(), Ok(SessionKind::Port | SessionKind::Protocol) @@ -207,17 +209,23 @@ pub(crate) async fn resolve_session_route( Ok(response) => return Ok(response), Err(error) => error, }; - let Some(owner) = - activation.filter(|_| !activated && activation_may_repair(ingress, &error)) + let Some(owner) = activation + .filter(|_| !activated && (restoring || activation_may_repair(ingress, &error))) else { return Err(error); }; - owner + // A restoring row waits on its restore here; if that fails, the + // caller still gets the retryable refusal rather than the waiter's + // error. + if let Err(prepare_error) = owner .prepare( sandbox_id, deadline.saturating_duration_since(tokio::time::Instant::now()), ) - .await?; + .await + { + return Err(if restoring { error } else { prepare_error }); + } activated = true; // Re-read identity and authorization after activation; never replay bytes. } diff --git a/crates/libs/internal/api/server/src/data_plane_sessions/tests.rs b/crates/libs/internal/api/server/src/data_plane_sessions/tests.rs index 15d391a57..f195e355e 100644 --- a/crates/libs/internal/api/server/src/data_plane_sessions/tests.rs +++ b/crates/libs/internal/api/server/src/data_plane_sessions/tests.rs @@ -170,3 +170,53 @@ async fn stalled_current_worker_leaves_time_for_delegated_recovery() { .await .unwrap(); } + +/// A session whose stored route is stale is re-pointed by `update_route_row`, +/// which re-reads the sandbox. A suspend and a restore can commit between the +/// resolver's first read, which saw the sandbox `started`, and that update; the +/// refreshed row then names the restoring replacement, and must be refused +/// retryably rather than signed and returned. +#[tokio::test(flavor = "current_thread")] +async fn a_route_refresh_refuses_a_sandbox_that_started_restoring() +-> pplx_space_test_env::TestResult { + let Some(db) = pplx_space_db_test_fixtures::try_pg_test_db().await? else { + return Ok(()); + }; + let pool = &db.pool; + let node_id = NodeId::new(); + let sandbox_id = SandboxId::new(); + let session_id = format!("term_{}", uuid::Uuid::now_v7()); + crate::tests::insert_data_plane_worker_node(pool, node_id, "10.0.0.45").await?; + crate::tests::insert_data_plane_active_sandbox(pool, sandbox_id, node_id).await?; + crate::tests::insert_active_execution(pool, sandbox_id, "10.80.0.45", 2).await?; + crate::tests::insert_data_plane_session(pool, &session_id, sandbox_id).await?; + let first = route_rows::fetch_route_row(pool, &session_id, None).await?; + assert_eq!(first.sandbox_lifecycle, SandboxLifecycle::Started); + // A same-node suspend and restore commit after that read. + sqlx::query("UPDATE sandboxes SET lifecycle = 'restoring' WHERE id = $1") + .bind(sandbox_id) + .execute(pool) + .await?; + + let refreshed = + route_rows::update_route_row(pool, &session_id, node_id, "10.0.0.45:50051", "grpc").await; + + let error = refreshed.expect_err("a restoring sandbox must not be routed to"); + assert_eq!(error.code(), tonic::Code::Unavailable, "{error:?}"); + assert!(error.message().contains("restoring"), "{error:?}"); + // The refused refresh changed no route. + let after = route_rows::fetch_route_row(pool, &session_id, None).await?; + assert_eq!( + ( + after.route_generation, + after.current_node_id, + after.backend_endpoint + ), + ( + first.route_generation, + first.current_node_id, + first.backend_endpoint + ) + ); + Ok(()) +} diff --git a/crates/libs/internal/api/server/src/tests.rs b/crates/libs/internal/api/server/src/tests.rs index 4e05a50a4..8891d0da4 100644 --- a/crates/libs/internal/api/server/src/tests.rs +++ b/crates/libs/internal/api/server/src/tests.rs @@ -3017,7 +3017,7 @@ fn route_credential_proof( } } -async fn insert_data_plane_worker_node( +pub(crate) async fn insert_data_plane_worker_node( pool: &sqlx::PgPool, node_id: NodeId, address: &str, @@ -3076,7 +3076,7 @@ async fn insert_data_plane_k8s_worker_node_without_address( Ok(()) } -async fn insert_data_plane_active_sandbox( +pub(crate) async fn insert_data_plane_active_sandbox( pool: &sqlx::PgPool, sandbox_id: SandboxId, node_id: NodeId, @@ -3116,7 +3116,7 @@ async fn mark_sandbox_command_ready_pending( Ok(()) } -async fn insert_active_execution( +pub(crate) async fn insert_active_execution( pool: &sqlx::PgPool, sandbox_id: SandboxId, pod_ip: &str, @@ -3200,7 +3200,7 @@ fn sha256_hex(value: &str) -> String { hex::encode(hasher.finalize()) } -async fn insert_data_plane_session( +pub(crate) async fn insert_data_plane_session( pool: &sqlx::PgPool, session_id: &str, sandbox_id: SandboxId, @@ -7263,6 +7263,7 @@ async fn a_missing_templates_table_degrades_end_to_end() -> TestResult { mod backup_generation_fence; mod publication; +mod restoring_routes; #[tokio::test(flavor = "current_thread")] async fn ecw_route_resolve_enforces_key_surface_and_organization() -> TestResult { diff --git a/crates/libs/internal/api/server/src/tests/restoring_routes.rs b/crates/libs/internal/api/server/src/tests/restoring_routes.rs new file mode 100644 index 000000000..4606c93be --- /dev/null +++ b/crates/libs/internal/api/server/src/tests/restoring_routes.rs @@ -0,0 +1,207 @@ +//! No data-plane route resolves to a sandbox that is `restoring`, on any +//! transport: the resolver waits on the restore through the activation owner +//! and resolves once the sandbox is `started`, and otherwise refuses with a +//! retryable `Unavailable`, never the backend endpoint of the restoring pod. + +use super::*; + +/// An activation owner whose restore completes while it is waited on. +struct CompletingActivation { + pool: sqlx::PgPool, + calls: std::sync::atomic::AtomicUsize, +} + +#[tonic::async_trait] +impl crate::ingress_activation::IngressActivation for CompletingActivation { + async fn prepare(&self, sandbox_id: SandboxId, _: std::time::Duration) -> Result<(), Status> { + self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + sqlx::query("UPDATE sandboxes SET lifecycle = 'started' WHERE id = $1") + .bind(sandbox_id) + .execute(&self.pool) + .await + .map_err(|error| Status::internal(error.to_string()))?; + Ok(()) + } +} + +#[derive(Debug, Clone, Copy)] +enum Owner { + /// No activation owner is configured. + Absent, + /// The owner can't finish the restore. + Unavailable, + /// The owner finishes the restore. + Completes, +} + +fn restoring_route_request( + sandbox_id: SandboxId, + api_key_hash: String, +) -> Request { + Request::new(UnstableResolveSandboxServiceRouteRequest { + sandbox_id: sandbox_id.to_string(), + port: 49_983, + route_kind: SandboxServiceRouteKind::Agent as i32, + transport: SandboxServiceRouteTransport::Direct as i32, + credential_proofs: vec![route_credential_proof( + SandboxServiceRouteCredentialKind::OrgApiKey, + api_key_hash, + )], + }) +} + +/// A direct (ECW envd) route to a restoring sandbox resolves only once its +/// restore completes, and is refused retryably when no owner can finish it. +#[tokio::test(flavor = "current_thread")] +async fn direct_route_to_a_restoring_sandbox_waits_for_its_restore() -> TestResult { + if !pplx_space_test_env::enter_test_process(&[ + ( + "SPACE_INTERNAL_RPC_INSECURE_DEV", + Some(std::ffi::OsStr::new("true")), + ), + ( + "SPACE_INTERNAL_TOKEN", + Some(std::ffi::OsStr::new("restoring-route-test-secret")), + ), + ]) { + return Ok(()); + } + + for owner in [Owner::Absent, Owner::Unavailable, Owner::Completes] { + let Some(mut fixture) = cluster_fixture().await? else { + return Ok(()); + }; + let pool = fixture.service.pool.clone(); + let completing = std::sync::Arc::new(CompletingActivation { + pool: pool.clone(), + calls: std::sync::atomic::AtomicUsize::new(0), + }); + let unavailable = std::sync::Arc::new(UnavailableIngressActivation( + std::sync::atomic::AtomicUsize::new(0), + )); + fixture.service.ingress_activation = match owner { + Owner::Absent => None, + Owner::Unavailable => Some(unavailable.clone()), + Owner::Completes => Some(completing.clone()), + }; + let node_id = NodeId::new(); + let sandbox_id = SandboxId::new(); + let api_key_hash = sha256_hex("restoring-route-api-key"); + insert_data_plane_worker_node(&pool, node_id, "10.0.0.43").await?; + let node_api = sim_worker_node_api(node_id).await?; + insert_data_plane_direct_node_endpoint(&pool, node_id, node_api.endpoint()).await?; + insert_data_plane_active_sandbox(&pool, sandbox_id, node_id).await?; + insert_active_execution(&pool, sandbox_id, "10.90.0.43", 1).await?; + insert_e2b_api_key(&pool, api_key_hash.as_str()).await?; + sqlx::query("UPDATE sandboxes SET lifecycle = 'restoring' WHERE id = $1") + .bind(sandbox_id) + .execute(&pool) + .await?; + + let resolved = fixture + .service + .unstable_resolve_sandbox_service_route(restoring_route_request( + sandbox_id, + api_key_hash, + )) + .await; + + match owner { + Owner::Absent | Owner::Unavailable => { + let error = resolved.expect_err("a restoring sandbox must not be routed to"); + assert_eq!(error.code(), Code::Unavailable, "{owner:?}: {error:?}"); + assert!( + error.message().contains("restoring"), + "{owner:?}: the refusal must be the restoring one, not the owner's: {error:?}" + ); + assert_eq!( + unavailable.0.load(std::sync::atomic::Ordering::SeqCst), + usize::from(matches!(owner, Owner::Unavailable)), + "{owner:?}: a configured owner is asked to finish the restore first" + ); + } + Owner::Completes => { + let route = resolved?.into_inner(); + assert_eq!(route.lifecycle, "started"); + assert_eq!( + completing.calls.load(std::sync::atomic::Ordering::SeqCst), + 1 + ); + } + } + } + Ok(()) +} + +/// A terminal session route to a restoring sandbox, which no ingress check +/// covers, resolves only once its restore completes, and is refused +/// retryably when no owner can finish it. +#[tokio::test(flavor = "current_thread")] +async fn session_route_to_a_restoring_sandbox_waits_for_its_restore() -> TestResult { + for owner in [Owner::Absent, Owner::Unavailable, Owner::Completes] { + let Some(mut fixture) = cluster_fixture().await? else { + return Ok(()); + }; + let pool = fixture.service.pool.clone(); + let completing = std::sync::Arc::new(CompletingActivation { + pool: pool.clone(), + calls: std::sync::atomic::AtomicUsize::new(0), + }); + let unavailable = std::sync::Arc::new(UnavailableIngressActivation( + std::sync::atomic::AtomicUsize::new(0), + )); + fixture.service.ingress_activation = match owner { + Owner::Absent => None, + Owner::Unavailable => Some(unavailable.clone()), + Owner::Completes => Some(completing.clone()), + }; + let node_id = NodeId::new(); + let sandbox_id = SandboxId::new(); + let session_id = format!("term_{}", Uuid::now_v7()); + insert_data_plane_worker_node(&pool, node_id, "10.0.0.44").await?; + insert_data_plane_active_sandbox(&pool, sandbox_id, node_id).await?; + insert_active_execution(&pool, sandbox_id, "10.80.0.44", 44).await?; + insert_data_plane_session(&pool, &session_id, sandbox_id).await?; + sqlx::query("UPDATE sandboxes SET lifecycle = 'restoring' WHERE id = $1") + .bind(sandbox_id) + .execute(&pool) + .await?; + + let resolved = fixture + .service + .resolve_data_plane_session_route(data_plane_request( + ResolveDataPlaneSessionRouteRequest { + session_id: session_id.clone(), + protocol_session_id: None, + token_sha256: None, + required_scope: None, + requested_transport: None, + }, + )) + .await; + + match owner { + Owner::Completes => { + assert_eq!(resolved?.into_inner().session_id, session_id); + assert_eq!( + completing.calls.load(std::sync::atomic::Ordering::SeqCst), + 1 + ); + } + Owner::Absent | Owner::Unavailable => { + let error = resolved.expect_err("a restoring sandbox must not be routed to"); + assert_eq!(error.code(), Code::Unavailable, "{owner:?}: {error:?}"); + assert!( + error.message().contains("restoring"), + "{owner:?}: {error:?}" + ); + assert_eq!( + unavailable.0.load(std::sync::atomic::Ordering::SeqCst), + usize::from(matches!(owner, Owner::Unavailable)), + "{owner:?}: a configured owner is asked to finish the restore first" + ); + } + } + } + Ok(()) +} diff --git a/crates/libs/metrics/src/sandbox/restore.rs b/crates/libs/metrics/src/sandbox/restore.rs index 35aad0e65..8630855ec 100644 --- a/crates/libs/metrics/src/sandbox/restore.rs +++ b/crates/libs/metrics/src/sandbox/restore.rs @@ -20,6 +20,11 @@ //! request emits a placement sample, so this counter is that request's only //! record of the re-drive. //! +//! [`claim_reverted`] fires once per failed fresh restore claim the API +//! rolls back toward `suspended`, tagged with what happened to the +//! execution the claim created. `execution_left_active` is the case that +//! still leaves an adoptable same-generation pod on a `suspended` row. +//! //! End-to-end restore duration stays on the existing //! `snapshot.restore.duration_ms` distribution (node side); this module owns //! only the placement decision and its re-drive. @@ -203,3 +208,44 @@ pub fn cross_node_redrive(path: RedrivePath, outcome: RedriveOutcome) { let tags = [tag("path", path.as_str()), tag("outcome", outcome.as_str())]; counter("sandbox.restore.redrive", 1, &tags); } + +/// What a failed fresh restore claim's rollback did. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum RevertOutcome { + /// Reverted to `suspended`, and the claim's execution was retired as + /// abandoned. + ExecutionRetired, + /// Reverted to `suspended` with no active execution left. + LifecycleOnly, + /// Reverted to `suspended` with an execution still active: the claim's + /// own, unproven, or one the claim reused rather than created. + ExecutionLeftActive, + /// The row had moved on from the claim; nothing was reverted. + NotReverted, +} + +impl RevertOutcome { + pub const fn as_str(self) -> &'static str { + match self { + Self::ExecutionRetired => "execution_retired", + Self::LifecycleOnly => "lifecycle_only", + Self::ExecutionLeftActive => "execution_left_active", + Self::NotReverted => "not_reverted", + } + } +} + +impl std::fmt::Display for RevertOutcome { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +/// Emit one failed fresh restore claim's rollback outcome. +pub fn claim_reverted(outcome: RevertOutcome) { + counter( + "sandbox.restore.claim_revert", + 1, + &[tag("outcome", outcome.as_str())], + ); +} diff --git a/docs/configuration.md b/docs/configuration.md index 751848261..5b62829db 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -50,6 +50,7 @@ because disabling it by mistake would leave every create queued forever. | `SPACE_TRANSIENT_LIFECYCLE_SWEEPER_TICK_SECS` | 60 | | | `SPACE_TRANSIENT_LIFECYCLE_SWEEPER_DEFAULT_AGE_SECS` | 600 | Age before a row in a transient lifecycle state is retired, unless a per-state age below applies. | | `SPACE_TRANSIENT_LIFECYCLE_SWEEPER_RESTORING_AGE_SECS`, `SPACE_TRANSIENT_LIFECYCLE_SWEEPER_SUSPENDING_AGE_SECS`, `SPACE_TRANSIENT_LIFECYCLE_SWEEPER_CHECKPOINTING_AGE_SECS` | default age | Per-state override; `0` disables that state. | +| `SPACE_ABANDONED_RESTORE_RETIREMENT_ENABLED` | off | API and cluster controller: whether a failed restore's rollback (API) and the transient-lifecycle sweeper's revert of an aged `restoring` row retire the abandoned restore's execution and delete its pod. Off, both revert only the lifecycle and the sweeper's cleanup refuses a restore pod that came up after its check. Turn it on only once every API and internal-API replica refuses to route to a `restoring` sandbox, and turn it off before rolling back to replicas that don't. `1`, `true`, `yes`, `on` or `enabled` turns it on; anything else keeps it off. | | `SPACE_STUCK_SNAPSHOT_REAPER_ENABLED` | off | | | `SPACE_SUSPENDED_EXECUTION_REAPER_ENABLED` | off | Runs dry by default. | | `SPACE_STUCK_CONTAINER_CREATING_GC_ENABLED` | off | | diff --git a/docs/space/diagrams/orphaned-restore-pod-gc.mmd b/docs/space/diagrams/orphaned-restore-pod-gc.mmd deleted file mode 100644 index d19c9752c..000000000 --- a/docs/space/diagrams/orphaned-restore-pod-gc.mmd +++ /dev/null @@ -1,51 +0,0 @@ -flowchart TD - classDef race fill:#fde2e4,stroke:#c0392b,color:#000 - classDef scan fill:#dbeafe,stroke:#1d4ed8,color:#000 - classDef skip fill:#e5e7eb,stroke:#374151,color:#000 - classDef reap fill:#dcfce7,stroke:#15803d,color:#000 - classDef gate fill:#fef3c7,stroke:#b45309,color:#000 - - subgraph race["The orphan"] - r1["Pinned restore pod
Unschedulable on source shortlist"]:::race - r2["Restore interrupted
(crash / abandoned request)"]:::race - r3["Row settles away from restoring;
execution retired or advanced"]:::race - r4["Source frees capacity:
stale pod binds → unrouted VM"]:::race - r1 --> r2 --> r3 --> r4 - end - - tick["Tick (every 60s, if ENABLED)"]:::scan - cache["Reflector cache:
restore-annotated pods only"]:::scan - filt{"terminating or
missing uid / sandbox id / generation?"}:::gate - classify["Batched Postgres classify
ANY($1): lifecycle + active generation
(excluding deleted tombstones)"]:::scan - - disp{"Supersession fence"}:::gate - skipT["Skip: row restoring, other transient,
quarantined, or unrecognized"]:::skip - skipA["Skip: pod gen ≥ active gen
(live runtime, re-drive adoptable,
same-gen orphan → suspend subsystem),
or no active execution outside suspended/failed"]:::skip - orphan["Orphan: row deleted,
suspended/failed with no active execution,
or pod gen < active gen"]:::reap - - clock{"continuously orphaned
≥ threshold (5m)?
(clock resets on cold cache / DB outage / restart)"}:::gate - budget{"rate limit + tick budget
(fairness cursor)"}:::gate - verify["Verify: cache-bypassing pod GET
(same UID, restore marker, sandbox, generation)
+ authoritative Postgres recheck"]:::scan - dry{"DRY_RUN?"}:::gate - would["Metric mode:dry_run + log, once per pod
no delete, no sandbox event"]:::skip - del["cleanup_requested → DELETE
preconditions: UID + resourceVersion
no DB lock held
→ requested_orphaned_restore_pod_delete"]:::reap - pend{"later ticks: pod still
in the cache?"}:::gate - done["gone: reaped_orphaned_restore_pod
+ cleanup_completed"]:::reap - stuck["terminating ≥ 5m and confirmed by a fresh GET:
teardown_stuck + cleanup_failed
(re-read until it settles)"]:::race - fail["error / timeout / 409:
cleanup_failed, retry next tick"]:::skip - - r4 -.-> cache - tick --> cache --> filt - filt -->|no| classify --> disp - disp --> skipT - disp --> skipA - disp --> orphan --> clock - clock -->|yes| budget --> verify - verify -->|still orphan| dry - verify -->|not orphan / gone / changed| stop["Skip this tick"]:::skip - dry -->|yes| would - dry -->|no| del - del -->|accepted| pend - del -->|not accepted| fail - pend -->|no| done - pend -->|yes, too long| stuck diff --git a/docs/space/diagrams/orphaned-restore-pod-gc.png b/docs/space/diagrams/orphaned-restore-pod-gc.png deleted file mode 100644 index 9d83ecf9d..000000000 Binary files a/docs/space/diagrams/orphaned-restore-pod-gc.png and /dev/null differ diff --git a/docs/space/diagrams/orphaned-restore-pod-gc.svg b/docs/space/diagrams/orphaned-restore-pod-gc.svg deleted file mode 100644 index 693f21636..000000000 --- a/docs/space/diagrams/orphaned-restore-pod-gc.svg +++ /dev/null @@ -1 +0,0 @@ -

The orphan

no

yes

still orphan

not orphan / gone / changed

yes

no

accepted

not accepted

no

yes, too long

Pinned restore pod
Unschedulable on source shortlist

Restore interrupted
(crash / abandoned request)

Row settles away from restoring;
execution retired or advanced

Source frees capacity:
stale pod binds → unrouted VM

Tick (every 60s, if ENABLED)

Reflector cache:
restore-annotated pods only

terminating or
missing uid / sandbox id / generation?

Batched Postgres classify
ANY($1): lifecycle + active generation
(excluding deleted tombstones)

Supersession fence

Skip: row restoring, other transient,
quarantined, or unrecognized

Skip: pod gen ≥ active gen
(live runtime, re-drive adoptable,
same-gen orphan → suspend subsystem),
or no active execution outside suspended/failed

Orphan: row deleted,
suspended/failed with no active execution,
or pod gen < active gen

continuously orphaned
≥ threshold (5m)?
(clock resets on cold cache / DB outage / restart)

rate limit + tick budget
(fairness cursor)

Verify: cache-bypassing pod GET
(same UID, restore marker, sandbox, generation)
+ authoritative Postgres recheck

DRY_RUN?

Metric mode:dry_run + log, once per pod
no delete, no sandbox event

cleanup_requested → DELETE
preconditions: UID + resourceVersion
no DB lock held
→ requested_orphaned_restore_pod_delete

later ticks: pod still
in the cache?

gone: reaped_orphaned_restore_pod
+ cleanup_completed

terminating ≥ 5m and confirmed by a fresh GET:
teardown_stuck + cleanup_failed
(re-read until it settles)

error / timeout / 409:
cleanup_failed, retry next tick

Skip this tick

 \ No newline at end of file