From a3ea39f42fe8425112fab2f3ddad625e5fb650a4 Mon Sep 17 00:00:00 2001 From: Amir Hatamzad Date: Tue, 14 Jul 2026 15:06:46 -0700 Subject: [PATCH 1/5] fix(dhcp): resync hostname on static assign/remove-address (#3383) assign-address and remove-address mutated the interface address but skipped the hostname resync every other allocation path runs, leaving a stale IP-derived hostname that collided with fqdn_must_be_unique and wedged DHCP allocation on the whole segment. --- .../src/handlers/machine_interface_address.rs | 19 +++++ .../integration/static_address_management.rs | 76 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/crates/api-core/src/handlers/machine_interface_address.rs b/crates/api-core/src/handlers/machine_interface_address.rs index 2f197ee187..17524e46e4 100644 --- a/crates/api-core/src/handlers/machine_interface_address.rs +++ b/crates/api-core/src/handlers/machine_interface_address.rs @@ -151,6 +151,17 @@ pub async fn assign_static_address( ); } + // Keep the interface's IP-derived hostname/domain consistent with the new + // address, matching every other assignment path. Skipping this leaves a + // stale hostname that encodes a now-freed IP and collides with the + // fqdn_must_be_unique constraint when the allocator reuses that IP. + db::machine_interface::sync_hostname_after_address_assignment( + &mut txn, + interface_id, + target_segment.config.subdomain_id, + ) + .await?; + txn.commit().await?; let status: rpc::AssignStaticAddressStatus = result.into(); @@ -180,6 +191,14 @@ pub async fn remove_static_address( AllocationType::Static, ) .await?; + + // Re-derive the interface's hostname/domain now that the address is gone, + // matching the DHCP lease-expiry path. Without this the interface keeps a + // hostname pinned to the removed IP. + if deleted { + db::machine_interface::sync_hostname_after_address_change(&mut txn, interface_id).await?; + } + txn.commit().await?; let status = if deleted { diff --git a/crates/api-core/tests/integration/static_address_management.rs b/crates/api-core/tests/integration/static_address_management.rs index 371ad5c65d..d17f75b866 100644 --- a/crates/api-core/tests/integration/static_address_management.rs +++ b/crates/api-core/tests/integration/static_address_management.rs @@ -452,6 +452,82 @@ async fn test_assign_remove_then_dhcp_reallocates( Ok(()) } +/// Regression for #3383: assign-address and remove-address must keep the +/// interface's IP-derived hostname consistent with its address, like every +/// other allocation path. A stale hostname encoding a now-freed IP collides +/// with the fqdn_must_be_unique constraint and wedges DHCP allocation on the +/// whole segment. +#[sqlx_test] +async fn test_assign_and_remove_resync_hostname( + pool: PgPool, +) -> Result<(), Box> { + let StaticAddressTestEnv { + env, admin_segment, .. + } = init(pool).await; + let mac = MacAddress::from_str("aa:bb:cc:dd:ee:17").unwrap(); + let mac_str = mac.to_string(); + + // Let the interface get a DHCP address; its hostname is derived from it. + let discover_resp = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(&mac_str, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let dhcp_ip = discover_resp.address.clone(); + let interface_id = discover_resp.machine_interface_id.unwrap(); + + let mut txn = env.db_txn().await; + let iface = db::machine_interface::find_one(&mut *txn, interface_id).await?; + assert_eq!( + iface.hostname, + dhcp_ip.replace('.', "-"), + "hostname should be derived from the DHCP address" + ); + txn.commit().await?; + + // Move it to a different static IP (same family, so it replaces the DHCP + // one). The hostname must follow the new address, not stay pinned to the + // now-freed DHCP IP. + let static_ip = "192.0.2.217"; + env.api() + .assign_static_address(Request::new(AssignStaticAddressRequest { + interface_id: Some(interface_id), + ip_address: static_ip.to_string(), + })) + .await?; + + let mut txn = env.db_txn().await; + let iface_after_assign = db::machine_interface::find_one(&mut *txn, interface_id).await?; + assert_eq!( + iface_after_assign.hostname, + static_ip.replace('.', "-"), + "hostname should resync to the newly assigned static address" + ); + txn.commit().await?; + + // Remove the static address; with no addresses left the hostname must reset + // to the dormant placeholder rather than keep encoding the freed IP. + env.api() + .remove_static_address(Request::new(RemoveStaticAddressRequest { + interface_id: Some(interface_id), + ip_address: static_ip.to_string(), + })) + .await?; + + let mut txn = env.db_txn().await; + let iface_after_remove = db::machine_interface::find_one(&mut *txn, interface_id).await?; + assert!( + iface_after_remove.hostname.to_lowercase().starts_with("noip"), + "hostname should reset to dormant format after removal, got: {}", + iface_after_remove.hostname, + ); + txn.commit().await?; + + Ok(()) +} + /// When assigning a static IP that's within a managed segment's prefix, /// the interface's segment_id should be updated to that segment. #[sqlx_test] From cb1842ecad67f12796311b6366ca48a4ae1099d7 Mon Sep 17 00:00:00 2001 From: Amir Hatamzad Date: Tue, 14 Jul 2026 21:01:25 -0700 Subject: [PATCH 2/5] fix(dhcp): resync the address owner on remove; fix fmt (#3383) Address review feedback: remove-address deletes by IP, so resync the interface returned by delete_by_address instead of the request's interface_id, which may not own the row. Also wrap an assert to satisfy nightly rustfmt. --- crates/api-core/src/dhcp/expire.rs | 1 + .../src/handlers/machine_interface_address.rs | 15 ++-- .../integration/static_address_management.rs | 71 ++++++++++++++++++- crates/api-db/src/machine_interface/tests.rs | 2 +- .../api-db/src/machine_interface_address.rs | 15 ++-- 5 files changed, 88 insertions(+), 16 deletions(-) diff --git a/crates/api-core/src/dhcp/expire.rs b/crates/api-core/src/dhcp/expire.rs index e403bb3b60..90f321c76a 100644 --- a/crates/api-core/src/dhcp/expire.rs +++ b/crates/api-core/src/dhcp/expire.rs @@ -83,6 +83,7 @@ pub async fn expire_dhcp_lease( model::allocation_type::AllocationType::Dhcp, ) .await? + .is_some() } }; diff --git a/crates/api-core/src/handlers/machine_interface_address.rs b/crates/api-core/src/handlers/machine_interface_address.rs index 17524e46e4..a43ab4df47 100644 --- a/crates/api-core/src/handlers/machine_interface_address.rs +++ b/crates/api-core/src/handlers/machine_interface_address.rs @@ -185,23 +185,24 @@ pub async fn remove_static_address( let ip_address: std::net::IpAddr = req.ip_address.parse()?; let mut txn = api.txn_begin().await?; - let deleted = db::machine_interface_address::delete_by_address( + let removed_owner = db::machine_interface_address::delete_by_address( &mut txn, ip_address, AllocationType::Static, ) .await?; - // Re-derive the interface's hostname/domain now that the address is gone, - // matching the DHCP lease-expiry path. Without this the interface keeps a - // hostname pinned to the removed IP. - if deleted { - db::machine_interface::sync_hostname_after_address_change(&mut txn, interface_id).await?; + // Re-derive the hostname/domain of the interface that actually owned the + // removed address, matching the DHCP lease-expiry path. delete_by_address + // deletes by IP, so the owner may differ from the request's interface_id; + // resyncing the wrong one would leave the real owner with a stale hostname. + if let Some(owner_id) = removed_owner { + db::machine_interface::sync_hostname_after_address_change(&mut txn, owner_id).await?; } txn.commit().await?; - let status = if deleted { + let status = if removed_owner.is_some() { tracing::info!(%interface_id, %ip_address, "Removed static address"); rpc::RemoveStaticAddressStatus::Removed } else { diff --git a/crates/api-core/tests/integration/static_address_management.rs b/crates/api-core/tests/integration/static_address_management.rs index d17f75b866..bf57bac133 100644 --- a/crates/api-core/tests/integration/static_address_management.rs +++ b/crates/api-core/tests/integration/static_address_management.rs @@ -519,7 +519,10 @@ async fn test_assign_and_remove_resync_hostname( let mut txn = env.db_txn().await; let iface_after_remove = db::machine_interface::find_one(&mut *txn, interface_id).await?; assert!( - iface_after_remove.hostname.to_lowercase().starts_with("noip"), + iface_after_remove + .hostname + .to_lowercase() + .starts_with("noip"), "hostname should reset to dormant format after removal, got: {}", iface_after_remove.hostname, ); @@ -528,6 +531,72 @@ async fn test_assign_and_remove_resync_hostname( Ok(()) } +/// Regression for #3383 (review follow-up): remove-address deletes by IP, so the +/// interface that owned the removed address may differ from the caller-supplied +/// interface_id. The resync must target the actual owner, otherwise the real +/// owner keeps a hostname pinned to the freed IP. +#[sqlx_test] +async fn test_remove_resyncs_the_address_owner_not_the_request_id( + pool: PgPool, +) -> Result<(), Box> { + let StaticAddressTestEnv { + env, admin_segment, .. + } = init(pool).await; + + // Owner interface: give it a static address so its hostname is IP-derived. + let owner_mac = MacAddress::from_str("aa:bb:cc:dd:ee:18").unwrap().to_string(); + let owner = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(&owner_mac, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let owner_id = owner.machine_interface_id.unwrap(); + let owned_ip = "192.0.2.218"; + env.api() + .assign_static_address(Request::new(AssignStaticAddressRequest { + interface_id: Some(owner_id), + ip_address: owned_ip.to_string(), + })) + .await?; + + // A second, unrelated interface whose id we will (incorrectly) pass to + // remove-address. + let other_mac = MacAddress::from_str("aa:bb:cc:dd:ee:19").unwrap().to_string(); + let other = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(&other_mac, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let other_id = other.machine_interface_id.unwrap(); + + // Remove the owner's IP but pass the *other* interface's id. + let resp = env + .api() + .remove_static_address(Request::new(RemoveStaticAddressRequest { + interface_id: Some(other_id), + ip_address: owned_ip.to_string(), + })) + .await? + .into_inner(); + assert_eq!(resp.status(), RemoveStaticAddressStatus::Removed); + + // The owner (which actually lost the address) must be resynced to dormant. + let mut txn = env.db_txn().await; + let owner_after = db::machine_interface::find_one(&mut *txn, owner_id).await?; + assert!( + owner_after.hostname.to_lowercase().starts_with("noip"), + "the address owner should be resynced to dormant, got: {}", + owner_after.hostname, + ); + txn.commit().await?; + + Ok(()) +} + /// When assigning a static IP that's within a managed segment's prefix, /// the interface's segment_id should be updated to that segment. #[sqlx_test] diff --git a/crates/api-db/src/machine_interface/tests.rs b/crates/api-db/src/machine_interface/tests.rs index 44e19b8605..8ec51182c1 100644 --- a/crates/api-db/src/machine_interface/tests.rs +++ b/crates/api-db/src/machine_interface/tests.rs @@ -303,7 +303,7 @@ async fn test_retain_bmc_address_pins_dhcp_and_survives_expiry( crate::machine_interface_address::find_for_interface(txn.as_pgconn(), interface_id).await?; txn.commit().await?; assert!( - !deleted, + deleted.is_none(), "DHCP-scoped expiry delete should not match a Static address" ); assert_eq!( diff --git a/crates/api-db/src/machine_interface_address.rs b/crates/api-db/src/machine_interface_address.rs index b38b6f9d6d..ea4c6d19a6 100644 --- a/crates/api-db/src/machine_interface_address.rs +++ b/crates/api-db/src/machine_interface_address.rs @@ -203,21 +203,22 @@ pub async fn assign_static( Ok(result) } -/// Delete an address allocation of the given type. Returns true if a -/// matching allocation was found and deleted, false otherwise. +/// Delete an address allocation of the given type. Returns the interface that +/// owned the deleted address, or `None` if no matching allocation was found, so +/// callers can resync that interface's hostname rather than guessing the owner. pub async fn delete_by_address( txn: &mut PgConnection, address: IpAddr, allocation_type: AllocationType, -) -> Result { +) -> Result, DatabaseError> { let query = - "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2"; - sqlx::query(query) + "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2 RETURNING interface_id"; + sqlx::query_scalar(query) .bind(address) .bind(allocation_type) - .execute(txn) + .fetch_all(txn) .await - .map(|r| r.rows_affected() > 0) + .map(|owners| owners.into_iter().next()) .map_err(|e| DatabaseError::query(query, e)) } From c904fedcd6e00a7be9b5c883f1399f1e86860f5d Mon Sep 17 00:00:00 2001 From: Amir Hatamzad Date: Tue, 14 Jul 2026 21:19:08 -0700 Subject: [PATCH 3/5] fix(dhcp): resync all owners of a removed address (#3383) delete_by_address can match multiple rows since (address, allocation_type) is not unique on its own; return every affected interface and resync each instead of dropping all but the first. --- crates/api-core/src/dhcp/expire.rs | 4 ++-- .../src/handlers/machine_interface_address.rs | 10 +++++----- crates/api-db/src/machine_interface/tests.rs | 2 +- crates/api-db/src/machine_interface_address.rs | 11 ++++++----- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/api-core/src/dhcp/expire.rs b/crates/api-core/src/dhcp/expire.rs index 90f321c76a..fb225e7e69 100644 --- a/crates/api-core/src/dhcp/expire.rs +++ b/crates/api-core/src/dhcp/expire.rs @@ -77,13 +77,13 @@ pub async fn expire_dhcp_lease( .await? } None => { - db::machine_interface_address::delete_by_address( + !db::machine_interface_address::delete_by_address( &mut txn, ip_address, model::allocation_type::AllocationType::Dhcp, ) .await? - .is_some() + .is_empty() } }; diff --git a/crates/api-core/src/handlers/machine_interface_address.rs b/crates/api-core/src/handlers/machine_interface_address.rs index a43ab4df47..4fd9c6876a 100644 --- a/crates/api-core/src/handlers/machine_interface_address.rs +++ b/crates/api-core/src/handlers/machine_interface_address.rs @@ -185,24 +185,24 @@ pub async fn remove_static_address( let ip_address: std::net::IpAddr = req.ip_address.parse()?; let mut txn = api.txn_begin().await?; - let removed_owner = db::machine_interface_address::delete_by_address( + let removed_owners = db::machine_interface_address::delete_by_address( &mut txn, ip_address, AllocationType::Static, ) .await?; - // Re-derive the hostname/domain of the interface that actually owned the + // Re-derive the hostname/domain of every interface that actually owned a // removed address, matching the DHCP lease-expiry path. delete_by_address // deletes by IP, so the owner may differ from the request's interface_id; // resyncing the wrong one would leave the real owner with a stale hostname. - if let Some(owner_id) = removed_owner { - db::machine_interface::sync_hostname_after_address_change(&mut txn, owner_id).await?; + for owner_id in &removed_owners { + db::machine_interface::sync_hostname_after_address_change(&mut txn, *owner_id).await?; } txn.commit().await?; - let status = if removed_owner.is_some() { + let status = if !removed_owners.is_empty() { tracing::info!(%interface_id, %ip_address, "Removed static address"); rpc::RemoveStaticAddressStatus::Removed } else { diff --git a/crates/api-db/src/machine_interface/tests.rs b/crates/api-db/src/machine_interface/tests.rs index 8ec51182c1..678b4c0cf9 100644 --- a/crates/api-db/src/machine_interface/tests.rs +++ b/crates/api-db/src/machine_interface/tests.rs @@ -303,7 +303,7 @@ async fn test_retain_bmc_address_pins_dhcp_and_survives_expiry( crate::machine_interface_address::find_for_interface(txn.as_pgconn(), interface_id).await?; txn.commit().await?; assert!( - deleted.is_none(), + deleted.is_empty(), "DHCP-scoped expiry delete should not match a Static address" ); assert_eq!( diff --git a/crates/api-db/src/machine_interface_address.rs b/crates/api-db/src/machine_interface_address.rs index ea4c6d19a6..1ea1b96cba 100644 --- a/crates/api-db/src/machine_interface_address.rs +++ b/crates/api-db/src/machine_interface_address.rs @@ -203,14 +203,16 @@ pub async fn assign_static( Ok(result) } -/// Delete an address allocation of the given type. Returns the interface that -/// owned the deleted address, or `None` if no matching allocation was found, so -/// callers can resync that interface's hostname rather than guessing the owner. +/// Delete an address allocation of the given type. Returns the interfaces that +/// owned the deleted addresses (normally one, empty if nothing matched) so +/// callers can resync each one's hostname rather than guessing the owner. The +/// delete removes every matching row, so all owners are returned — not just the +/// first — since `(address, allocation_type)` is not unique on its own. pub async fn delete_by_address( txn: &mut PgConnection, address: IpAddr, allocation_type: AllocationType, -) -> Result, DatabaseError> { +) -> Result, DatabaseError> { let query = "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2 RETURNING interface_id"; sqlx::query_scalar(query) @@ -218,7 +220,6 @@ pub async fn delete_by_address( .bind(allocation_type) .fetch_all(txn) .await - .map(|owners| owners.into_iter().next()) .map_err(|e| DatabaseError::query(query, e)) } From 987bec4bb1d73577a7764232fee935c8df9e0fc0 Mon Sep 17 00:00:00 2001 From: Amir Hatamzad Date: Tue, 14 Jul 2026 21:56:45 -0700 Subject: [PATCH 4/5] fix(dhcp): resync all deleted owners on lease expiry; fix nightly fmt (#3383) Address review feedback: the DHCP lease-expiry path now resyncs every interface returned by delete_by_address instead of only the find_by_ip owner. Also apply nightly rustfmt to the query literal and test setup. --- crates/api-core/src/dhcp/expire.rs | 25 +++++++++++++------ .../integration/static_address_management.rs | 8 ++++-- .../api-db/src/machine_interface_address.rs | 3 +-- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/crates/api-core/src/dhcp/expire.rs b/crates/api-core/src/dhcp/expire.rs index fb225e7e69..a5c0d69602 100644 --- a/crates/api-core/src/dhcp/expire.rs +++ b/crates/api-core/src/dhcp/expire.rs @@ -66,24 +66,33 @@ pub async fn expire_dhcp_lease( // pair. Otherwise, just call the address-only variant, which would // be something we would see from an admin-cli call used for deleting // a specific IP allocation. - let deleted = match mac_address { + // Determine which interfaces need a hostname resync after the delete. The + // mac-scoped variant targets a single (ip, mac) row, so resync the interface + // looked up above; the address-only variant can match multiple rows, so + // resync every owner it reports rather than dropping all but one. + let (deleted, resync_targets) = match mac_address { Some(mac) => { - db::machine_interface_address::delete_by_address_and_mac( + let deleted = db::machine_interface_address::delete_by_address_and_mac( &mut txn, ip_address, mac, model::allocation_type::AllocationType::Dhcp, ) - .await? + .await?; + let targets = match (deleted, &interface) { + (true, Some(iface)) => vec![iface.id], + _ => Vec::new(), + }; + (deleted, targets) } None => { - !db::machine_interface_address::delete_by_address( + let owners = db::machine_interface_address::delete_by_address( &mut txn, ip_address, model::allocation_type::AllocationType::Dhcp, ) - .await? - .is_empty() + .await?; + (!owners.is_empty(), owners) } }; @@ -91,8 +100,8 @@ pub async fn expire_dhcp_lease( // consistent: the IP style re-derives (and re-derives again from the next // allocated IP on rediscovery); the other styles keep their names and only // drop out of DNS while addressless. - if deleted && let Some(iface) = interface { - db::machine_interface::sync_hostname_after_address_change(&mut txn, iface.id).await?; + for iface_id in &resync_targets { + db::machine_interface::sync_hostname_after_address_change(&mut txn, *iface_id).await?; } txn.commit().await?; diff --git a/crates/api-core/tests/integration/static_address_management.rs b/crates/api-core/tests/integration/static_address_management.rs index bf57bac133..1ef9474f98 100644 --- a/crates/api-core/tests/integration/static_address_management.rs +++ b/crates/api-core/tests/integration/static_address_management.rs @@ -544,7 +544,9 @@ async fn test_remove_resyncs_the_address_owner_not_the_request_id( } = init(pool).await; // Owner interface: give it a static address so its hostname is IP-derived. - let owner_mac = MacAddress::from_str("aa:bb:cc:dd:ee:18").unwrap().to_string(); + let owner_mac = MacAddress::from_str("aa:bb:cc:dd:ee:18") + .unwrap() + .to_string(); let owner = env .api() .discover_dhcp( @@ -563,7 +565,9 @@ async fn test_remove_resyncs_the_address_owner_not_the_request_id( // A second, unrelated interface whose id we will (incorrectly) pass to // remove-address. - let other_mac = MacAddress::from_str("aa:bb:cc:dd:ee:19").unwrap().to_string(); + let other_mac = MacAddress::from_str("aa:bb:cc:dd:ee:19") + .unwrap() + .to_string(); let other = env .api() .discover_dhcp( diff --git a/crates/api-db/src/machine_interface_address.rs b/crates/api-db/src/machine_interface_address.rs index 1ea1b96cba..eaf7e463b5 100644 --- a/crates/api-db/src/machine_interface_address.rs +++ b/crates/api-db/src/machine_interface_address.rs @@ -213,8 +213,7 @@ pub async fn delete_by_address( address: IpAddr, allocation_type: AllocationType, ) -> Result, DatabaseError> { - let query = - "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2 RETURNING interface_id"; + let query = "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2 RETURNING interface_id"; sqlx::query_scalar(query) .bind(address) .bind(allocation_type) From 448d710d9a828255264273e4a4b6c04e9293030b Mon Sep 17 00:00:00 2001 From: Amir Hatamzad Date: Wed, 15 Jul 2026 08:52:30 -0700 Subject: [PATCH 5/5] fix(dhcp): derive expiry resync target from the deleted row (#3383) Address review feedback: delete_by_address_and_mac now returns the affected interface_id(s) via RETURNING, and expire_dhcp_lease resyncs those authoritative owners, dropping the separate find_by_ip lookup that could resync the wrong interface. Add a mac-scoped ownership test. --- crates/api-core/src/dhcp/expire.rs | 36 ++++------ .../integration/dhcp_lease_expiration.rs | 67 +++++++++++++++++++ .../api-db/src/machine_interface_address.rs | 14 ++-- 3 files changed, 87 insertions(+), 30 deletions(-) diff --git a/crates/api-core/src/dhcp/expire.rs b/crates/api-core/src/dhcp/expire.rs index a5c0d69602..2f18e8e31f 100644 --- a/crates/api-core/src/dhcp/expire.rs +++ b/crates/api-core/src/dhcp/expire.rs @@ -57,44 +57,32 @@ pub async fn expire_dhcp_lease( let mut txn = api.txn_begin().await?; - // Look up the interface that owns this IP before deleting so we can clear - // its hostname afterward. The JOIN in find_by_ip requires the address row - // to still exist, so we must do this before the delete. - let interface = db::machine_interface::find_by_ip(&mut txn, ip_address).await?; - - // When the caller provides the MAC, scope the delete to the (ip, mac) - // pair. Otherwise, just call the address-only variant, which would - // be something we would see from an admin-cli call used for deleting - // a specific IP allocation. - // Determine which interfaces need a hostname resync after the delete. The - // mac-scoped variant targets a single (ip, mac) row, so resync the interface - // looked up above; the address-only variant can match multiple rows, so - // resync every owner it reports rather than dropping all but one. - let (deleted, resync_targets) = match mac_address { + // When the caller provides the MAC, scope the delete to the (ip, mac) pair. + // Otherwise use the address-only variant, which is what an admin-cli call + // deleting a specific IP allocation would hit. Either way, both variants + // return the interfaces whose rows were actually deleted, so we resync those + // authoritative owners rather than a separately looked-up interface (which + // could differ if ownership changed or multiple rows share the address). + let resync_targets = match mac_address { Some(mac) => { - let deleted = db::machine_interface_address::delete_by_address_and_mac( + db::machine_interface_address::delete_by_address_and_mac( &mut txn, ip_address, mac, model::allocation_type::AllocationType::Dhcp, ) - .await?; - let targets = match (deleted, &interface) { - (true, Some(iface)) => vec![iface.id], - _ => Vec::new(), - }; - (deleted, targets) + .await? } None => { - let owners = db::machine_interface_address::delete_by_address( + db::machine_interface_address::delete_by_address( &mut txn, ip_address, model::allocation_type::AllocationType::Dhcp, ) - .await?; - (!owners.is_empty(), owners) + .await? } }; + let deleted = !resync_targets.is_empty(); // Sync the hostname to the remaining address state so DNS stays // consistent: the IP style re-derives (and re-derives again from the next diff --git a/crates/api-core/tests/integration/dhcp_lease_expiration.rs b/crates/api-core/tests/integration/dhcp_lease_expiration.rs index 0fde7e2126..a85298bdf1 100644 --- a/crates/api-core/tests/integration/dhcp_lease_expiration.rs +++ b/crates/api-core/tests/integration/dhcp_lease_expiration.rs @@ -537,3 +537,70 @@ async fn test_expire_with_mismatched_mac_is_no_op( Ok(()) } + +/// Regression for #3383 (review follow-up): the MAC-scoped expiry path must +/// resync the interface that actually owned the deleted (ip, mac) row — derived +/// from the delete itself — and leave other interfaces on the segment untouched. +#[sqlx_test] +async fn test_mac_scoped_expiry_resyncs_only_the_owner( + pool: PgPool, +) -> Result<(), Box> { + let (env, admin_segment) = init(pool).await; + let owner_mac = "aa:bb:cc:dd:ee:20"; + let other_mac = "aa:bb:cc:dd:ee:21"; + + // Two interfaces, each with its own DHCP address and IP-derived hostname. + let owner = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(owner_mac, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let owner_ip = owner.address.clone(); + let owner_id = owner.machine_interface_id.unwrap(); + + let other = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(other_mac, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let other_id = other.machine_interface_id.unwrap(); + + let mut txn = env.db_txn().await; + let other_hostname_before = db::machine_interface::find_one(&mut *txn, other_id) + .await? + .hostname; + txn.commit().await?; + + // Expire the owner's lease scoped by (ip, mac). + let expire_response = env + .api() + .expire_dhcp_lease(Request::new(ExpireDhcpLeaseRequest { + ip_address: owner_ip, + mac_address: Some(owner_mac.to_string()), + })) + .await? + .into_inner(); + assert_eq!(expire_response.status(), ExpireDhcpLeaseStatus::Released); + + // The owner is resynced to the dormant placeholder; the other interface is + // left exactly as it was. + let mut txn = env.db_txn().await; + let owner_after = db::machine_interface::find_one(&mut *txn, owner_id).await?; + let other_after = db::machine_interface::find_one(&mut *txn, other_id).await?; + txn.commit().await?; + assert!( + owner_after.hostname.to_lowercase().starts_with("noip"), + "the (ip, mac) owner should be resynced to dormant, got: {}", + owner_after.hostname, + ); + assert_eq!( + other_after.hostname, other_hostname_before, + "an unrelated interface must not be resynced" + ); + + Ok(()) +} diff --git a/crates/api-db/src/machine_interface_address.rs b/crates/api-db/src/machine_interface_address.rs index eaf7e463b5..d650e42d50 100644 --- a/crates/api-db/src/machine_interface_address.rs +++ b/crates/api-db/src/machine_interface_address.rs @@ -225,26 +225,28 @@ pub async fn delete_by_address( /// Delete an address allocation for a given (ip, mac) pair, which /// of course only actually deletes when the pair matches. /// -/// Returns true if a matching allocation was found and deleted. +/// Returns the interfaces that owned the deleted allocations (normally one, +/// empty if the pair matched nothing) so callers can resync each one's hostname +/// against the authoritative deleted rows rather than a separate lookup. pub async fn delete_by_address_and_mac( txn: &mut PgConnection, address: IpAddr, mac_address: mac_address::MacAddress, allocation_type: AllocationType, -) -> Result { +) -> Result, DatabaseError> { let query = "DELETE FROM machine_interface_addresses mia USING machine_interfaces mi WHERE mia.interface_id = mi.id AND mia.address = $1::inet AND mia.allocation_type = $2 - AND mi.mac_address = $3::macaddr"; - sqlx::query(query) + AND mi.mac_address = $3::macaddr + RETURNING mia.interface_id"; + sqlx::query_scalar(query) .bind(address) .bind(allocation_type) .bind(mac_address) - .execute(txn) + .fetch_all(txn) .await - .map(|r| r.rows_affected() > 0) .map_err(|e| DatabaseError::query(query, e)) }