fix(instance): mark Instances deleted before force-delete cleanup

crates/api-core/src/handlers/instance.rs

@@ -1998,12 +1998,29 @@ pub(super) async fn force_delete_instance(
     api: &Api,
     response: &mut AdminForceDeleteMachineResponse,
 ) -> CarbideResult<()> {
-    let instance = db::instance::find_by_id(&api.database_connection, instance_id)
+    // The caller has already committed the Machine ForceDeletion state. Lock
+    // and reread the Instance in a separate transaction so this path never
+    // holds Machine and Instance locks together; IB updates lock the Instance
+    // before the Machine. Once the deletion marker commits, the captured
+    // snapshot is the last configuration a tenant update can commit before
+    // external cleanup.
+    let mut txn = api.txn_begin().await?;
+    let Some(locked_instance) =
+        db::instance::find_by_id_for_update(txn.as_mut(), instance_id).await?
+    else {
+        txn.commit().await?;
+        return Ok(());
+    };
+
+    if locked_instance.deleted.is_none() {
+        db::instance::mark_as_deleted(instance_id, txn.as_mut()).await?;
+    }
+    let instance = db::instance::find_by_id(&mut txn, instance_id)
         .await?
         .ok_or_else(|| {
             CarbideError::internal(format!("could not find an instance for {instance_id}"))
-        })?
-        .to_owned();
+        })?;
+    txn.commit().await?;
 
     response.ufm_unregistrations += unbind_all_instance_ib_ports(api, &instance).await?;
 

crates/api-core/src/tests/machine_admin_force_delete.rs

@@ -31,6 +31,7 @@ use carbide_machine_controller::dpf::{DpfOperations, MockDpfOperations};
 use carbide_uuid::infiniband::IBPartitionId;
 use carbide_uuid::instance::InstanceId;
 use carbide_uuid::machine::{MachineId, MachineType};
+use carbide_uuid::vpc::VpcPrefixId;
 use common::api_fixtures::dpu::create_dpu_machine;
 use common::api_fixtures::host::host_discover_dhcp;
 use common::api_fixtures::ib_partition::{DEFAULT_TENANT, create_ib_partition};
@@ -49,7 +50,7 @@ use model::instance::NewInstance;
 use model::instance::config::InstanceConfig;
 use model::instance::config::extension_services::InstanceExtensionServicesConfig;
 use model::instance::config::infiniband::InstanceInfinibandConfig;
-use model::instance::config::network::InstanceNetworkConfig;
+use model::instance::config::network::{InstanceNetworkConfig, NetworkDetails};
 use model::instance::config::nvlink::InstanceNvLinkConfig;
 use model::instance::config::spx::InstanceSpxConfig;
 use model::instance::config::tenant_config::TenantConfig;
@@ -647,6 +648,26 @@ fn force_delete_request(machine_id: &MachineId) -> AdminForceDeleteMachineReques
     }
 }
 
+/// Test-only function that locks one address so force-delete pauses after
+/// marking the Instance but before physically deleting it.
+async fn lock_instance_address(txn: &mut PgConnection, instance_id: InstanceId) {
+    let address_id: Option<uuid::Uuid> = sqlx::query_scalar(
+        "SELECT id FROM instance_addresses
+         WHERE instance_id = $1
+         ORDER BY id
+         LIMIT 1
+         FOR UPDATE",
+    )
+    .bind(instance_id)
+    .fetch_optional(txn)
+    .await
+    .unwrap();
+    assert!(
+        address_id.is_some(),
+        "fixture Instance must have an address to gate cleanup",
+    );
+}
+
 async fn retired_membership_is_recorded(pool: &sqlx::PgPool, membership: &IbMembership) -> bool {
     db::retired_ib_membership::find_recorded_candidates(pool, std::slice::from_ref(membership))
         .await
@@ -875,6 +896,172 @@ async fn test_admin_force_delete_reads_instance_after_machine_lock(pool: sqlx::P
     }
 }
 
+#[crate::sqlx_test]
+async fn test_admin_force_delete_rereads_config_committed_before_marker(pool: sqlx::PgPool) {
+    let env = create_test_env(pool).await;
+    let [segment_id, generated_segment_id] = env
+        .create_vpc_and_tenant_segments(2)
+        .await
+        .try_into()
+        .unwrap();
+    let managed_host = create_managed_host(&env).await;
+    let instance = managed_host
+        .instance_builer(&env)
+        .single_interface_network_config(segment_id)
+        .build()
+        .await;
+
+    let mut writer_txn = env.pool.begin().await.unwrap();
+    let initial = db::instance::find_by_id(writer_txn.as_mut(), instance.id)
+        .await
+        .unwrap()
+        .unwrap();
+    let mut updated_metadata = initial.metadata.clone();
+    updated_metadata.description = "committed before force-delete".to_string();
+    // Model the final writes of a config transaction after its generated
+    // segment exists. The transaction keeps the Instance lock until it commits.
+    let mut requested_network = initial.config.network.clone();
+    let interface = &mut requested_network.interfaces[0];
+    interface.network_details = Some(NetworkDetails::VpcPrefixId(VpcPrefixId::new()));
+    interface.network_segment_id = Some(generated_segment_id);
+    db::instance::trigger_update_network_config_request(
+        &instance.id,
+        &initial.config.network,
+        &requested_network,
+        &mut writer_txn,
+    )
+    .await
+    .unwrap();
+    db::instance::update_config(
+        writer_txn.as_mut(),
+        instance.id,
+        initial.config_version,
+        initial.config.clone(),
+        updated_metadata.clone(),
+    )
+    .await
+    .unwrap();
+
+    let mut address_guard = env.pool.begin().await.unwrap();
+    lock_instance_address(address_guard.as_mut(), instance.id).await;
+
+    let api = env.api.clone();
+    let machine_id = managed_host.id;
+    let force_delete = async move {
+        api.admin_force_delete_machine(Request::new(force_delete_request(&machine_id)))
+            .await
+    };
+    let orchestrate = async {
+        wait_until_blocked_on(&env.pool, "FOR UPDATE OF i").await;
+        writer_txn.commit().await.unwrap();
+
+        wait_until_blocked_on(&env.pool, "SELECT id FROM instance_addresses").await;
+        let marked = db::instance::find_by_id(&env.pool, instance.id)
+            .await
+            .unwrap()
+            .expect("address gate must keep the marked Instance available");
+        assert!(marked.deleted.is_some());
+        assert_eq!(
+            marked
+                .update_network_config_request
+                .as_ref()
+                .expect("the pending network update committed before the marker must be captured")
+                .new_config,
+            requested_network,
+        );
+        assert_eq!(marked.metadata, updated_metadata);
+        assert_eq!(
+            marked.config_version.version_nr(),
+            initial.config_version.version_nr() + 1,
+        );
+
+        address_guard.commit().await.unwrap();
+    };
+
+    let (response, ()) = tokio::join!(force_delete, orchestrate);
+    let response = response.unwrap().into_inner();
+    validate_delete_response(&response, Some(&managed_host.id), &managed_host.dpu().id);
+    for machine_id in [managed_host.id, managed_host.dpu().id] {
+        validate_machine_deletion(&env, &machine_id, None).await;
+    }
+    let generated_segment_is_deleted: bool =
+        sqlx::query_scalar("SELECT deleted IS NOT NULL FROM network_segments WHERE id = $1")
+            .bind(generated_segment_id)
+            .fetch_one(&env.pool)
+            .await
+            .unwrap();
+    assert!(
+        generated_segment_is_deleted,
+        "force-delete must clean resources from the configuration committed before the marker",
+    );
+}
+
+#[crate::sqlx_test]
+async fn test_admin_force_delete_marker_rejects_started_config_update(pool: sqlx::PgPool) {
+    let env = create_test_env(pool).await;
+    let segment_id = env.create_vpc_and_tenant_segment().await;
+    let managed_host = create_managed_host(&env).await;
+    let instance = managed_host
+        .instance_builer(&env)
+        .single_interface_network_config(segment_id)
+        .build()
+        .await;
+
+    let mut stale_writer_txn = env.pool.begin().await.unwrap();
+    let initial = db::instance::find_by_id(stale_writer_txn.as_mut(), instance.id)
+        .await
+        .unwrap()
+        .unwrap();
+    let initial_metadata = initial.metadata.clone();
+    let mut rejected_metadata = initial_metadata.clone();
+    rejected_metadata.description = "must not commit after force-delete".to_string();
+
+    let mut address_guard = env.pool.begin().await.unwrap();
+    lock_instance_address(address_guard.as_mut(), instance.id).await;
+
+    let api = env.api.clone();
+    let machine_id = managed_host.id;
+    let force_delete = async move {
+        api.admin_force_delete_machine(Request::new(force_delete_request(&machine_id)))
+            .await
+    };
+    let orchestrate = async {
+        wait_until_blocked_on(&env.pool, "SELECT id FROM instance_addresses").await;
+
+        let error = db::instance::update_config(
+            stale_writer_txn.as_mut(),
+            instance.id,
+            initial.config_version,
+            initial.config.clone(),
+            rejected_metadata,
+        )
+        .await
+        .expect_err("a config update whose terminal write runs after the marker must not commit");
+        assert!(
+            matches!(error, db::DatabaseError::FailedPrecondition(_)),
+            "unexpected config update error: {error:?}",
+        );
+        stale_writer_txn.rollback().await.unwrap();
+
+        let marked = db::instance::find_by_id(&env.pool, instance.id)
+            .await
+            .unwrap()
+            .expect("address gate must keep the marked Instance available");
+        assert!(marked.deleted.is_some());
+        assert_eq!(marked.metadata, initial_metadata);
+        assert_eq!(marked.config_version, initial.config_version);
+
+        address_guard.commit().await.unwrap();
+    };
+
+    let (response, ()) = tokio::join!(force_delete, orchestrate);
+    let response = response.unwrap().into_inner();
+    validate_delete_response(&response, Some(&managed_host.id), &managed_host.dpu().id);
+    for machine_id in [managed_host.id, managed_host.dpu().id] {
+        validate_machine_deletion(&env, &machine_id, None).await;
+    }
+}
+
 #[crate::sqlx_test]
 async fn test_admin_force_delete_host_with_ib_instance(pool: sqlx::PgPool) {
     let mut config = common::api_fixtures::get_config();
@@ -1045,18 +1232,38 @@ async fn test_admin_force_delete_host_with_ib_instance(pool: sqlx::PgPool) {
         .await
         .expect_err("the simulated UFM failure must stop force-delete");
     assert!(error.message().contains("simulated UFM unbind failure"));
-    assert!(
-        db::instance::find_by_id(&env.pool, tinstance.id)
-            .await
-            .unwrap()
-            .is_some(),
-        "UFM failure must not delete the Instance"
-    );
+    let retained_instance = db::instance::find_by_id(&env.pool, tinstance.id)
+        .await
+        .unwrap()
+        .expect("UFM failure must not delete the Instance");
+    let deleted_at = retained_instance
+        .deleted
+        .expect("force-delete must mark the Instance before calling UFM");
     assert!(
         retired_membership_is_recorded(&env.pool, &retired_membership).await,
         "force-delete must commit the retired membership before calling UFM"
     );
 
+    let repeated_error = env
+        .api
+        .admin_force_delete_machine(Request::new(force_delete_request(&mh.id)))
+        .await
+        .expect_err("the repeated UFM failure must stop the retry");
+    assert!(
+        repeated_error
+            .message()
+            .contains("simulated UFM unbind failure")
+    );
+    let retained_after_retry = db::instance::find_by_id(&env.pool, tinstance.id)
+        .await
+        .unwrap()
+        .expect("a repeated UFM failure must retain the Instance");
+    assert_eq!(
+        retained_after_retry.deleted.as_ref(),
+        Some(&deleted_at),
+        "a retry must preserve the original deletion timestamp",
+    );
+
     mock_fabric.set_unbind_failure(false);
 
     let response = force_delete(&env, &mh.id).await;