From 6a1c00f38587a51dc19a741f21e673b129310181 Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Tue, 13 Oct 2026 22:04:15 +0000 Subject: [PATCH] sbx-mgr: create completion and warm start write only over the state they expect Convert three generic update_state callers of the node-local SQLite sandboxes.state column to update_state_from with an expected from-set, and map the outcomes with require_state_write_from and state_write_result. This does not touch the control-plane Postgres public.sandboxes.lifecycle. - Create completion (create_with_id): creating -> created through CREATE_COMPLETE, and, in base-snapshot mode, created -> suspended through CREATE_BASE_SNAPSHOT_COMPLETE (write_create_completed, write_base_snapshot_create_completed). Create holds no lock between its insert and these writes. A row another transition moved, such as a Stop's stopping, is now refused with Aborted ": sandbox state write was superseded: sandbox is ", and Create fails through its cleanup guard while the Stop keeps the row and settles it stopped. Before, the write overwrote stopping and left a created or suspended row with a stale mark_stopped intent that refuses every activation. The Stop RPC cannot reach a row during create (no runtime), so this is defense in depth. - Warm-snapshot Start (try_start_from_warm_snapshot): - admit_warm_start refuses a row read outside created, stopped and suspended with FailedPrecondition before the base snapshot is staged into the sandbox's suspend snapshot directory. Before, such a row was staged and rewritten to suspended for a checkpoint restore, and started and paused need no concurrency to reach that. - The restage write, created or stopped -> suspended through WARM_START_RESTAGE (write_warm_start_restaged), checks again. A row that moved to suspended needs no write; a row moved anywhere else is the same FailedPrecondition. Generic-guard refusals and a missing row keep require_state_write_from's Aborted. Guard refusals and missing rows keep update_state's Aborted messages. Caller mutex and lifecycle-lock scopes and each writer's side columns are unchanged; in autocommit update_state_from adds its own IMMEDIATE transaction around its reads and the UPDATE, which update_state did not (harmless on the single connection). The three converted sites no longer log update_state's "ignored generic sandbox state update" warning; the refusal reaches the client status instead. The WARM_START_UNVALIDATED known-race edge is deleted. The parity allowlist drops the five pairs only it wrote (started, pausing, paused, resuming, checkpointing -> suspended) and the warm-start mention from six mixed reasons. The edge doc comments name the new writer functions. The state_write_fixtures helpers the tests reuse become crate-visible, and require_state_write_from, StateWriteRejected, StateWriteGuard and state_write_result lose their dead-code expectation. Tests: - service/create_support/state_write_tests.rs (DB level, because both paths create directories under the fixed host cache before these writes): - a Stop admitted before either create completion write: the write is refused with Aborted and changes nothing, Create's cleanup request is superseded, and Stop completion settles stopped; - each of the three writers over a plain row in every SandboxState, carrying a failure reason, a receipt and backdated timestamps, with wildcard-free expected outcomes and accepting controls; an already-suspended warm-start row stays byte-identical; - warm-start rows a concurrent activation or cleanup episode owns keep the guard's Aborted, including a suspended one; - admit_warm_start over a row in every SandboxState accepts exactly created, stopped and suspended and refuses the rest with the exact FailedPrecondition. - start/tests.rs: a warm Start of a started or paused row is refused before staging and leaves the row unchanged. The Stop-race test and the source-state sweep fail with a writer's generic update_state call restored or its edge's from-set widened to every state. The guard-refusal test fails only when the warm start mishandles a guard refusal: the guards answer before the from-set. The start/tests.rs cases fail without the pre-staging check, and the admission sweep fails when any of created, stopped or suspended is dropped from it. --- .../libs/sbx/mgr/server/src/db/state_write.rs | 23 +- .../src/db/tests/state_write_fixtures.rs | 12 +- .../server/src/db/tests/state_write_table.rs | 17 +- .../mgr/server/src/service/cleanup_support.rs | 20 - .../libs/sbx/mgr/server/src/service/create.rs | 14 +- .../mgr/server/src/service/create_support.rs | 73 ++++ .../create_support/state_write_tests.rs | 404 ++++++++++++++++++ .../libs/sbx/mgr/server/src/service/start.rs | 14 +- .../sbx/mgr/server/src/service/start/tests.rs | 18 + 9 files changed, 522 insertions(+), 73 deletions(-) create mode 100644 crates/libs/sbx/mgr/server/src/service/create_support/state_write_tests.rs 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 debafe6a2f..628812854b 100644 --- a/crates/libs/sbx/mgr/server/src/db/state_write.rs +++ b/crates/libs/sbx/mgr/server/src/db/state_write.rs @@ -181,10 +181,10 @@ edge_table! { /// pool clone) and `transfer::reserve_import` insert a `creating` row with /// an armed cleanup episode. CREATE_INSERT: Request, INSERT => Creating; - /// `create_with_id`: `update_state(Created)`. + /// `create_with_id` through `write_create_completed`. CREATE_COMPLETE: Completion, of(&[Creating]) => Created; - /// `create_with_id` in base-snapshot mode: `update_state(Suspended)` - /// after its own `created` write. + /// `create_with_id` in base-snapshot mode, through + /// `write_base_snapshot_create_completed` after its own `created` write. CREATE_BASE_SNAPSHOT_COMPLETE: Completion, of(&[Created]) => Suspended; /// `complete_pool_clone` and `complete_import`, under their armed /// episodes. @@ -200,8 +200,8 @@ edge_table! { /// `begin_activation_with_route_evidence(Restore)` from /// `start_from_checkpoint`. RESTORE_BEGIN: Request, of(&[Suspended]) => Restoring; - /// `try_start_from_warm_snapshot`: `update_state(Suspended)` when the row - /// is not already `suspended`, before the cold path. + /// `try_start_from_warm_snapshot` through `write_warm_start_restaged` + /// when the row is not already `suspended`, before the cold path. WARM_START_RESTAGE: Request, of(&[Created, Stopped]) => Suspended; /// `insert_restoring_sandbox_row_from_snapshot` through /// `restore_adopt_orphan_or_insert_sandbox_in_transaction`: a @@ -432,19 +432,6 @@ edge_table! { /// `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; - /// `try_start_from_warm_snapshot`'s `update_state(Suspended)`. An - /// empty-checkpoint warm Start rejects only `failed` before it, so it - /// writes from every state the generic guards admit, with no concurrency - /// needed; a row with a current activation episode answers - /// `CleanupOwned` instead. - WARM_START_UNVALIDATED: KnownRace, - StateSet::ALL - .without(Created) - .without(Stopped) - .without(Suspended) - .without(Deleting) - .without(Quarantined) - .without(Failed) => Suspended; /// 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_fixtures.rs b/crates/libs/sbx/mgr/server/src/db/tests/state_write_fixtures.rs index 4beb232dc6..e4653bdab4 100644 --- a/crates/libs/sbx/mgr/server/src/db/tests/state_write_fixtures.rs +++ b/crates/libs/sbx/mgr/server/src/db/tests/state_write_fixtures.rs @@ -356,7 +356,7 @@ pub(crate) fn assert_unchanged(case: &str, before: &Snapshot, conn: &Connection) } /// Insert a plain sandbox row in `state`. -pub(super) fn insert_row(conn: &Connection, state: SandboxState) -> String { +pub(crate) fn insert_row(conn: &Connection, state: SandboxState) -> String { let sandbox_id = SandboxId::new(); let id = sandbox_id.to_string(); insert_test_sandbox( @@ -373,7 +373,7 @@ pub(super) fn insert_row(conn: &Connection, state: SandboxState) -> String { /// Move a seeded row to `state` without any writer's side effects. A /// `restoring` row carries a remote restore's three restore columns, which the /// table's `CHECK` ties to that state. -pub(super) fn force_state(conn: &Connection, id: &str, state: SandboxState) { +pub(crate) fn force_state(conn: &Connection, id: &str, state: SandboxState) { let restoring = state == SandboxState::Restoring; conn.execute( "UPDATE sandboxes @@ -395,7 +395,7 @@ pub(super) fn restore_source() -> pplx_space_common_core::types::ExactStorageUri } /// Backdate every timestamp a writer may stamp, so a fresh stamp is visible. -pub(super) fn backdate(conn: &Connection, id: &str) { +pub(crate) fn backdate(conn: &Connection, id: &str) { conn.execute( "UPDATE sandboxes SET state_changed_at = '1', updated_at = '1' WHERE id = ?1", params![id], @@ -488,7 +488,7 @@ pub(super) fn runtime_cleared(retained: &[&str]) -> Vec<(&'static str, Cell)> { /// A transition receipt targeting `target`, in the kind the table's `CHECK` /// admits for it: an activation completion for created, started and paused, /// otherwise an activation rollback. -pub(super) fn seed_receipt(conn: &Connection, id: &str, target: SandboxState) { +pub(crate) fn seed_receipt(conn: &Connection, id: &str, target: SandboxState) { let (kind, reason) = match target { SandboxState::Created | SandboxState::Started | SandboxState::Paused => { ("activation_completed", None) @@ -528,7 +528,7 @@ pub(super) fn receipt_cleared(target: SandboxState) -> Vec<(&'static str, Cell)> cells } -pub(super) fn set_failure_reason(conn: &Connection, id: &str, reason: &str) { +pub(crate) fn set_failure_reason(conn: &Connection, id: &str, reason: &str) { conn.execute( "UPDATE sandboxes SET failure_reason = ?2 WHERE id = ?1", params![id, reason], @@ -744,7 +744,7 @@ pub(super) fn insert_creating_with( (id, cleanup_id) } -pub(super) fn insert_creating( +pub(crate) fn insert_creating( conn: &Connection, completion: CleanupCompletion, ) -> (String, SandboxCleanupEpisodeId) { 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 70c85aaf4c..07908797fd 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 @@ -44,7 +44,7 @@ const ALLOWLIST: &[(SandboxState, SandboxState, Disagreement, &str)] = &[ (Creating, Created, AllowedNotRequested, COMPLETION_ONLY), (Creating, Stopped, WriteForbidden, "import completion of a stopped export"), (Creating, Deleting, RequestForbidden, "Delete admission supersedes the create episode"), - (Creating, Suspended, WriteForbidden, "pool clone and import completion; known race: unvalidated warm start"), + (Creating, Suspended, WriteForbidden, "pool clone and import completion"), (Creating, CleanupRequired, WriteForbidden, "an interrupted create is handed to cleanup"), (Creating, Quarantined, AllowedUnwritten, NO_QUARANTINE_WRITER), (Created, Stopping, RequestForbidden, STOP_SETTLED_SOURCE), @@ -55,14 +55,13 @@ const ALLOWLIST: &[(SandboxState, SandboxState, Disagreement, &str)] = &[ (Starting, Started, AllowedNotRequested, COMPLETION_ONLY), (Starting, Stopping, RequestForbidden, STOP_LIVE_RUNTIME), (Starting, Stopped, WriteForbidden, "cold-start rollback and startup recovery restore the entry state"), - (Starting, Suspended, WriteForbidden, "cold-start rollback restores a suspended entry state; known race: warm start"), + (Starting, Suspended, WriteForbidden, "cold-start rollback restores a suspended entry state"), (Starting, CleanupRequired, WriteForbidden, "an interrupted cold start is handed to cleanup"), (Starting, Quarantined, AllowedUnwritten, NO_QUARANTINE_WRITER), (Started, Suspending, AllowedUnwritten, "suspend pauses a started row first"), - (Started, Suspended, WriteForbidden, "known race: unvalidated warm start"), (Started, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), (Stopping, Stopped, AllowedNotRequested, COMPLETION_ONLY), - (Stopping, Suspended, WriteForbidden, "known races: unvalidated warm start; process-snapshot rollback in the gate-to-fence window"), + (Stopping, Suspended, WriteForbidden, "known race: process-snapshot rollback in the gate-to-fence window"), (Stopping, CleanupRequired, WriteForbidden, "an interrupted stop is handed to cleanup"), (Stopping, CleanupFailed, WriteForbidden, "stop cleanup failed"), (Stopping, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), @@ -78,19 +77,15 @@ const ALLOWLIST: &[(SandboxState, SandboxState, Disagreement, &str)] = &[ (Pausing, Started, WriteForbidden, "pause rollback and startup pause recovery"), (Pausing, Stopping, RequestForbidden, STOP_LIVE_RUNTIME), (Pausing, Paused, AllowedNotRequested, COMPLETION_ONLY), - (Pausing, Suspended, WriteForbidden, "known race: unvalidated warm start"), (Pausing, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), - (Paused, Suspended, WriteForbidden, "known race: unvalidated warm start"), (Paused, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), (Resuming, Started, AllowedNotRequested, COMPLETION_ONLY), (Resuming, Stopping, RequestForbidden, STOP_LIVE_RUNTIME), (Resuming, Paused, WriteForbidden, "resume rollback and startup resident recovery"), - (Resuming, Suspended, WriteForbidden, "known race: unvalidated warm start"), (Resuming, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), (Checkpointing, Started, AllowedNotRequested, COMPLETION_ONLY), (Checkpointing, Stopping, RequestForbidden, STOP_LIVE_RUNTIME), (Checkpointing, Paused, AllowedNotRequested, COMPLETION_ONLY), - (Checkpointing, Suspended, WriteForbidden, "known race: unvalidated warm start"), (Checkpointing, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), (Suspending, Stopping, RequestForbidden, STOP_LIVE_RUNTIME), (Suspending, Paused, WriteForbidden, "suspend rollback and startup suspend recovery"), @@ -106,19 +101,19 @@ const ALLOWLIST: &[(SandboxState, SandboxState, Disagreement, &str)] = &[ (Restoring, Stopping, RequestForbidden, STOP_LIVE_RUNTIME), (Restoring, Stopped, WriteForbidden, "restore-as-new publishes a stopped disk"), (Restoring, Paused, AllowedNotRequested, COMPLETION_ONLY), - (Restoring, Suspended, WriteForbidden, "restore rollback, cross-node publication, restore recovery; known race: warm start"), + (Restoring, Suspended, WriteForbidden, "restore rollback, cross-node publication, restore recovery"), (Restoring, CleanupRequired, WriteForbidden, "an interrupted or ambiguous restore is handed to cleanup"), (Restoring, Quarantined, AllowedUnwritten, NO_QUARANTINE_WRITER), (CleanupRequired, Created, WriteForbidden, CLEANUP_SETTLES_INTENT), (CleanupRequired, Stopping, RequestForbidden, STOP_RUNTIME_OWNER), (CleanupRequired, Stopped, WriteForbidden, CLEANUP_SETTLES_INTENT), - (CleanupRequired, Suspended, WriteForbidden, "cleanup completion settles a mark_suspended intent; known race: warm start"), + (CleanupRequired, Suspended, WriteForbidden, "cleanup completion settles a mark_suspended intent"), (CleanupRequired, CleanupFailed, AllowedNotRequested, "only cleanup records its own failure"), (CleanupRequired, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), (CleanupFailed, Created, WriteForbidden, CLEANUP_SETTLES_INTENT), (CleanupFailed, Stopping, RequestForbidden, STOP_RUNTIME_OWNER), (CleanupFailed, Stopped, WriteForbidden, CLEANUP_SETTLES_INTENT), - (CleanupFailed, Suspended, WriteForbidden, "cleanup completion settles a mark_suspended intent; known race: warm start"), + (CleanupFailed, Suspended, WriteForbidden, "cleanup completion settles a mark_suspended intent"), (CleanupFailed, CleanupRequired, AllowedNotRequested, "only cleanup recovery re-requests cleanup"), (CleanupFailed, Quarantined, AllowedNotRequested, RECOVERY_QUARANTINE), (Quarantined, Started, WriteForbidden, "pause-failed quarantine recovery"), diff --git a/crates/libs/sbx/mgr/server/src/service/cleanup_support.rs b/crates/libs/sbx/mgr/server/src/service/cleanup_support.rs index 9183a26e3e..ac35beaf5c 100644 --- a/crates/libs/sbx/mgr/server/src/service/cleanup_support.rs +++ b/crates/libs/sbx/mgr/server/src/service/cleanup_support.rs @@ -238,10 +238,6 @@ pub(super) fn require_state_write( /// error is `Internal`. Every refusal is `Aborted`: a from-state refusal /// reports the state another transition left the row in, and a generic-guard /// refusal or a missing row keeps `update_state`'s message. -#[cfg_attr( - not(test), - expect(dead_code, reason = "the update_state_from conversions adopt it") -)] pub(super) fn require_state_write_from( result: rusqlite::Result, context: &'static str, @@ -255,10 +251,6 @@ pub(super) fn require_state_write_from( /// Why an `update_state_from` write did not apply. A caller that answers one /// case differently matches it and passes the rest to /// [`StateWriteRejected::into_status`]. -#[cfg_attr( - not(test), - expect(dead_code, reason = "the update_state_from conversions adopt it") -)] #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(super) enum StateWriteRejected { /// Another transition moved the row outside the edge's source states. @@ -275,20 +267,12 @@ pub(super) enum StateWriteRejected { } /// The generic guards `update_state` and `update_state_from` share. -#[cfg_attr( - not(test), - expect(dead_code, reason = "the update_state_from conversions adopt it") -)] #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(super) enum StateWriteGuard { Quarantined, CleanupOwned, } -#[cfg_attr( - not(test), - expect(dead_code, reason = "the update_state_from conversions adopt it") -)] pub(super) fn state_write_result( outcome: crate::db::StateWriteOutcome, ) -> Result { @@ -317,10 +301,6 @@ pub(super) fn state_write_result( } } -#[cfg_attr( - not(test), - expect(dead_code, reason = "the update_state_from conversions adopt it") -)] impl StateWriteRejected { pub(super) fn into_status(self, context: &str) -> Status { use crate::db::{SandboxStateWriteNotApplied, SandboxStateWriteOutcome}; diff --git a/crates/libs/sbx/mgr/server/src/service/create.rs b/crates/libs/sbx/mgr/server/src/service/create.rs index e46a361d44..158d1fd31f 100644 --- a/crates/libs/sbx/mgr/server/src/service/create.rs +++ b/crates/libs/sbx/mgr/server/src/service/create.rs @@ -5,9 +5,7 @@ use super::activation_support::{ space_api_route_identity_to_wire, spawn_restore_state_transition, }; use super::cache_cleanup::ensure_sandbox_cache_root; -use super::cleanup_support::{ - ActivationCleanupSpec, CleanupCompletion, read_node_execution_fence, require_state_write, -}; +use super::cleanup_support::{ActivationCleanupSpec, CleanupCompletion, read_node_execution_fence}; use super::create_support::*; use super::network_runtime::*; use super::spaced_bridge::*; @@ -416,9 +414,9 @@ impl SandboxManagerService { let update_result = { let db = self.db_conn()?; - crate::db::update_state(&db, &id, SandboxState::Created) + write_create_completed(&db, &id) }; - if let Err(status) = require_state_write(update_result, "complete sandbox create") { + if let Err(status) = update_result { create_cleanup.cleanup_after_failure( SandboxQuarantineReasonCode::CreateInterrupted, status.message(), @@ -528,11 +526,9 @@ impl SandboxManagerService { { let update_result = { let db = self.db_conn()?; - crate::db::update_state(&db, &id, SandboxState::Suspended) + write_base_snapshot_create_completed(&db, &id) }; - if let Err(status) = - require_state_write(update_result, "complete restored sandbox create") - { + if let Err(status) = update_result { create_cleanup.cleanup_after_failure( SandboxQuarantineReasonCode::CreateInterrupted, status.message(), diff --git a/crates/libs/sbx/mgr/server/src/service/create_support.rs b/crates/libs/sbx/mgr/server/src/service/create_support.rs index 1b00b5af80..9837548cbe 100644 --- a/crates/libs/sbx/mgr/server/src/service/create_support.rs +++ b/crates/libs/sbx/mgr/server/src/service/create_support.rs @@ -1,4 +1,5 @@ use super::activation_support::space_api_route_identity_to_wire; +use super::cleanup_support::{StateWriteRejected, require_state_write_from, state_write_result}; use super::template_snapshot_support::compute_partial_sha256; use super::validation::{ TemplateRuntimeDefaults, reject_incompatible_template_arch, @@ -285,6 +286,78 @@ pub(super) fn template_create_hash( } } +/// Create completion's node-local SQLite `sandboxes.state` write, +/// `creating → created`. Create holds no lock between its insert and this +/// write, so a row a Stop moved to `stopping` refuses it, and the Stop keeps +/// the row. +pub(super) fn write_create_completed(db: &rusqlite::Connection, id: &str) -> Result<(), Status> { + let edge = &crate::db::edges::CREATE_COMPLETE; + require_state_write_from( + crate::db::update_state_from(db, id, edge), + "complete sandbox create", + ) +} + +/// Base-snapshot create's node-local SQLite `sandboxes.state` write after its +/// own `created` write, `created → suspended`, refused like +/// [`write_create_completed`]. +pub(super) fn write_base_snapshot_create_completed( + db: &rusqlite::Connection, + id: &str, +) -> Result<(), Status> { + let edge = &crate::db::edges::CREATE_BASE_SNAPSHOT_COMPLETE; + require_state_write_from( + crate::db::update_state_from(db, id, edge), + "complete restored sandbox create", + ) +} + +fn warm_start_refused(id: &str, found: SandboxState) -> Status { + Status::failed_precondition(format!( + "cannot start sandbox {id} from a warm snapshot while local state is {found}; \ + expected suspended, created or stopped" + )) +} + +/// Admit a warm-snapshot Start only of a row read in its node-local SQLite +/// `sandboxes.state` as a restage source or already `suspended`, before it +/// stages the base snapshot into the sandbox's suspend snapshot directory, +/// which restore treats as local suspend evidence. The restage write checks +/// again: a row can move while the snapshot is staged. +pub(super) fn admit_warm_start(sandbox: &crate::db::SandboxRow) -> Result<(), Status> { + if sandbox.state == SandboxState::Suspended + || crate::db::edges::WARM_START_RESTAGE + .from() + .contains(sandbox.state) + { + Ok(()) + } else { + Err(warm_start_refused(&sandbox.id, sandbox.state)) + } +} + +/// Warm-snapshot Start's node-local SQLite `sandboxes.state` write after it +/// staged the base snapshot, `created` or `stopped` → `suspended`, so the +/// checkpoint restore can claim the row. A row that moved to `suspended` in +/// the meantime needs no write. A row another transition moved anywhere else +/// is `FailedPrecondition`, as [`admit_warm_start`] answers it; a generic-guard +/// refusal or a missing row keeps `require_state_write_from`'s `Aborted`. +pub(super) fn write_warm_start_restaged(db: &rusqlite::Connection, id: &str) -> Result<(), Status> { + const CONTEXT: &str = "prepare recovered sandbox restore"; + let outcome = crate::db::update_state_from(db, id, &crate::db::edges::WARM_START_RESTAGE) + .map_err(|error| Status::internal(format!("{CONTEXT}: {error}")))?; + match state_write_result(outcome) { + Ok(_) + | Err(StateWriteRejected::FromState { + found: SandboxState::Suspended, + }) => Ok(()), + Err(StateWriteRejected::FromState { found }) => Err(warm_start_refused(id, found)), + Err(rejected) => Err(rejected.into_status(CONTEXT)), + } +} + +#[cfg(test)] +mod state_write_tests; #[cfg(test)] mod tests; diff --git a/crates/libs/sbx/mgr/server/src/service/create_support/state_write_tests.rs b/crates/libs/sbx/mgr/server/src/service/create_support/state_write_tests.rs new file mode 100644 index 0000000000..2429d68f0d --- /dev/null +++ b/crates/libs/sbx/mgr/server/src/service/create_support/state_write_tests.rs @@ -0,0 +1,404 @@ +//! Create completion's and warm-snapshot Start's node-local SQLite +//! `sandboxes.state` writes, which write only over the states they expect. +//! +//! Each writer is driven directly: past the durable insert, Create and a warm +//! Start reach these writes only after creating directories under the fixed +//! host sandbox cache. The expected outcomes are written out here, not derived +//! from the edge table. + +use pplx_space_common_core::types::SandboxQuarantineReasonCode; + +use super::*; +use crate::db::tests::state_write_fixtures::{ + assert_unchanged, backdate, force_state, insert_creating, insert_row, seed_receipt, + set_failure_reason, snapshot, +}; +use crate::db::{ + ActivationKind, CleanupCompletion, CleanupEpisodeRequestOutcome, + CompleteRowIntentCleanupDisposition, +}; + +/// A non-blocking Stop's admission transaction: supersede the activation, +/// write `stopping` and persist the `mark_stopped` intent. +fn admit_stop(conn: &rusqlite::Connection, id: &str) { + 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(); +} + +fn state(conn: &rusqlite::Connection, id: &str) -> SandboxState { + crate::db::get_sandbox(conn, id).unwrap().unwrap().state +} + +type Write = fn(&rusqlite::Connection, &str) -> Result<(), Status>; + +/// A writer's expected answer for the row with an id in each source state. +type Outcomes = fn(&str, SandboxState) -> Expected; + +const CREATE: &str = "complete sandbox create"; +const BASE_SNAPSHOT_CREATE: &str = "complete restored sandbox create"; +const WARM_START: &str = "prepare recovered sandbox restore"; + +fn superseded(context: &str, found: SandboxState) -> String { + format!("{context}: sandbox state write was superseded: sandbox is {found}") +} + +fn not_applied(context: &str, outcome: &str) -> String { + format!("{context}: sandbox state write was not applied: {outcome}") +} + +/// Create holds no lock between its insert and its completion writes, so a +/// Stop admitted in between leaves the row `stopping`. The completion write +/// must refuse it and leave the row to the Stop, whose completion then +/// settles `stopped`. A completion that overwrote `stopping` would leave the +/// row behind a failed create with a stale `mark_stopped` intent, which +/// refuses every activation. The Stop RPC cannot reach a row during create, +/// which has no runtime, so this is the admission transaction itself. +#[test] +fn create_completion_leaves_a_row_a_stop_admitted_to_the_stop() { + struct Case { + name: &'static str, + /// The create's own writes before the Stop is admitted. + before_stop: &'static [Write], + write: Write, + context: &'static str, + } + for case in [ + Case { + name: "created completion", + before_stop: &[], + write: write_create_completed, + context: CREATE, + }, + Case { + name: "base-snapshot completion", + before_stop: &[write_create_completed], + write: write_base_snapshot_create_completed, + context: BASE_SNAPSHOT_CREATE, + }, + ] { + let conn = crate::db::test_conn(); + let (id, cleanup_id) = insert_creating(&conn, CleanupCompletion::DeleteRow); + for write in case.before_stop { + write(&conn, &id).unwrap(); + } + admit_stop(&conn, &id); + + let before = snapshot(&conn); + let status = (case.write)(&conn, &id).expect_err(case.name); + assert_eq!(status.code(), tonic::Code::Aborted, "{}", case.name); + assert_eq!( + status.message(), + superseded(case.context, SandboxState::Stopping), + "{}", + case.name + ); + assert_unchanged(case.name, &before, &conn); + + // Create then fails through its cleanup guard, which requests the + // create episode the Stop superseded, and changes nothing. + assert_eq!( + crate::db::request_cleanup_episode( + &conn, + &id, + cleanup_id, + SandboxQuarantineReasonCode::CreateInterrupted, + status.message(), + ) + .unwrap(), + CleanupEpisodeRequestOutcome::Superseded, + "{}", + case.name + ); + assert_unchanged(case.name, &before, &conn); + + // The Stop still owns its intent and settles the row. + assert_eq!( + crate::db::complete_row_intent_cleanup_for_job( + &conn, + &id, + CleanupCompletion::MarkStopped, + None, + None, + None, + ) + .unwrap(), + CompleteRowIntentCleanupDisposition::Applied, + "{}", + case.name + ); + let row = crate::db::get_sandbox(&conn, &id).unwrap().unwrap(); + assert_eq!(row.state, SandboxState::Stopped, "{}", case.name); + assert_eq!(row.cleanup_completion, None, "{}", case.name); + } +} + +/// What one writer answers for a decorated plain row in a given state. +#[derive(Clone, Debug, Eq, PartialEq)] +enum Expected { + /// The row moves to the state. + Writes(SandboxState), + /// The writer succeeds and changes nothing. + AlreadyThere, + /// Nothing is written; the status code and the exact message. + Refused(tonic::Code, String), +} + +fn aborted(message: String) -> Expected { + Expected::Refused(tonic::Code::Aborted, message) +} + +/// Create completion: only `creating` is written. A `deleting` row's delete +/// owns it, so the cleanup guard answers. Wildcard-free, so a new lifecycle +/// state does not compile until it is placed. +fn create_completed(_: &str, state: SandboxState) -> Expected { + match state { + SandboxState::Creating => Expected::Writes(SandboxState::Created), + SandboxState::Quarantined => aborted(not_applied(CREATE, "blocked_by_quarantine")), + SandboxState::Deleting => aborted(not_applied(CREATE, "superseded_by_cleanup_episode")), + SandboxState::Created + | SandboxState::Starting + | SandboxState::Started + | SandboxState::Stopping + | SandboxState::Stopped + | SandboxState::Pausing + | SandboxState::Paused + | SandboxState::Resuming + | SandboxState::Checkpointing + | SandboxState::Suspending + | SandboxState::Suspended + | SandboxState::Restoring + | SandboxState::CleanupRequired + | SandboxState::CleanupFailed + | SandboxState::Failed => aborted(superseded(CREATE, state)), + } +} + +/// Base-snapshot create completion: only `created` is written. +fn base_snapshot_create_completed(_: &str, state: SandboxState) -> Expected { + const CONTEXT: &str = BASE_SNAPSHOT_CREATE; + match state { + SandboxState::Created => Expected::Writes(SandboxState::Suspended), + SandboxState::Quarantined => aborted(not_applied(CONTEXT, "blocked_by_quarantine")), + SandboxState::Deleting => aborted(not_applied(CONTEXT, "superseded_by_cleanup_episode")), + SandboxState::Creating + | SandboxState::Starting + | SandboxState::Started + | SandboxState::Stopping + | SandboxState::Stopped + | SandboxState::Pausing + | SandboxState::Paused + | SandboxState::Resuming + | SandboxState::Checkpointing + | SandboxState::Suspending + | SandboxState::Suspended + | SandboxState::Restoring + | SandboxState::CleanupRequired + | SandboxState::CleanupFailed + | SandboxState::Failed => aborted(superseded(CONTEXT, state)), + } +} + +fn warm_start_refused(id: &str, found: SandboxState) -> Expected { + Expected::Refused( + tonic::Code::FailedPrecondition, + format!( + "cannot start sandbox {id} from a warm snapshot while local state is {found}; \ + expected suspended, created or stopped" + ), + ) +} + +/// Warm Start restaging: `created` and `stopped` are written, and `suspended` +/// needs no write. Any other state another transition moved the row to is +/// refused like the read-state check before staging refuses it: a warm Start +/// must not restage a `started` row for a checkpoint restore. A `deleting` or +/// quarantined row answers its generic guard. Start refuses a `failed` row +/// before warm staging, so that row is the writer's contract only. +fn warm_start_restaged(id: &str, state: SandboxState) -> Expected { + match state { + SandboxState::Created | SandboxState::Stopped => Expected::Writes(SandboxState::Suspended), + SandboxState::Suspended => Expected::AlreadyThere, + SandboxState::Quarantined => aborted(not_applied(WARM_START, "blocked_by_quarantine")), + SandboxState::Deleting => aborted(not_applied(WARM_START, "superseded_by_cleanup_episode")), + SandboxState::Creating + | SandboxState::Starting + | SandboxState::Started + | SandboxState::Stopping + | SandboxState::Pausing + | SandboxState::Paused + | SandboxState::Resuming + | SandboxState::Checkpointing + | SandboxState::Suspending + | SandboxState::Restoring + | SandboxState::CleanupRequired + | SandboxState::CleanupFailed + | SandboxState::Failed => warm_start_refused(id, state), + } +} + +fn check(case: &str, conn: &rusqlite::Connection, id: &str, write: Write, expected: Expected) { + let before = snapshot(conn); + let result = write(conn, id); + match expected { + Expected::Writes(target) => { + result.unwrap_or_else(|status| panic!("{case}: {status:?}")); + assert_eq!(state(conn, id), target, "{case}"); + } + Expected::AlreadyThere => { + result.unwrap_or_else(|status| panic!("{case}: {status:?}")); + assert_unchanged(case, &before, conn); + } + Expected::Refused(code, message) => { + let status = result.expect_err(case); + assert_eq!(status.code(), code, "{case}: {status:?}"); + assert_eq!(status.message(), message, "{case}"); + assert_unchanged(case, &before, conn); + } + } +} + +/// Every row carries the side columns a same-state `update_state` write would +/// change: a failure reason, a receipt for another target and backdated +/// timestamps. So a writer that rewrites a row it should leave alone, such as +/// an already-`suspended` warm-start row, is visible in the row. +#[test] +fn completion_and_warm_start_writes_answer_every_source_state() { + let writers: [(&str, Write, Outcomes); 3] = [ + ( + "created completion", + write_create_completed, + create_completed, + ), + ( + "base-snapshot completion", + write_base_snapshot_create_completed, + base_snapshot_create_completed, + ), + ("warm start", write_warm_start_restaged, warm_start_restaged), + ]; + for (name, write, expected) in writers { + for from in SandboxState::all() { + let case = format!("{name} from {from}"); + let conn = crate::db::test_conn(); + let id = insert_row(&conn, from); + force_state(&conn, &id, from); + set_failure_reason(&conn, &id, "an earlier attempt failed"); + seed_receipt(&conn, &id, SandboxState::Started); + backdate(&conn, &id); + check(&case, &conn, &id, write, expected(&id, from)); + } + } +} + +/// A row that moved while the warm Start staged its snapshot and is now owned +/// by cleanup answers the generic guard before the from-set, and keeps the +/// guard's `Aborted`: only a row outside the from-set without a guard refusal +/// is `FailedPrecondition`. A concurrent cold Start or restore claims the row +/// with an activation episode. The `stopped` and `suspended` rows owned by an +/// episode are rewound by hand; they are the writer's contract only. +#[test] +fn warm_start_write_keeps_a_guard_refusal_aborted() { + struct Case { + name: &'static str, + entry: SandboxState, + kind: ActivationKind, + /// Where the row is put back after the activation began, if anywhere. + rewind: Option, + } + for case in [ + Case { + name: "cold start claimed the row", + entry: SandboxState::Stopped, + kind: ActivationKind::ColdStart, + rewind: None, + }, + Case { + name: "restore claimed the row", + entry: SandboxState::Suspended, + kind: ActivationKind::Restore, + rewind: None, + }, + Case { + name: "stopped row owned by a cleanup episode", + entry: SandboxState::Stopped, + kind: ActivationKind::ColdStart, + rewind: Some(SandboxState::Stopped), + }, + Case { + name: "suspended row owned by a cleanup episode", + entry: SandboxState::Suspended, + kind: ActivationKind::Restore, + rewind: Some(SandboxState::Suspended), + }, + ] { + let conn = crate::db::test_conn(); + let id = insert_row(&conn, case.entry); + assert!( + crate::db::begin_activation(&conn, &id, case.entry, case.kind, None, None) + .unwrap() + .is_some(), + "{}", + case.name + ); + if let Some(rewind) = case.rewind { + force_state(&conn, &id, rewind); + } + check( + case.name, + &conn, + &id, + write_warm_start_restaged, + aborted(not_applied(WARM_START, "superseded_by_cleanup_episode")), + ); + } +} + +/// What the read-state check before warm staging answers for a row in each +/// state: production warm Starts target `suspended` rows, and an operator's +/// `created` or `stopped` row is restaged. Wildcard-free, so a new lifecycle +/// state does not compile until it is placed. +fn warm_start_admitted(state: SandboxState) -> bool { + match state { + SandboxState::Created | SandboxState::Stopped | SandboxState::Suspended => true, + SandboxState::Creating + | SandboxState::Starting + | SandboxState::Started + | SandboxState::Stopping + | SandboxState::Deleting + | SandboxState::Pausing + | SandboxState::Paused + | SandboxState::Resuming + | SandboxState::Checkpointing + | SandboxState::Suspending + | SandboxState::Restoring + | SandboxState::CleanupRequired + | SandboxState::CleanupFailed + | SandboxState::Quarantined + | SandboxState::Failed => false, + } +} + +#[test] +fn warm_start_admits_only_its_sources_and_suspended_rows() { + for from in SandboxState::all() { + let case = format!("warm start admission of {from}"); + let conn = crate::db::test_conn(); + let id = insert_row(&conn, from); + force_state(&conn, &id, from); + let sandbox = crate::db::get_sandbox(&conn, &id).unwrap().unwrap(); + let result = admit_warm_start(&sandbox); + if warm_start_admitted(from) { + result.unwrap_or_else(|status| panic!("{case}: {status:?}")); + } else { + let status = result.expect_err(&case); + assert_eq!( + Expected::Refused(status.code(), status.message().to_owned()), + warm_start_refused(&id, from), + "{case}" + ); + } + } +} diff --git a/crates/libs/sbx/mgr/server/src/service/start.rs b/crates/libs/sbx/mgr/server/src/service/start.rs index 3b4c662709..d3b0e36dd4 100644 --- a/crates/libs/sbx/mgr/server/src/service/start.rs +++ b/crates/libs/sbx/mgr/server/src/service/start.rs @@ -11,17 +11,15 @@ use super::activation_support::{ space_api_route_identity_from_start_request, space_api_route_identity_to_wire, spawn_wait_ready_state_transition, wait_ready_and_mark_started, wake_target_state_from_proto, }; -use super::cleanup_support::{ - ActivationCleanupSpec, read_node_execution_fence, require_state_write, -}; +use super::cleanup_support::{ActivationCleanupSpec, read_node_execution_fence}; use super::create::StartFromCheckpointLaunch; use super::create_support::{ CheckpointRestoreRetryAction, GUEST_GATEWAY, GUEST_IP, RestoredSnapshotProvenance, - RuntimeActivatedEvent, SUSPEND_CHECKPOINT, checkpoint_restore_retry_action, + RuntimeActivatedEvent, SUSPEND_CHECKPOINT, admit_warm_start, checkpoint_restore_retry_action, checkpoint_start_response_for_existing_started_sandbox, emit_runtime_activated_event, guest_console_logs_enabled, resolve_template_paths_for_start, sandbox_rootfs_exists, stage_base_snapshot_runtime_metadata, template_runtime_defaults_for_sandbox_start, - template_source_for_sandbox_prepare, + template_source_for_sandbox_prepare, write_warm_start_restaged, }; use super::network_runtime::{ launch_net_gateway_via_node, net_gateway_launch_config, register_net_gateway_launch, @@ -1352,6 +1350,7 @@ impl SandboxManagerService { if !ctx.from_warm { return Ok(None); } + admit_warm_start(sandbox)?; self.ensure_snapshot_restore_supported("start_from_warm")?; tracing::Span::current().record( @@ -1426,10 +1425,7 @@ impl SandboxManagerService { if sandbox.state != SandboxState::Suspended { let db = self.db_conn()?; - require_state_write( - crate::db::update_state(&db, &sandbox.id, SandboxState::Suspended), - "prepare recovered sandbox restore", - )?; + write_warm_start_restaged(&db, &sandbox.id)?; } let sandbox = self.lookup_sandbox(&sandbox.id)?; diff --git a/crates/libs/sbx/mgr/server/src/service/start/tests.rs b/crates/libs/sbx/mgr/server/src/service/start/tests.rs index 7c57fb39aa..b96e309cd6 100644 --- a/crates/libs/sbx/mgr/server/src/service/start/tests.rs +++ b/crates/libs/sbx/mgr/server/src/service/start/tests.rs @@ -815,6 +815,24 @@ async fn start_refusals_before_activation_leave_the_row_state_unchanged() { code: tonic::Code::FailedPrecondition, message: "file-backed memory", }, + Case { + name: "warm start of a started row stages nothing", + state: SandboxState::Started, + template_hash: true, + hugepages: false, + restrict_to_template_manager_owned: false, + code: tonic::Code::FailedPrecondition, + message: "from a warm snapshot while local state is started", + }, + Case { + name: "warm start of a paused row stages nothing", + state: SandboxState::Paused, + template_hash: true, + hugepages: false, + restrict_to_template_manager_owned: false, + code: tonic::Code::FailedPrecondition, + message: "from a warm snapshot while local state is paused", + }, Case { name: "warm start never resolves a template by name alone", state: SandboxState::Stopped,