diff --git a/crates/api-core/src/dhcp/discover.rs b/crates/api-core/src/dhcp/discover.rs index 2f72a57272..7ab95fba2c 100644 --- a/crates/api-core/src/dhcp/discover.rs +++ b/crates/api-core/src/dhcp/discover.rs @@ -267,6 +267,14 @@ async fn ensure_dhcp_address_for_family( match existing_allocation_type { None => {} Some(AllocationType::Slaac) if address_family == IpAddressFamily::Ipv6 => { + // Take the segment lock before dropping the SLAAC row so the + // delete-then-allocate pair holds locks in the allocator order + // (segment advisory lock first, then address rows). + db::machine_interface::lock_network_segments_exclusive( + &mut *txn, + std::slice::from_ref(&segment.id), + ) + .await?; db::machine_interface_address::delete_by_interface_family( &mut *txn, machine_interface.id, diff --git a/crates/api-core/src/handlers/machine.rs b/crates/api-core/src/handlers/machine.rs index 7da5fdaad5..d717eb51d5 100644 --- a/crates/api-core/src/handlers/machine.rs +++ b/crates/api-core/src/handlers/machine.rs @@ -618,6 +618,43 @@ pub(crate) async fn admin_force_delete_machine( let mut txn = api.txn_begin().await?; let mut machines_to_clear_credentials = Vec::new(); + // Advisory-lock the admin segments before any row locks so the deletion + // follows the allocator lock order -- segment advisory lock first, then + // machine interface/address rows (the convention + // `reconcile_admin_addresses_for_host` documents). This serializes the + // deletion against the allocator, reconcile, and discovery transactions + // that hold those locks while they touch interface rows, so the two + // sides can't hold segment locks and interface rows in opposite orders. + // All admin segments, rather than a set computed from this machine's + // interfaces: the snapshots predate the BMC work above, and they omit + // BMC-typed interfaces that `force_cleanup` still row-locks. + db::machine_interface::lock_all_admin_segments(&mut txn).await?; + + // Clean up the explored tables next, in site-explorer's write order + // (`explored_managed_hosts`, then `explored_endpoints`, then interface + // rows), so this delete and a concurrent exploration pass can't hold the + // same tables in opposite orders. + if let Some(machine) = &host_machine + && let Some(addr) = machine.bmc_info.ip + { + tracing::info!("Cleaning up explored endpoint at {addr} {}", machine.id); + + // If this delete waited out a concurrent exploration rewrite, its + // statement snapshot can miss the row that rewrite re-inserted; the + // leftover clears on the next exploration pass, which rebuilds the + // table from the (now deleted) explored endpoints. + db::explored_managed_host::delete_by_host_bmc_addr(&mut txn, addr).await?; + + db::explored_endpoints::delete(&mut txn, addr).await?; + } + for dpu_machine in dpu_machines.iter() { + if let Some(addr) = dpu_machine.bmc_info.ip { + tracing::info!("Cleaning up explored endpoint at {addr} {}", dpu_machine.id); + + db::explored_endpoints::delete(&mut txn, addr).await?; + } + } + if let Some(machine) = &host_machine { if request.delete_bmc_interfaces && let Some(bmc_ip) = machine.bmc_info.ip @@ -642,14 +679,6 @@ pub(crate) async fn admin_force_delete_machine( response.host_interfaces_deleted = true; } - if let Some(addr) = machine.bmc_info.ip { - tracing::info!("Cleaning up explored endpoint at {addr} {}", machine.id); - - db::explored_endpoints::delete(&mut txn, addr).await?; - - db::explored_managed_host::delete_by_host_bmc_addr(&mut txn, addr).await?; - } - if request.delete_bmc_credentials { machines_to_clear_credentials.push(machine); } @@ -727,12 +756,6 @@ pub(crate) async fn admin_force_delete_machine( response.dpu_interfaces_deleted = true; } - if let Some(addr) = dpu_machine.bmc_info.ip { - tracing::info!("Cleaning up explored endpoint at {addr} {}", dpu_machine.id); - - db::explored_endpoints::delete(&mut txn, addr).await?; - } - if request.delete_bmc_credentials { machines_to_clear_credentials.push(dpu_machine); } diff --git a/crates/api-core/src/handlers/machine_discovery.rs b/crates/api-core/src/handlers/machine_discovery.rs index 5d111527f0..d9c81b9d3b 100644 --- a/crates/api-core/src/handlers/machine_discovery.rs +++ b/crates/api-core/src/handlers/machine_discovery.rs @@ -116,6 +116,15 @@ pub(crate) async fn discover_machine( }; let mut txn = api.txn_begin().await?; + + // Advisory-lock the admin segments before any machine-interface row + // writes in this transaction (`associate_interface_with_dpu_machine`, + // the proactive host-interface create, `set_primary_interface`), so the + // whole transaction holds locks in the allocator order (segment advisory + // lock first, then interface rows) all the way to the reconcile pass -- + // which re-acquires the same locks as a no-op. + db::machine_interface::lock_all_admin_segments(&mut txn).await?; + tracing::debug!( ?remote_ip, ?interface_id, diff --git a/crates/api-core/src/handlers/managed_host.rs b/crates/api-core/src/handlers/managed_host.rs index 9762f87deb..01965d85c5 100644 --- a/crates/api-core/src/handlers/managed_host.rs +++ b/crates/api-core/src/handlers/managed_host.rs @@ -271,6 +271,12 @@ async fn set_primary_interface_core( let mut txn = api.txn_begin().await?; + // Advisory-lock the admin segments before the `set_primary_interface` + // row writes below, so this transaction holds locks in the allocator + // order (segment advisory lock first, then interface rows) on both + // branches -- the reconcile passes re-acquire the same locks as no-ops. + db::machine_interface::lock_all_admin_segments(&mut txn).await?; + // Normalize the current admin primary's address before moving the flag, so the // active DHCP address is one reconciliation can move onto the new primary -- // but only when there IS a current admin primary to preserve. If the host has 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 995812e529..35a814f523 100644 --- a/crates/api-core/src/tests/machine_admin_force_delete.rs +++ b/crates/api-core/src/tests/machine_admin_force_delete.rs @@ -44,6 +44,7 @@ use model::hardware_info::TpmEkCertificate; use model::ib::DEFAULT_IB_FABRIC_NAME; use model::machine::machine_search_config::MachineSearchConfig; use model::machine::{InstanceState, ManagedHostState}; +use model::site_explorer::ExploredManagedHost; use sqlx::{PgConnection, Row}; use tonic::Request; @@ -276,6 +277,105 @@ async fn test_admin_force_delete_dpu_and_partially_discovered_host(pool: sqlx::P assert_eq!(iface.attached_dpu_machine_id, None); } +/// Force-deletion and a concurrent exploration pass touch the same tables; +/// this pins the lock ordering that keeps the pair deadlock-free. The +/// exploration-order transaction holds every `explored_managed_hosts` row +/// (`explored_managed_host::update` opens with a full-table delete) while +/// force-delete runs, then touches one of the host's `machine_interfaces` +/// rows -- the two-table cycle captured from CI. Force-delete takes the +/// explored tables before any interface rows, so it blocks cleanly on the +/// exploration transaction instead of deadlocking against it. +#[crate::sqlx_test] +async fn test_admin_force_delete_orders_locks_against_exploration(pool: sqlx::PgPool) { + let env = create_test_env(pool).await; + let host = create_managed_host(&env).await; + + // The host's BMC ip and one interface id, plus an `explored_managed_hosts` + // row for the BMC ip so the delete has a row to contend on. + let mut txn = env.pool.begin().await.unwrap(); + let machine = db::machine::find_one(txn.as_mut(), &host.id, MachineSearchConfig::default()) + .await + .unwrap() + .unwrap(); + let bmc_ip = machine + .bmc_info + .ip + .expect("managed host fixture has a BMC ip"); + let interface_id = machine.interfaces[0].id; + let explored_host = ExploredManagedHost { + host_bmc_ip: bmc_ip, + dpus: Vec::new(), + }; + db::explored_managed_host::update(txn.as_mut(), &[&explored_host]) + .await + .unwrap(); + txn.commit().await.unwrap(); + + // Exploration-order transaction: hold the explored_managed_hosts rows + // first, exactly like site-explorer's persistence pass. + let mut exploration_txn = env.pool.begin().await.unwrap(); + db::explored_managed_host::update(exploration_txn.as_mut(), &[&explored_host]) + .await + .unwrap(); + + // Launch the force-delete and wait until it blocks on those rows. + let api = host.api.clone(); + let host_id = host.id; + let force_delete_task = tokio::spawn(async move { + api.admin_force_delete_machine(tonic::Request::new(AdminForceDeleteMachineRequest { + host_query: host_id.to_string(), + delete_interfaces: true, + delete_bmc_interfaces: true, + delete_bmc_credentials: false, + allow_delete_with_orphaned_dpf_crds: false, + })) + .await + }); + wait_until_blocked_on(&env.pool, "explored_managed_hosts").await; + + // Now take the machine_interfaces row exploration touches second (the + // identity UPDATE only exists for its row lock). If force-delete already + // held interface rows here, this pair would deadlock with a 40P01. + sqlx::query("UPDATE machine_interfaces SET id = id WHERE id = $1") + .bind(interface_id) + .execute(exploration_txn.as_mut()) + .await + .expect("exploration-order interface update must not deadlock against force-delete"); + exploration_txn.commit().await.unwrap(); + + let response = force_delete_task + .await + .unwrap() + .expect("force delete completes once exploration commits") + .into_inner(); + assert!(response.all_done); + validate_machine_deletion(&env, &host.dpu_ids[0], None).await; +} + +/// Polls `pg_stat_activity` until some backend in this test's database sits +/// in a lock wait on a query that names `relation`. The `datname` filter +/// keeps parallel per-test databases on the shared server out of the match, +/// and the monitor query receives the relation as a bind parameter, so it +/// never matches its own text. The generous cap covers the force-delete +/// RPC's pre-transaction work (Redfish attempts against the fixture BMC) +/// on slow CI runners. +async fn wait_until_blocked_on(pool: &sqlx::PgPool, relation: &str) { + for _ in 0..600 { + let waiting: i64 = sqlx::query_scalar( + "SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND wait_event_type = 'Lock' AND query ILIKE '%' || $1 || '%'", + ) + .bind(relation) + .fetch_one(pool) + .await + .unwrap(); + if waiting > 0 { + return; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } + panic!("force delete never blocked on {relation}"); +} + async fn force_delete( env: &TestEnv, machine_id: &MachineId, diff --git a/crates/api-db/src/machine_interface.rs b/crates/api-db/src/machine_interface.rs index 09e1ad1e53..1275d6a985 100644 --- a/crates/api-db/src/machine_interface.rs +++ b/crates/api-db/src/machine_interface.rs @@ -1860,12 +1860,44 @@ async fn lock_network_segment_exclusive( txn: &mut PgTransaction<'_>, segment: &NetworkSegment, ) -> DatabaseResult<()> { - let query = "SELECT pg_advisory_xact_lock(hashtextextended($1::text, 0))"; - sqlx::query_scalar(query) - .bind(format!("network_segment.{}", segment.id)) - .fetch_one(txn.as_mut()) - .await - .map_err(|e| DatabaseError::query(query, e)) + lock_network_segments_exclusive(txn.as_mut(), std::slice::from_ref(&segment.id)).await +} + +/// Advisory-lock every segment in `segment_ids`, in ascending id order -- +/// the allocator convention: segment advisory lock first, then machine +/// interface/address row locks. This is the one home for the lock key and +/// ordering; every segment-lock helper funnels through it. Must run inside a +/// transaction: the locks are `pg_advisory_xact_lock`-scoped and release on +/// commit or rollback. +pub async fn lock_network_segments_exclusive( + txn: &mut PgConnection, + segment_ids: &[NetworkSegmentId], +) -> DatabaseResult<()> { + let mut ids = segment_ids.to_vec(); + ids.sort_unstable(); + ids.dedup(); + for id in ids { + let query = "SELECT pg_advisory_xact_lock(hashtextextended($1::text, 0))"; + sqlx::query_scalar::<_, ()>(query) + .bind(format!("network_segment.{id}")) + .fetch_one(&mut *txn) + .await + .map_err(|e| DatabaseError::query(query, e))?; + } + Ok(()) +} + +/// Advisory-lock every admin segment, in ascending id order. Transactions +/// that touch machine-interface rows before their segment locks -- the flows +/// that end in `reconcile_admin_addresses_for_host`, and machine teardown, +/// which deletes interface rows wholesale -- call this right after opening +/// the transaction so the whole transaction follows the allocator order. +/// Later acquisitions of the same locks in the same transaction (reconcile's +/// own pass) are no-ops. +pub async fn lock_all_admin_segments(txn: &mut PgConnection) -> DatabaseResult<()> { + let segment_ids = + db_network_segment::list_segment_ids(&mut *txn, Some(NetworkSegmentType::Admin)).await?; + lock_network_segments_exclusive(txn, &segment_ids).await } pub async fn allocate_svi_ip( @@ -2656,9 +2688,7 @@ async fn load_and_lock_all_admin_segments( ))); } - for segment in &segments { - lock_network_segment_exclusive(txn.as_mut(), segment).await?; - } + lock_network_segments_exclusive(txn.as_pgconn(), &segment_ids).await?; Ok(segments) } diff --git a/crates/site-explorer/src/machine_creator.rs b/crates/site-explorer/src/machine_creator.rs index c22d13b885..9605c74206 100644 --- a/crates/site-explorer/src/machine_creator.rs +++ b/crates/site-explorer/src/machine_creator.rs @@ -157,6 +157,13 @@ impl MachineCreator { let mut txn = Transaction::begin(pool).await?; + // Advisory-lock the admin segments before any machine-interface row + // writes (`attach_dpu_to_host` / `configure_dpu_interface`), so this + // transaction holds locks in the allocator order (segment advisory + // lock first, then interface rows) all the way to the reconcile + // pass -- which re-acquires the same locks as a no-op. + db::machine_interface::lock_all_admin_segments(txn.as_pgconn()).await?; + // Zero-dpu case: If the explored host had no DPUs, we can create the machine now if managed_host.explored_host.dpus.is_empty() { if let Some(machine_id) = self