diff --git a/architecture-baseline.toml b/architecture-baseline.toml index 6325c7c3a6..77de4b6969 100644 --- a/architecture-baseline.toml +++ b/architecture-baseline.toml @@ -82,7 +82,7 @@ "crates/libs/sbx/mgr/server/src/run.rs" = 1101 "crates/libs/sbx/mgr/server/src/sandbox_eviction.rs" = 1273 "crates/libs/sbx/mgr/server/src/service/activation_support.rs" = 1592 -"crates/libs/sbx/mgr/server/src/service/auth_lookup.rs" = 1237 +"crates/libs/sbx/mgr/server/src/service/auth_lookup.rs" = 1221 "crates/libs/sbx/mgr/server/src/service/cache_cleanup.rs" = 1455 "crates/libs/sbx/mgr/server/src/service/cleanup.rs" = 2704 "crates/libs/sbx/mgr/server/src/service/create.rs" = 1195 diff --git a/crates/libs/sbx/mgr/server/src/db/state_write.rs b/crates/libs/sbx/mgr/server/src/db/state_write.rs index 628812854b..873cb2f7b7 100644 --- a/crates/libs/sbx/mgr/server/src/db/state_write.rs +++ b/crates/libs/sbx/mgr/server/src/db/state_write.rs @@ -240,8 +240,8 @@ edge_table! { /// and the suspend auto-pause; `update_state_best_effort(Started)` after a /// node connect failure and in `pause_with_reconciliation`. PAUSE_ROLLBACK: Compensation, of(&[Pausing]) => Started; - /// `begin_resident_resume_with_policy`, and `resume_after_wake_lock`'s - /// `update_state(Resuming)` without an activation policy. + /// `begin_resident_resume_with_policy`, and `begin_resident_resume`'s + /// `update_state_from` without an activation policy. RESUME_BEGIN: Request, of(&[Paused]) => Resuming; /// `restore_resident_resume_state(Started)` through /// `rollback_activation_state`. @@ -429,9 +429,6 @@ edge_table! { PAUSE_ROLLBACK_AFTER_STOP: KnownRace, of(&[Stopped]) => Started; /// Pause completion (`update_state(Paused)`) after a concurrent Stop. PAUSE_COMPLETE_AFTER_STOP: KnownRace, of(&[Stopped]) => Paused; - /// `resume_after_wake_lock`'s policy-free `update_state(Resuming)` after - /// a Stop completed during the pre-wake policy refresh. - RESUME_BEGIN_AFTER_STOP: KnownRace, of(&[Stopped]) => Resuming; /// The full process-snapshot rollback's `update_state(Suspended)` after a /// Stop admitted while it restaged the snapshot. Its own stop leaves no /// runtime, so the second Stop is reached only in the window between diff --git a/crates/libs/sbx/mgr/server/src/db/tests/state_write_table.rs b/crates/libs/sbx/mgr/server/src/db/tests/state_write_table.rs index 07908797fd..4ca292bd96 100644 --- a/crates/libs/sbx/mgr/server/src/db/tests/state_write_table.rs +++ b/crates/libs/sbx/mgr/server/src/db/tests/state_write_table.rs @@ -68,7 +68,6 @@ const ALLOWLIST: &[(SandboxState, SandboxState, Disagreement, &str)] = &[ (Stopped, Started, WriteForbidden, "known race: pause rollback after Stop"), (Stopped, Stopping, RequestForbidden, STOP_SETTLED_SOURCE), (Stopped, Paused, WriteForbidden, "known races: pause and template-snapshot completion after Stop"), - (Stopped, Resuming, WriteForbidden, "known race: policy-free resume after Stop"), (Stopped, Suspended, RequestForbidden, "warm start and process-snapshot rollback restage a stopped row"), (Stopped, Quarantined, AllowedUnwritten, NO_QUARANTINE_WRITER), (Deleting, CleanupRequired, WriteForbidden, "an interrupted delete is handed to cleanup"), diff --git a/crates/libs/sbx/mgr/server/src/service/auth_lookup.rs b/crates/libs/sbx/mgr/server/src/service/auth_lookup.rs index 6c43e9c6a1..ba087c39a8 100644 --- a/crates/libs/sbx/mgr/server/src/service/auth_lookup.rs +++ b/crates/libs/sbx/mgr/server/src/service/auth_lookup.rs @@ -2,7 +2,6 @@ use super::activation_support::{ GuestActivityKind, IdentityFreeDirectActivationAuthority, configure_process_env_for_activation, spawn_resume_clock_sync, wait_ready_guest_uptime_ms, wait_ready_via_spaced_host, }; -use super::cleanup_support::require_state_write; use super::create_support::*; use super::network_policy::EffectiveNetworkPolicy; use super::network_runtime::{network_policy_refresh_error_to_status, refresh_network_policy}; @@ -26,6 +25,7 @@ use pplx_space_metrics::sandbox::worker::SpaceApiRouteTokenAuthorizationResult; use pplx_space_net_common::{EffectiveNetworkPolicyRef, NetworkPolicy}; use tonic::metadata::MetadataMap; +mod resume_begin; #[cfg(test)] mod tests; @@ -790,30 +790,8 @@ impl SandboxManagerService { // policy is unchanged. self.apply_network_policy_before_wake(sandbox_id, "resume") .await?; - - { - let db = self.db_conn()?; - if let Some((policy, generation)) = activation_policy { - if !crate::db::begin_resident_resume_with_policy( - &db, - &sandbox.id, - generation, - policy, - ) - .map_err(|error| { - Status::internal(format!("resume policy admission failed: {error}")) - })? { - return Err(Status::failed_precondition( - "resident execution changed before resume", - )); - } - } else { - require_state_write( - crate::db::update_state(&db, &sandbox.id, SandboxState::Resuming), - "begin resident resume", - )?; - } - } + self.begin_resident_resume(&sandbox.id, activation_policy) + .await?; self.watchdog.record_lifecycle_edge(&sandbox.id).await; let mut node_client = connect_node(&self.node_manager) diff --git a/crates/libs/sbx/mgr/server/src/service/auth_lookup/resume_begin.rs b/crates/libs/sbx/mgr/server/src/service/auth_lookup/resume_begin.rs new file mode 100644 index 0000000000..9519d870c9 --- /dev/null +++ b/crates/libs/sbx/mgr/server/src/service/auth_lookup/resume_begin.rs @@ -0,0 +1,55 @@ +//! The write that starts a resident resume: `paused → resuming` in the +//! node-local SQLite `sandboxes.state`. + +use super::super::cleanup_support::require_state_write_from; +use super::*; + +#[cfg(test)] +mod tests; + +#[cfg(test)] +tokio::task_local! { + /// Holds a resume between its pre-wake policy gate and this write, so a + /// test can move the row in that window. + static BEGIN_BARRIER: Arc; +} + +impl SandboxManagerService { + /// Persist `resuming` for the `paused` row `resume_after_wake_lock` + /// validated before its pre-wake policy gate. + /// + /// Stop takes neither the wake nor the artifact lock, and its runtime gate + /// admits the paused VM because its `fc.pid` resolves, so Stop can settle + /// `stopped` during the gate. Each branch + /// therefore writes only over `paused`, and a resume that lost the race + /// fails here, before any node-manager call: with an activation policy as + /// `FailedPrecondition`, without one as `Aborted`. + pub(super) async fn begin_resident_resume( + &self, + sandbox_id: &str, + activation_policy: Option<(&AutoLifecyclePolicy, Option)>, + ) -> Result<(), Status> { + #[cfg(test)] + if let Ok(barrier) = BEGIN_BARRIER.try_with(Arc::clone) { + barrier.entered.notify_one(); + let _permit = barrier.release.acquire().await.unwrap(); + } + let db = self.db_conn()?; + if let Some((policy, generation)) = activation_policy { + if !crate::db::begin_resident_resume_with_policy(&db, sandbox_id, generation, policy) + .map_err(|error| { + Status::internal(format!("resume policy admission failed: {error}")) + })? + { + return Err(Status::failed_precondition( + "resident execution changed before resume", + )); + } + return Ok(()); + } + require_state_write_from( + crate::db::update_state_from(&db, sandbox_id, &crate::db::edges::RESUME_BEGIN), + "begin resident resume", + ) + } +} diff --git a/crates/libs/sbx/mgr/server/src/service/auth_lookup/resume_begin/tests.rs b/crates/libs/sbx/mgr/server/src/service/auth_lookup/resume_begin/tests.rs new file mode 100644 index 0000000000..6f61633fdb --- /dev/null +++ b/crates/libs/sbx/mgr/server/src/service/auth_lookup/resume_begin/tests.rs @@ -0,0 +1,290 @@ +//! A Resume without an activation policy whose node-local SQLite +//! `sandboxes.state` row leaves `paused` while the resume is in its pre-wake +//! policy gate, or is deleted then. +//! +//! The row is a warm-pool member, so the gate has no net-gateway to confirm +//! and returns at once; a test barrier then holds the resume before its +//! `resuming` write while the test moves the row. Node-manager is a fake that +//! answers only `ResumeSandbox`, with the error a VM that is gone gets. + +use std::collections::BTreeMap; +use std::sync::Arc; +use std::time::Duration; + +use pplx_space_common_core::types::{ + AutoLifecyclePolicy, ExecutionGeneration, RouteGeneration, SandboxId, SandboxOwner, + SandboxRuntimeLabel, SpaceApiRouteIdentity, +}; +use pplx_space_node_mgr_client::proto::{ResumeSandboxRequest, ResumeSandboxResponse}; +use tonic::{Code, Status}; + +use super::BEGIN_BARRIER; +use crate::db::CleanupCompletion; +use crate::db::tests::state_write_fixtures::snapshot; +use crate::node_manager_transport::fake::FakeNodeManager; +use crate::proto::ResumeRequest; +use crate::service::activation_support::internal_request; +use crate::service::cleanup_executor::TestAdmissionBarrier; +use crate::service::{SandboxManagerService, SandboxManagerServiceOptions}; +use crate::state::SandboxState; + +const WAIT: Duration = Duration::from_secs(30); + +/// What the resume's write finds. +#[derive(Clone, Copy, Debug)] +enum Found { + State(SandboxState), + /// The row was deleted. + Missing, +} + +/// What the resume answers. +#[derive(Debug)] +enum Expected { + /// The write applies, and the resume asks node-manager to thaw the VM. + Resumes, + /// `Aborted` with this message, every row unchanged and no node-manager + /// call. + Aborted(String), +} + +/// Wildcard-free, so a new lifecycle state does not compile until it is +/// placed. +fn expected(found: Found) -> Expected { + let not_applied = |detail: &str| { + Expected::Aborted(format!( + "begin resident resume: sandbox state write was not applied: {detail}" + )) + }; + let state = match found { + // The message a resume reported before it had a from-set. + Found::Missing => return not_applied("missing"), + Found::State(state) => state, + }; + match state { + SandboxState::Paused => Expected::Resumes, + // The generic guards refuse these before the from-set is consulted, + // with the messages a resume reported before it had a from-set. + SandboxState::Stopping + | SandboxState::Deleting + | SandboxState::CleanupRequired + | SandboxState::CleanupFailed => not_applied("superseded_by_cleanup_episode"), + SandboxState::Quarantined => not_applied("blocked_by_quarantine"), + // The generic guards admit these; only the from-set refuses them. + SandboxState::Creating + | SandboxState::Created + | SandboxState::Starting + | SandboxState::Started + | SandboxState::Stopped + | SandboxState::Pausing + | SandboxState::Resuming + | SandboxState::Checkpointing + | SandboxState::Suspending + | SandboxState::Suspended + | SandboxState::Restoring + | SandboxState::Failed => Expected::Aborted(format!( + "begin resident resume: sandbox state write was superseded: sandbox is {state}" + )), + } +} + +/// A paused warm-pool member whose runtime columns are populated: the test +/// process's pid, route generation 3 and execution generation 7. Nothing in +/// the resume path probes them before its write. +fn insert_paused_pool_member(conn: &rusqlite::Connection, sandbox_id: SandboxId, scratch: &str) { + let id = sandbox_id.to_string(); + crate::db::insert_sandbox( + conn, + crate::db::InsertSandboxParams { + id: &id, + uuid8: &SandboxRuntimeLabel::from_sandbox_id(sandbox_id), + state: SandboxState::Paused, + template: "seed", + template_hash: Some("hash"), + user: &SandboxOwner::parse("user").unwrap(), + runtime: "fc", + snapshots_enabled: false, + rootfs_size_mb: None, + disk_io_limits: Default::default(), + pool_template: Some("seed-pool"), + volume_encryption: false, + process_env: &BTreeMap::new(), + auto_lifecycle: &AutoLifecyclePolicy::disabled(), + network_policy_override: None, + network_policy_generation: 0, + network_policy_authority: crate::db::PolicyAuthority::Cluster, + auto_lifecycle_authority: crate::db::PolicyAuthority::Cluster, + }, + ) + .unwrap(); + crate::db::update_runtime_fields( + conn, + sandbox_id, + crate::db::RuntimeFields { + uid: 10_000, + slot: 3, + guest_cid: 33, + pid: std::process::id(), + spaced_host_pid: 10_001, + netgtw_pid: 10_002, + vsock_uds_path: &format!("{scratch}/v.sock"), + api_socket_path: &format!("{scratch}/api.sock"), + spaced_endpoint_proof: None, + space_api_route_identity: SpaceApiRouteIdentity { + route_generation: Some(RouteGeneration::new(3).unwrap()), + execution_generation: Some(ExecutionGeneration::new(7).unwrap()), + ..SpaceApiRouteIdentity::default() + }, + route_identity_provenance: + crate::auto_lifecycle_delete_receipts::RouteIdentityProvenance::Unfenced, + }, + ) + .unwrap(); +} + +/// Move the paused row to what the write should find. `stopped` is reached by +/// the writers Stop runs, which take neither the wake nor the artifact lock: a +/// non-blocking Stop's admission of the paused VM, then its completion. Every +/// other state is written directly, and `Missing` deletes the row. +fn move_row(conn: &rusqlite::Connection, id: &str, found: Found) { + let state = match found { + Found::Missing => { + crate::db::delete_sandbox(conn, id).unwrap(); + assert!(crate::db::get_sandbox(conn, id).unwrap().is_none()); + return; + } + Found::State(state) => state, + }; + match state { + SandboxState::Paused => {} + SandboxState::Stopped => { + let tx = crate::db::immediate_transaction(conn).unwrap(); + crate::db::update_state_superseding_activation_in_transaction( + &tx, + id, + SandboxState::Stopping, + ) + .unwrap(); + assert!( + crate::db::set_cleanup_intent(&tx, id, true, CleanupCompletion::MarkStopped) + .unwrap() + ); + tx.commit().unwrap(); + crate::db::complete_row_intent_cleanup_for_job( + conn, + id, + CleanupCompletion::MarkStopped, + None, + None, + None, + ) + .unwrap(); + } + state => { + // The schema ties the restore columns to `restoring`. + conn.execute( + "UPDATE sandboxes + SET state = ?1, + restore_origin = CASE WHEN ?1 = 'restoring' THEN 'local' END, + restore_phase = CASE WHEN ?1 = 'restoring' THEN 'activation' END + WHERE id = ?2", + rusqlite::params![state.as_str(), id], + ) + .unwrap(); + } + } + let row = crate::db::get_sandbox(conn, id).unwrap().unwrap(); + assert_eq!(row.state, state, "{state}: the row moved"); +} + +/// Every case runs before the assertion, so a failure lists each case that +/// answered differently. +#[tokio::test] +async fn policy_free_resume_writes_resuming_only_over_paused() { + let cases = SandboxState::all() + .into_iter() + .map(Found::State) + .chain([Found::Missing]); + let mut mismatches = Vec::new(); + for found in cases { + let scratch = tempfile::tempdir().unwrap(); + let node_manager = FakeNodeManager::default() + .unary( + "ResumeSandbox", + |_: ResumeSandboxRequest| -> Result { + Err(Status::not_found("no Firecracker process for the sandbox")) + }, + ) + .serve(); + let conn = crate::db::test_conn(); + let sandbox_id = SandboxId::new(); + let id = sandbox_id.to_string(); + insert_paused_pool_member(&conn, sandbox_id, &scratch.path().to_string_lossy()); + let service = SandboxManagerService::new_with_options( + conn, + SandboxManagerServiceOptions { + node_manager: node_manager.connector(), + ..SandboxManagerServiceOptions::for_test_in(scratch.path()) + }, + ); + let barrier = Arc::new(TestAdmissionBarrier { + entered: tokio::sync::Notify::new(), + release: tokio::sync::Semaphore::new(0), + }); + let resume = tokio::spawn(BEGIN_BARRIER.scope(Arc::clone(&barrier), { + let service = service.clone(); + let id = id.clone(); + async move { + service + .resume(internal_request(ResumeRequest { + sandbox_id: id, + ..Default::default() + })) + .await + .map(|_| ()) + } + })); + tokio::time::timeout(WAIT, barrier.entered.notified()) + .await + .expect("the resume reaches its write within 30 seconds"); + + let before = { + let db = service.db_conn().unwrap(); + move_row(&db, &id, found); + snapshot(&db) + }; + barrier.release.add_permits(1); + let result = tokio::time::timeout(WAIT, resume) + .await + .expect("the resume finishes within 30 seconds") + .unwrap() + .map_err(|status| (status.code(), status.message().to_owned())); + let calls = node_manager.calls(); + + let expected = expected(found); + let matched = match &expected { + Expected::Resumes => { + matches!(result, Err((Code::Internal, _))) + && calls.iter().any(|call| call == "ResumeSandbox") + } + Expected::Aborted(message) => { + result == Err((Code::Aborted, message.clone())) + && snapshot(&service.db_conn().unwrap()) == before + && calls.is_empty() + } + }; + if !matched { + mismatches.push(format!( + "{found:?}: expected {expected:?}; got {result:?}, node-manager calls \ + {calls:?}, rows unchanged: {}", + snapshot(&service.db_conn().unwrap()) == before + )); + } + service.shutdown_token.cancel(); + service.task_tracker.close(); + tokio::time::timeout(WAIT, service.task_tracker.wait()) + .await + .expect("service tasks stop within 30 seconds of shutdown"); + } + assert!(mismatches.is_empty(), "{mismatches:#?}"); +}