From 07b3010bd8648eec57f3f7dd8a5ed87ca9322f04 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 10 Jul 2026 14:35:22 -0700 Subject: [PATCH 1/2] [nexus] instance update saga: retry "inherit lock", too Similarly to #10166, a duplicate execution of the `siu_become_updater` saga action, which attempts to inherit the lock from the parent `start_instance_update` saga, must never fail if a previous execution of that node has already marked the instance record as locked. Like in PR #10177, which fixed #10166, we can resolve this by adding a retry loop that should retry any transient database errors indefinitely. And, as we discovered in #10785, the exception to this rule is cases where the instance record has been deleted, so we handle that gracefully here as well (or at least we will, once #10787 actually fixes the lock inheriting query's handling of deleted instance records...). --- nexus/db-queries/src/db/datastore/instance.rs | 4 +- nexus/src/app/sagas/instance_update/mod.rs | 137 +++++++++++++++--- 2 files changed, 117 insertions(+), 24 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 45fce7bebb9..c64c40d58f3 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -1724,11 +1724,11 @@ impl DataStore { &self, opctx: &OpContext, authz_instance: &authz::Instance, - parent_lock: UpdaterLock, + parent_lock: &UpdaterLock, child_lock_id: Uuid, ) -> Result { use nexus_db_schema::schema::instance::dsl; - let UpdaterLock { updater_id: parent_id, locked_gen } = parent_lock; + let &UpdaterLock { updater_id: parent_id, locked_gen } = parent_lock; let instance_id = authz_instance.id(); let new_gen = Generation(locked_gen.0.next()); diff --git a/nexus/src/app/sagas/instance_update/mod.rs b/nexus/src/app/sagas/instance_update/mod.rs index 1d753dba36c..1ea77de0f7b 100644 --- a/nexus/src/app/sagas/instance_update/mod.rs +++ b/nexus/src/app/sagas/instance_update/mod.rs @@ -360,6 +360,8 @@ use crate::app::instance_network::InstanceNetworkFilters; use crate::app::sagas::declare_saga_actions; use anyhow::Context; use chrono::Utc; +use dropshot::HttpError; +use futures::{TryFutureExt, future}; use nexus_db_lookup::LookupPath; use nexus_db_queries::{authn, authz}; use nexus_types::identity::Resource; @@ -367,6 +369,7 @@ use nexus_types::instance::SledVmmState; use nexus_types::saga::saga_action_failed; use omicron_common::api::external::Error; use omicron_common::backoff; +use omicron_common::backoff::BackoffError; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::PropolisUuid; @@ -1026,35 +1029,127 @@ async fn siu_become_updater( crate::context::op_context_for_saga_action(&sagactx, serialized_authn); let osagactx = sagactx.user_data(); let log = osagactx.log(); + let instance_id = authz_instance.id(); debug!( log, "instance update: trying to become instance updater..."; - "instance_id" => %authz_instance.id(), + "instance_id" => %instance_id, "saga_id" => %saga_id, "parent_lock" => ?orig_lock, ); - let lock = osagactx - .datastore() - .instance_updater_inherit_lock( - &opctx, - &authz_instance, - orig_lock, - saga_id, - ) - .await - .map_err(|e| match e { - // The `AlreadyLocked` variant must be serialized as - // `UpdaterLockError` so that the parent `start-instance-update` - // saga can identify this specific failure mode and handle it - // gracefully. - #[expect(clippy::disallowed_methods)] - instance::UpdaterLockError::AlreadyLocked => { - ActionError::action_failed(e) + // /!\ EXTREMELY IMPORTANT WARNING /!\ + // + // We are about to attempt to inherit the instance record's updater lock + // from the "parent" `start_instance_update` saga. If the parent saga is + // still holding the lock (which we expect it should be), we will proceed to + // actually perform the rest of the update. However, we may encounter a + // transient database error (i.e., one which does not positively indicate + // that either the expected parent lock ID was not present, or that the + // instance has been deleted out from under us) while attempting to do so. + // In that case, this action MUST continue retrying the lock operation + // forever until it either succeeds or indicates affirmatively that it + // cannot succeed ever, in order to satisfy the distributed saga requirement + // that executing an action must be idempotent. Retrying indefinitely is + // necessary to ensure idempotency because it is possible that a previous + // execution of this action *did* successfully inherit the lock but crashed + // before it completed. + // + // As an example of why this is important, consider a particularly + // unlucky sequence of a Nexus crash followed by a transient database error + // could leave the instance record permanently locked by this (now failed) + // saga. The scenario in which this would occur is as follows: + // + // 1. A Nexus starts executing this action, successfully inherits the lock + // and writes this saga's lock ID to the instance record, and then + // crashes *before* marking the saga node as having completed. + // 2. Subsequently, a new Nexus resumes executing the saga and runs this + // action again. It hits a transient query failure trying to inherit the + // lock, returns an `ActionFailed` error, and unwinds. + // 3. Because the saga node has not *completed*, our undo action + // (`siu_unbecome_updater()`), will *not* execute, so the instance + // record remains locked by this saga's lock ID. Since this saga has + // now failed, no one will ever unlock the instance. + // + // Due to this potential danger, we shall retry the lock operation forever + // until it either succeeds or indicates that the instance has been locked + // by another saga or has been deleted. Because the inherit-lock operation + // is idempotent if either *our* updater ID or the parent saga's ID is the + // one inside the lock, it is fine if this node executes multiple times. + // Retrying indefinitely is reasonable here based on the assumption that if + // we can't talk to the database, our only options are to keep retrying or + // unwind, and unwinding *also* requires that we be able to talk to the + // database, so we may as well retry. We will complain loudly if we've been + // retrying for a long time. + let lock = backoff::retry_notify_ext( + // This is an internal service query to CockroachDB. + backoff::retry_policy_internal_service(), + || { + osagactx + .datastore() + .instance_updater_inherit_lock( + &opctx, + &authz_instance, + &orig_lock, + saga_id, + ) + .map_err(move |e| match e { + // The `AlreadyLocked` variant must be serialized as + // `UpdaterLockError` so that the parent + // `start-instance-update` saga can identify this specific + // failure mode and handle it gracefully. + #[expect(clippy::disallowed_methods)] + instance::UpdaterLockError::AlreadyLocked => { + BackoffError::permanent(ActionError::action_failed(e)) + } + // The instance record has been deleted, give up. + instance::UpdaterLockError::Query( + err @ Error::ObjectNotFound { .. }, + ) => BackoffError::permanent(saga_action_failed(err)), + // Any other potential error should be retried. + instance::UpdaterLockError::Query(err) => { + BackoffError::transient(saga_action_failed(err)) + } + }) + }, + |error, call_count, total_duration| { + // N.B. that other notify closures attempt to distinguish between + // "client errors" and "server errors" based on the status code of + // the `HttpError` produced from the `external::Error`, so that we + // can log about them differently. That's a bit too tricky to do + // here, given that we must special-case the way the `AlreadyLocked` + // error is turned into an `ActionError` above, so...this code + // doesn't try to do that. Which is fine. + const MSG: &str = + "instance update: error while inheriting instance lock, \ + retrying"; + if total_duration > RETRY_WARN_AFTER { + warn!( + log, + "{MSG}"; + "instance_id" => %instance_id, + "saga_id" => %saga_id, + "parent_lock" => ?orig_lock, + "error" => %error, + "call_count" => call_count, + "total_duration" => ?total_duration, + ); + } else { + info!( + log, + "{MSG}"; + "instance_id" => %instance_id, + "saga_id" => %saga_id, + "parent_lock" => ?orig_lock, + "error" => %error, + "call_count" => call_count, + "total_duration" => ?total_duration, + ); } - instance::UpdaterLockError::Query(err) => saga_action_failed(err), - })?; + }, + ) + .await?; info!( log, @@ -1580,8 +1675,6 @@ async fn unwind_instance_lock( // - succeeds, and we know the instance is now unlocked. // - fails *because the instance doesn't exist*, in which case we can die // happily because it doesn't matter if the instance is actually unlocked. - use dropshot::HttpError; - use futures::{TryFutureExt, future}; let osagactx = sagactx.user_data(); let log = osagactx.log(); From f3f8ff9b46e6f16eb15eaa972ae5312936ab40cf Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 10 Jul 2026 14:58:08 -0700 Subject: [PATCH 2/2] fix some related incorrect comments while we're here --- nexus/db-queries/src/db/datastore/instance.rs | 6 +++--- nexus/src/app/sagas/instance_update/start.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index c64c40d58f3..c5dd93c86eb 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -1756,8 +1756,8 @@ impl DataStore { match result { // If we updated the record, the lock has been successfully - // inherited! Return `Ok(true)` to indicate that we have acquired - // the lock successfully. + // inherited! Return the new lock to indicate that we have acquired + // it successfully. UpdateAndQueryResult { status: UpdateStatus::Updated, .. } => { slog::debug!( &opctx.log, @@ -1775,7 +1775,7 @@ impl DataStore { } // The generation has advanced past the generation at which the // lock was held. This means that we have already inherited the - // lock. Return `Ok(false)` here for idempotency. + // lock. Return `Ok` here for idempotency. UpdateAndQueryResult { status: UpdateStatus::NotUpdatedButExists, ref found, diff --git a/nexus/src/app/sagas/instance_update/start.rs b/nexus/src/app/sagas/instance_update/start.rs index 5a3b8e0da4d..3ca60e0d03c 100644 --- a/nexus/src/app/sagas/instance_update/start.rs +++ b/nexus/src/app/sagas/instance_update/start.rs @@ -125,7 +125,7 @@ async fn siu_lock_instance( // This is necessary in order to satisfy the distributed saga requirement // that executing an action must be idempotent. Retrying transient database // errors indefinitely is necessary to ensure idempotency because it is - // possible that a previous execution of this action *did* succesfully + // possible that a previous execution of this action *did* successfully // acquire the lock but crashed before it completed. // // As an example of why this is important, consider a particularly