fix(dhcp): resync hostname on static assign/remove-address (#3383)

crates/api-core/src/dhcp/expire.rs

@@ -66,32 +66,42 @@ 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?
+            .await?;
+            (!owners.is_empty(), owners)
         }
     };
 
     // Sync the hostname to the remaining address state so DNS stays
     // 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?;

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();
@@ -174,15 +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_owners = db::machine_interface_address::delete_by_address(
         &mut txn,
         ip_address,
         AllocationType::Static,
     )
     .await?;
+
+    // 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.
+    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 deleted {
+    let status = if !removed_owners.is_empty() {
         tracing::info!(%interface_id, %ip_address, "Removed static address");
         rpc::RemoveStaticAddressStatus::Removed
     } else {

crates/api-core/tests/integration/static_address_management.rs

@@ -452,6 +452,155 @@ 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<dyn std::error::Error>> {
+    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(())
+}
+
+/// 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<dyn std::error::Error>> {
+    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]

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_empty(),
         "DHCP-scoped expiry delete should not match a Static address"
     );
     assert_eq!(

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 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<bool, DatabaseError> {
-    let query =
-        "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2";
-    sqlx::query(query)
+) -> Result<Vec<MachineInterfaceId>, DatabaseError> {
+    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)
-        .execute(txn)
+        .fetch_all(txn)
         .await
-        .map(|r| r.rows_affected() > 0)
         .map_err(|e| DatabaseError::query(query, e))
 }