Skip to content

sbx-mgr: retire the generic update_state #498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ac-pplx merged 1 commit into from
Oct 15, 2026
Merged

sbx-mgr: retire the generic update_state #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion architecture-baseline.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
"crates/libs/sbx/mgr/server/src/db/cleanup_episodes.rs" = 1833
"crates/libs/sbx/mgr/server/src/db/quarantine_recovery.rs" = 1042
"crates/libs/sbx/mgr/server/src/db/sandboxes.rs" = 1553
"crates/libs/sbx/mgr/server/src/db/state_transitions.rs" = 1140
"crates/libs/sbx/mgr/server/src/db/state_transitions.rs" = 1018
"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" = 1574
Expand Down
11 changes: 5 additions & 6 deletions crates/libs/sbx/mgr/server/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@

## Changes

- Write `sandboxes.state` only through `src/db` writers. A new transition
calls `update_state_from` with its edge, or adds a specialized
compare-and-set writer in `src/db`. Do not add `update_state` callers: it
overwrites any state its cleanup and quarantine guards admit. Record the
edge in `src/db/state_write.rs`, and add any new disagreement with
`can_transition_to`, with a reason, to `ALLOWLIST` in
- Write the node-local SQLite `sandboxes.state` only through `src/db`: every
persisted write goes through `update_state_from` with an explicit edge, or
through a named specialized writer. A new edge in `src/db/state_write.rs`
needs a kind and a doc comment naming its writer; add any new disagreement
with `can_transition_to`, with a reason, to `ALLOWLIST` in
`src/db/tests/state_write_table.rs`. Nothing checks the table against the
writers; the `&'static Edge` argument does that at call sites. A refused
write must leave every row unchanged. Map an `update_state_from` outcome to
Expand Down
31 changes: 24 additions & 7 deletions crates/libs/sbx/mgr/server/src/db/startup_recovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,22 @@ pub enum RecoverStartupStatesError {
CleanupRequest(#[from] RequestCleanupEpisodeError),
#[error("armed cleanup episode has unsupported activation state {0}")]
UnsupportedActivationState(SandboxState),
#[error("startup recovery write {edge} was not applied: {outcome:?}")]
StateWriteNotApplied {
#[error("startup recovery write {edge} was refused: sandbox is {found}, {}", refusal_text(.refusal))]
StateWriteRefused {
edge: &'static str,
outcome: StateWriteOutcome,
found: SandboxState,
refusal: StateWriteRefusal,
},
#[error("startup recovery write {edge} found no sandbox row")]
StateWriteMissing { edge: &'static str },
}

fn refusal_text(refusal: &StateWriteRefusal) -> &'static str {
match refusal {
StateWriteRefusal::FromState => "outside the edge's source states",
StateWriteRefusal::Guard(StateWriteGuard::Quarantined) => "blocked by quarantine",
StateWriteRefusal::Guard(StateWriteGuard::CleanupOwned) => "owned by cleanup",
}
}

/// [`update_state_from`] for a startup recovery writer that requires its
Expand All @@ -393,9 +404,15 @@ pub fn require_startup_state_write(
) -> std::result::Result<(), RecoverStartupStatesError> {
match update_state_from(conn, id, edge)? {
StateWriteOutcome::Applied { .. } => Ok(()),
outcome => Err(RecoverStartupStatesError::StateWriteNotApplied {
edge: edge.name(),
outcome,
}),
StateWriteOutcome::Refused { found, refusal } => {
Err(RecoverStartupStatesError::StateWriteRefused {
edge: edge.name(),
found,
refusal,
})
}
StateWriteOutcome::Missing => {
Err(RecoverStartupStatesError::StateWriteMissing { edge: edge.name() })
}
}
}
148 changes: 9 additions & 139 deletions crates/libs/sbx/mgr/server/src/db/state_transitions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,136 +48,6 @@ pub enum SupersedeActivationError {
StateWriteNotApplied(#[from] SandboxStateWriteNotApplied),
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[must_use = "callers must distinguish an applied state write from a superseded one"]
pub enum SandboxStateWriteOutcome {
Applied,
Missing,
BlockedByQuarantine,
SupersededByCleanupEpisode,
}

impl SandboxStateWriteOutcome {
pub const fn as_str(self) -> &'static str {
match self {
Self::Applied => "applied",
Self::Missing => "missing",
Self::BlockedByQuarantine => "blocked_by_quarantine",
Self::SupersededByCleanupEpisode => "superseded_by_cleanup_episode",
}
}

pub const fn is_applied(self) -> bool {
matches!(self, Self::Applied)
}

pub fn require_applied(self) -> std::result::Result<(), SandboxStateWriteNotApplied> {
if self.is_applied() {
Ok(())
} else {
Err(SandboxStateWriteNotApplied { outcome: self })
}
}
}

impl fmt::Display for SandboxStateWriteOutcome {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(self.as_str())
}
}

#[derive(Debug, thiserror::Error)]
#[error("sandbox state write was not applied: {outcome}")]
pub struct SandboxStateWriteNotApplied {
pub outcome: SandboxStateWriteOutcome,
}

pub fn update_state(
conn: &Connection,
id: &str,
state: SandboxState,
) -> Result<SandboxStateWriteOutcome> {
if matches!(state, SandboxState::Quarantined | SandboxState::Failed) {
return Err(rusqlite::Error::InvalidQuery);
}
if state == SandboxState::Deleting && conn.is_autocommit() {
let tx = immediate_transaction(conn)?;
let outcome = update_state(&tx, id, state)?;
tx.commit()?;
return Ok(outcome);
}
let previous_state = current_state_label(conn, id)?;
let previous_state_value = previous_state
.as_deref()
.and_then(|state| state.parse::<SandboxState>().ok());
match generic_state_write_refusal(conn, id, previous_state_value, state)? {
Some(StateWriteRefusal::Quarantined) => {
tracing::warn!(
sandbox_id = %id,
target_state = %state,
"ignored generic sandbox state update because sandbox is quarantined"
);
return Ok(SandboxStateWriteOutcome::BlockedByQuarantine);
}
// The generic guards never answer `FromState`.
Some(StateWriteRefusal::CleanupOwned | StateWriteRefusal::FromState) => {
tracing::warn!(
sandbox_id = %id,
target_state = %state,
"ignored generic sandbox state update because cleanup owns the transition"
);
return Ok(SandboxStateWriteOutcome::SupersededByCleanupEpisode);
}
None => {}
}

let changed = conn.execute(
"UPDATE sandboxes
SET state = ?1,
state_changed_at = CASE WHEN state IS NOT ?1 THEN ?2 ELSE state_changed_at END,
failure_reason = NULL,
restore_origin = CASE WHEN ?1 = 'restoring' THEN restore_origin ELSE NULL END,
restore_phase = CASE WHEN ?1 = 'restoring' THEN restore_phase ELSE NULL END,
restore_storage_uri = CASE WHEN ?1 = 'restoring' THEN restore_storage_uri ELSE NULL END,
transition_receipt_id = CASE
WHEN ?1 = transition_receipt_target_state THEN transition_receipt_id
ELSE NULL
END,
transition_receipt_kind = CASE
WHEN ?1 = transition_receipt_target_state THEN transition_receipt_kind
ELSE NULL
END,
transition_receipt_target_state = CASE
WHEN ?1 = transition_receipt_target_state THEN transition_receipt_target_state
ELSE NULL
END,
transition_receipt_execution_generation = CASE
WHEN ?1 = transition_receipt_target_state THEN transition_receipt_execution_generation
ELSE NULL
END,
transition_receipt_activation_attempt_id = CASE
WHEN ?1 = transition_receipt_target_state THEN transition_receipt_activation_attempt_id
ELSE NULL
END,
transition_receipt_reason_code = CASE
WHEN ?1 = transition_receipt_target_state THEN transition_receipt_reason_code
ELSE NULL
END,
updated_at = ?2
WHERE id = ?3
AND (?1 = 'deleting' OR state != 'quarantined')",
params![state.as_str(), now()?, id],
)?;
if changed != 0 && state == SandboxState::Deleting {
teardown_jobs::fail_active(conn, id, TeardownSuspendFailure::Deleting)?;
}
Ok(if changed == 0 {
SandboxStateWriteOutcome::Missing
} else {
SandboxStateWriteOutcome::Applied
})
}

pub(crate) fn restore_checkpoint_state(
conn: &Connection,
id: &str,
Expand All @@ -199,9 +69,10 @@ pub(crate) fn restore_checkpoint_state(
Ok(changed == 1)
}

/// Fence policy installation and Paused -> Resuming in one transaction.
/// An absent generation matches only an identity-free legacy resident; it is
/// never a wildcard for an execution that has acquired a generation.
/// Fence policy installation and `paused` -> `resuming` in one transaction:
/// [`edges::RESUME_BEGIN`] refuses any other state. An absent generation
/// matches only an identity-free legacy resident; it is never a wildcard for an
/// execution that has acquired a generation.
pub(crate) fn begin_resident_resume_with_policy(
conn: &Connection,
id: &str,
Expand All @@ -212,13 +83,12 @@ pub(crate) fn begin_resident_resume_with_policy(
let Some(sandbox) = get_sandbox(&tx, id)? else {
return Ok(false);
};
if sandbox.state != SandboxState::Paused
|| sandbox.space_api_route_identity.execution_generation != generation
{
if sandbox.space_api_route_identity.execution_generation != generation {
return Ok(false);
}
if update_state(&tx, id, SandboxState::Resuming)? != SandboxStateWriteOutcome::Applied {
return Ok(false);
match update_state_from(&tx, id, &edges::RESUME_BEGIN)? {
StateWriteOutcome::Applied { .. } => {}
StateWriteOutcome::Refused { .. } | StateWriteOutcome::Missing => return Ok(false),
}
install_activation_auto_lifecycle(&tx, id, policy)?;
tx.commit()?;
Expand Down Expand Up @@ -311,7 +181,7 @@ pub(crate) fn update_state_superseding_activation_in_transaction(
params![id, episode.completion.as_str(), episode.keep_filesystem],
)?;
}
update_state(conn, id, state)?.require_applied()?;
write_cleanup_admission_state(conn, id, state)??;
Ok(())
}

Expand Down
Loading
Loading