diff --git a/crates/api-core/src/handlers/instance.rs b/crates/api-core/src/handlers/instance.rs index 5796e94f2b..123cc5e017 100644 --- a/crates/api-core/src/handlers/instance.rs +++ b/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?; diff --git a/crates/api-core/src/tests/machine_admin_force_delete.rs b/crates/api-core/src/tests/machine_admin_force_delete.rs index fca4805321..9693d288cc 100644 --- a/crates/api-core/src/tests/machine_admin_force_delete.rs +++ b/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 = 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;