[nexus] instance update saga: retry "inherit lock", too

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<UpdaterLock, UpdaterLockError> {
         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());
 
@@ -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,

nexus/src/app/sagas/instance_update/mod.rs

@@ -360,13 +360,16 @@ 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;
 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();

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