Free idle-peer IPs only when allPeers owns them

[saltzm]

Summary

While investigating the recurring peer unexpectedly not found in allPeers - did /disconnect race with the periodic peer-GC loop? warning (~2.8k/wk in Datadog), I found the warning itself is benign — allPeers is correctly guarded by srv.mu and the losing caller correctly no-ops. But the same race exposes a real, if rare, correctness bug in the IP allocator, which this PR fixes.

The bug: free-realloc-free in removeIdlePeers

Two removal paths free IPs differently:

• cleanupPeer (the /disconnect path) frees peerInfo.PeerIp from allPeers, and only when it actually removed the entry.
• removeIdlePeers (the GC loop) freed the IP taken from the WireGuard peer.AllowedIPs snapshot, for every idle peer — including peers with no allPeers entry (the idle=true "no entry" branch, which is exactly the peer involved in the race that emits the warning).

That opens a window where an in-use IP gets freed:

1. /disconnect -> cleanupPeer: delete(allPeers, key) under lock; unlock; WG remove; ipAllocator.Free(X)
2. /connect: ipAllocator.Allocate() -> returns the just-freed X to a NEW peer; adds it to WireGuard
3. GC removeIdlePeers: its device.Peers snapshot still lists the OLD peer, hits the
   "no allPeers entry" branch, and calls Free(X) again -- but X now belongs to the new peer

IpAllocator.Free guards against a plain back-to-back double-free (it checks membership before deleting/decrementing), but the reallocation in step 2 defeats that guard: X is dropped from allocated while a live peer holds it. Impact:

• X can later be handed to a second peer → duplicate tunnel IP → mis-SNAT / routing breakage.
• allocated_ips gauge drifts below the true count.

Low probability (needs a reconnect to land in the window) but a genuine correctness issue, not just log noise.

The fix

Gate the IP free on allPeers ownership and free the tracked PeerIp, so both removal paths free each IP exactly once — by whoever wins the allPeers delete under srv.mu:

peerInfo, exists := srv.allPeers[peer.PublicKey]
...
if idle {
    removePeers = append(removePeers, wgtypes.PeerConfig{PublicKey: peer.PublicKey, Remove: true})
    if exists {                      // only free if we still own it
        delete(srv.allPeers, peer.PublicKey)
        if !peerInfo.PeerIp.IsUnspecified() {
            removeIps = append(removeIps, peerInfo.PeerIp)
        }
    }                                // else: another path owns/freed the IP
}

Normal peers are unaffected — PeerIp equals the value previously read from AllowedIPs[0]. Peers still get removed from WireGuard regardless; only the allocator free is now gated. The warning log is intentionally left in place as an accurate race indicator.

Not addressed here (follow-up)

connectHandler reads allPeers under one lock, releases it, then Allocate()s and re-inserts under a second lock. A reconnect of an idle peer racing GC can leave allPeers/WireGuard holding an IP the allocator has freed. Fixing that means doing check + allocate + insert in a single critical section — rarer and riskier, so left out of this minimal change.

Testing

• go build ./..., go vet ./..., gofmt -l lib/server.go (clean), go test ./... all pass.

Link to Devin session: https://modal.devinenterprise.com/sessions/6820fff430bc46799775cbc191010b7d
Requested by: @saltzm

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR that start with 'DevinAI' or '@devin'.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring