Skip to content

lib: make /connect O(1) with an in-memory peer IP index - #13

Merged
pbardea merged 3 commits into
mainfrom
devin/1783622318-connect-peer-cache
Jul 29, 2026
Merged

lib: make /connect O(1) with an in-memory peer IP index#13
pbardea merged 3 commits into
mainfrom
devin/1783622318-connect-peer-cache

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 9, 2026

Copy link
Copy Markdown

Summary

connectHandler did a full WireGuard device dump on every /connect to find whether the peer already had an assigned IP:

device, _ := srv.WgClient.Device(srv.Ifname())   // O(peers) netlink dump
for _, peer := range device.Peers {               // scan every peer
    if peer.PublicKey == peerKey { peerIp = ... }
}

That makes each request O(peers), so total cost is O(peers × connect-rate). During a burst of static-IP VM launches (the 2026-07-09 18:04–18:07 UTC incident: ~511 new peers in ~3 min), /connect p99 climbed from ~240ms to ~7.4s — past the agent's 5s client timeout — which is exactly what fires the VProxy Server Unreachable alert.

This replaces the per-request dump with an authoritative in-memory index and makes /connect O(1):

peerIPs map[wgtypes.Key]netip.Addr   // new, guarded by srv.mu

// connectHandler:
srv.mu.Lock()
peerIp, isExisting := srv.peerIPs[peerKey]
if !isExisting {
    peerIp = srv.ipAllocator.Allocate()   // unchanged allocator
    srv.peerIPs[peerKey] = peerIp
}
srv.newPeers[peerKey] = time.Now()
srv.mu.Unlock()
// ... single-peer ConfigureDevice (unchanged); on error, roll back the new entry

peerIPs is kept in sync with ipAllocator and the kernel device at the only two mutation points:

  • add: on a new peer, under the lock, alongside ipAllocator.Allocate() (rolled back if the subsequent ConfigureDevice fails).
  • remove: in removeIdlePeers, entries are deleted after the batch ConfigureDevice(remove) succeeds, in lockstep with ipAllocator.Free(), so a later reconnect re-allocates cleanly.

No rebuild-on-startup is needed: StartWireguard recreates the interface fresh (LinkDel/LinkAdd), so the device, ipAllocator, and peerIPs all start empty and reconnecting clients get new IPs — same as before.

Behavior is otherwise unchanged: the reconnect path still reuses the existing IP and re-issues an idempotent single-peer ConfigureDevice; the "no IPs available" 503 path is preserved. The idle reaper still does one device dump per 5s (not on the request path), which is fine.

Follow-ups discussed but intentionally out of scope here: raising/adding jitter to the agent's static-IP retry (self-amplifies bursts), and a p99-setup-duration early-warning alert.

Link to Devin session: https://app.devin.ai/sessions/dfdbb0929a794ffabbad104836af2942
Requested by: @taha-au


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled. (Staging)

connectHandler previously dumped the entire WireGuard device via
WgClient.Device() on every /connect request and scanned all peers to
find whether the peer's public key already had an assigned IP. That is
an O(peers) netlink call on the hot path, so under a burst of static-IP
launches the cost becomes O(peers x connect-rate) and /connect latency
climbs past the client timeout, which is what triggered the VProxy
Server Unreachable alert.

Maintain an authoritative in-memory map from peer public key to assigned
IP (peerIPs), populated when a peer is allocated an IP and pruned when
the idle reaper removes a peer, so /connect resolves an existing peer's
IP with a single map lookup under srv.mu instead of a device dump.

Co-Authored-By: taha <tahashabbir27@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 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. Add '(aside)' to your comment to have me ignore it.
  • 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

Comment thread lib/server.go
The reaper could remove a peer and free its IP between connectHandler
resolving the peer's IP under srv.mu and writing it to the device,
leaving the kernel with a peer whose IP the allocator considers free.
connectHandler refreshes newPeers under srv.mu before its device write,
so the reaper (which checks newPeers under the same lock) now skips any
peer with a live grace entry, regardless of handshake age.

Co-Authored-By: taha <tahashabbir27@gmail.com>
Comment thread lib/server.go Outdated
A stale-handshake peer that keeps calling /connect no longer retains
its device entry indefinitely: the reaper now only skips such a peer if
its most recent /connect was within ConnectGracePeriod (1 minute) -
enough to cover an in-flight handler device write plus the client's
handshake and health checks - and otherwise evicts on handshake age as
before.

Co-Authored-By: taha <tahashabbir27@gmail.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bfbef11. Configure here.

Comment thread lib/server.go
// it would free an IP the handler is handing out, desyncing
// peerIPs/ipAllocator from the device.
idle = time.Since(peer.LastHandshakeTime) > PeerIdleTimeout &&
time.Since(lastConnect) > ConnectGracePeriod

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale handshake grace mismatch

Medium Severity

In removeIdlePeers, peers with no handshake stay idle only while they remain in newPeers (up to about five minutes), but peers with a stale LastHandshakeTime are treated as idle once ConnectGracePeriod (one minute) passes after the last /connect, even if newPeers still records a recent connect. Reconnecting clients can be reaped and lose peerIPs entries while still within the longer grace window used for first-time connects.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bfbef11. Configure here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asymmetry is intentional, and this concern is the mirror image of the previous one (which flagged giving stale-handshake peers the full newPeers-lifetime grace as indefinite retention). The two cases legitimately differ:

  • Zero handshake (first-time connect): the peer has never proven liveness, so it keeps the original, more generous grace — the newPeers entry lifetime (up to PeerIdleTimeout) — before being reaped. This is unchanged from pre-PR behavior.
  • Stale handshake (reconnect): a WireGuard client initiates its handshake within seconds of a successful /connect (the agent's health checks complete within ~15s), so ConnectGracePeriod = 1 min is ample. Once the handshake lands, LastHandshakeTime is fresh and governs from then on. If the client fails to handshake within a minute of reconnecting, reaping is the correct outcome — its next /connect retry simply gets a fresh IP, same as before this PR.

So a genuinely reconnecting client can't be reaped mid-setup, and a handshake-dead one can't hold its entry indefinitely. No change made.

@pbardea
pbardea merged commit 7b30d64 into main Jul 29, 2026
3 checks passed
@adityamaru

Copy link
Copy Markdown

nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants