fix(dhcp): add expiry for declined IPs to prevent pool exhaustion by AprilNEA · Pull Request #152 · arcboxlabs/arcbox

AprilNEA · 2026-03-31T12:38:37Z

Summary

Add 5-minute quarantine for DHCP DECLINE'd IPs instead of permanent allocation
Track declined IPs with timestamps in declined_ips: HashMap<Ipv4Addr, Instant>
Release quarantined IPs back to pool during cleanup_expired_leases()

Test plan

Unit test: test_declined_ip_quarantine_and_expiry

Closes ABX-235

…e lock

linear · 2026-03-31T12:38:41Z

ABX-235 DHCP DECLINE permanently consumes IP addresses from pool

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21b37fea8d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-31T12:40:46Z

+        for ip in expired_declines {
+            self.declined_ips.remove(&ip);
+            self.allocator.release(ip);


Preserve reservations when decline quarantine expires

When quarantine entries expire, cleanup_expired_leases unconditionally calls self.allocator.release(ip). This breaks reserved addresses: if a reserved IP is ever declined, it is added to declined_ips and then released after 5 minutes even though self.reservations still maps that IP to a MAC. At that point the allocator can reassign the reserved IP to other clients, causing reservation violations and potential duplicate-IP conflicts when the reserved client reconnects. The release path should skip IPs that are still reserved.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

Adds a time-bounded quarantine for DHCP DECLINE’d IP addresses to avoid permanent pool exhaustion, and tightens machine creation concurrency to prevent duplicate machine names under concurrent create requests.

Changes:

Track declined IPs with a timestamp and release them back to the allocator after a 5-minute quarantine.
Update DHCPDECLINE handling to remove the client’s pending lease and mark the declined IP as allocated during quarantine.
Hold a write lock across MachineManager::create to eliminate TOCTOU races on machine name uniqueness.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`virt/arcbox-dhcp/src/server.rs`	Adds declined-IP quarantine tracking, expiry cleanup logic, and a unit test for quarantine expiry behavior.
`app/arcbox-core/src/machine.rs`	Changes locking strategy in `MachineManager::create` to prevent concurrent duplicate creation of the same machine name.

Copilot · 2026-03-31T12:43:34Z

        if let Some(ip) = packet.requested_ip {
+            // Remove the lease so the client can start fresh with DISCOVER.
+            if let Some(lease) = self.leases.remove(&mac) {
+                // If the declined IP differs from the lease IP (unusual, but
+                // possible), release the lease IP since the client won't use it.
+                if lease.ip != ip && !self.reservations.contains_key(&mac) {
+                    self.allocator.release(lease.ip);
+                }
+            }
+
+            // Ensure the declined IP is marked as allocated so it cannot be
+            // handed out during the quarantine window. If it was already
+            // allocated (the common case), this is a no-op.
            self.allocator.allocate_specific(ip);

-            tracing::warn!("DHCPDECLINE: {} reported as in use", ip);
+            // Record the quarantine start time.
+            self.declined_ips.insert(ip, Instant::now());
+


handle_decline() records any requested_ip in declined_ips even if it is outside the configured pool range. Since allocate_specific() returns false for out-of-range addresses, these entries aren't actually quarantined in the allocator and can accumulate until cleanup runs. Consider validating the IP is within the pool range (or only inserting into declined_ips when the IP is in-range) before quarantining.

Copilot · 2026-03-31T12:43:34Z

+        server.declined_ips.insert(
+            ip,
+            Instant::now() - DECLINE_QUARANTINE - Duration::from_secs(1),
+        );


This test backdates the timestamp via Instant::now() - DECLINE_QUARANTINE - .... Subtracting a duration from Instant can panic if the system uptime is less than that duration (e.g., freshly started CI runner). Use checked_sub(...) (with a fallback) to avoid panics and test flakiness.

Suggested change

server.declined_ips.insert(

ip,

Instant::now() - DECLINE_QUARANTINE - Duration::from_secs(1),

);

let quarantine_expired_at = Instant::now()

.checked_sub(DECLINE_QUARANTINE + Duration::from_secs(1))

.unwrap_or_else(Instant::now);

server.declined_ips.insert(ip, quarantine_expired_at);

Copilot · 2026-03-31T12:43:35Z

+        // Hold the write lock for the entire create operation to prevent TOCTOU
+        // races: without this, two concurrent creates with the same name could
+        // both pass the existence check before either inserts.
+        let mut machines = self.machines.write().map_err(|_| CoreError::LockPoisoned)?;
+
+        if machines.contains_key(&config.name) {
            return Err(CoreError::already_exists(config.name));
        }


create() now holds a std::sync::RwLock write guard across filesystem I/O (create_dir_all), VM creation, and persistence writes. This blocks all other readers/writers of machines for the entire duration and can degrade concurrency (and potentially block a Tokio worker thread if this runs on the async runtime). Consider narrowing the lock scope (e.g., insert a temporary "creating" entry under the lock, drop the guard while doing I/O, then finalize/update under the lock with rollback on error).

Copilot · 2026-03-31T12:43:35Z

+        for ip in expired_declines {
+            self.declined_ips.remove(&ip);


cleanup_expired_leases() releases quarantined declined IPs unconditionally. If a client sends a DECLINE for an IP that is still in use (e.g., belongs to an active lease for a different MAC, or is reserved in reservations), this will remove it from the allocator and allow it to be reallocated, creating duplicate assignments and breaking reservations. Consider only releasing a quarantined IP if it is not reserved for any MAC and is not currently present in any active lease (and ideally only quarantine/release IPs that were actually offered to the declining MAC).

Suggested change

for ip in expired_declines {

self.declined_ips.remove(&ip);

for ip in expired_declines {

// Only release the IP back to the allocator if it is not currently

// in use by any active lease and not reserved for any MAC.

let ip_in_use = self.leases.values().any(|lease| lease.ip == ip);

let ip_reserved = self

.reservations

.values()

.any(|reserved_ip| *reserved_ip == ip);

self.declined_ips.remove(&ip);

if ip_in_use || ip_reserved {

tracing::debug!(

"Skipped releasing quarantined declined IP {} (in_use={}, reserved={})",

ip,

ip_in_use,

ip_reserved

);

continue;

}

AprilNEA added 2 commits March 31, 2026 20:29

fix(core): eliminate TOCTOU in MachineManager::create by holding writ…

6fb908b

…e lock

fix(dhcp): add expiry for declined IPs to prevent pool exhaustion

21b37fe

Copilot AI review requested due to automatic review settings March 31, 2026 12:38

Copilot started reviewing on behalf of AprilNEA March 31, 2026 12:39 View session

chatgpt-codex-connector Bot reviewed Mar 31, 2026

View reviewed changes

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dhcp): add expiry for declined IPs to prevent pool exhaustion#152

fix(dhcp): add expiry for declined IPs to prevent pool exhaustion#152
AprilNEA wants to merge 2 commits intomasterfrom
fix/dhcp-decline-ip-expiry

AprilNEA commented Mar 31, 2026

Uh oh!

linear Bot commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        server.declined_ips.insert(
-            ip,
-            Instant::now() - DECLINE_QUARANTINE - Duration::from_secs(1),
-        );
+        let quarantine_expired_at = Instant::now()
+            .checked_sub(DECLINE_QUARANTINE + Duration::from_secs(1))
+            .unwrap_or_else(Instant::now);
+        server.declined_ips.insert(ip, quarantine_expired_at);

-        for ip in expired_declines {
-            self.declined_ips.remove(&ip);
+        for ip in expired_declines {
+            // Only release the IP back to the allocator if it is not currently
+            // in use by any active lease and not reserved for any MAC.
+            let ip_in_use = self.leases.values().any(|lease| lease.ip == ip);
+            let ip_reserved = self
+                .reservations
+                .values()
+                .any(|reserved_ip| *reserved_ip == ip);
+            self.declined_ips.remove(&ip);
+            if ip_in_use || ip_reserved {
+                tracing::debug!(
+                    "Skipped releasing quarantined declined IP {} (in_use={}, reserved={})",
+                    ip,
+                    ip_in_use,
+                    ip_reserved
+                );
+                continue;
+            }

Conversation

AprilNEA commented Mar 31, 2026

Summary

Test plan

Uh oh!

linear Bot commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants