fix(server): enforce global UDP connection cap

[justin-layerv]

What changed

• replace the mutually exclusive overload/cap check with one locked predicate
• keep overload state accurate on the rejection path
• add boundary and concurrent-call regression coverage

Why

The previous if overload { ... } else if at-cap { ... } structure made the hard cap unreachable after the lower overload threshold was crossed. A UDP source-port rotation flood could therefore continue growing remoteConnectionMap beyond MaxConcurrentConnection.

Impact

New direct UDP connection tuples are rejected at the documented global cap while existing connections continue normally. Overload-cookie behavior is unchanged.

This is adapted from layervai/nhp#1568 (f3e8f528) for current OpenNHP main.

Validation

• go test -race ./server

The upstream server package initializes KBS material under /opt; the local non-root test run redirected only that init path to a temporary directory, then restored it. No KBS change is included here.

Dependencies

None. This PR is independently based on OpenNHP main at 8278c4e.

[github-actions]

✅ Looks Good - Code looks good

This is a correct, well-scoped fix for a genuine resource-exhaustion / DoS bug. I traced the logic against the constants and the parallel relay path and it holds up.

Why the bug was real

With OverloadConnectionThreshold = 16384 (80%) and MaxConcurrentConnection = 20480 (endpoints/server/constants.go:6-7), the old structure at udpserver.go:643-650:

if len(map) > OverloadConnectionThreshold {   // 16384
    SetOverload(true)
} else if len(map) >= MaxConcurrentConnection { // 20480 — unreachable
    reject
}

Any n >= 20480 also satisfies n > 16384, so the first branch always won and the hard cap was never enforced. A UDP source-port-rotation flood could grow remoteConnectionMap without bound. Good catch.

Why the fix is correct

globalCapAdmits() makes the two conditions independent — set overload on n > threshold, admit on n < cap. This now matches the already-correct relay path in msghandler.go:904-927 (>= MaxConcurrentConnection reject, then > OverloadConnectionThreshold overload), so both connection-creation paths are finally consistent.

Concurrency

• SetOverload(true) remains inside remoteConnectionMapMutex, preserving the serialization invariant documented at udpserver.go:777-785.
• Only a single recvPacketRoutine goroutine (launched once at udpserver.go:419) runs the create path, so moving the check into a lock/unlock helper introduces no new TOCTOU beyond what already existed (the original code also unlocked between check and insert).

Tests

• Boundary table is accurate: "at overload threshold" expects wantOverload=false (16384 > 16384 is false) and "at cap" expects wantAdmit=false, wantOverload=true — both correct.
• The concurrent test correctly uses t.Error (goroutine-safe) inside goroutines and reserves t.Fatal for the main goroutine; all callers serialize through the mutex, so it's clean under -race.
• Import grouping and core.NewDevice/Stop/IsOverload/SetOverload signatures verified.

Minor (optional, non-blocking)

The concurrency comment at udpserver.go:777-785 enumerates the SetOverload(true) call sites by line number ("udpserver.go:540 and msghandler.go:875"). Those references were already stale before this PR, and this PR relocates one of them into globalCapAdmits(). Consider updating that comment to point at globalCapAdmits() so the invariant documentation stays accurate — purely a doc nit, not a blocker.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
endpoints/server/udpserver.go	87.50%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main    #1634      +/-   ##
==========================================
+ Coverage   12.54%   12.58%   +0.04%     
==========================================
  Files          96       96              
  Lines       14526    14528       +2     
==========================================
+ Hits         1822     1829       +7     
+ Misses      12526    12520       -6     
- Partials      178      179       +1     

Flag	Coverage Δ
unittests	12.58% <87.50%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
endpoints/server/udpserver.go	0.89% <87.50%> (+0.89%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

✅ Looks Good - Code looks good

This is a well-scoped, correct fix for a genuine denial-of-service vector. I reviewed the logic, boundary semantics, concurrency, and test coverage.

The bug is real

With OverloadConnectionThreshold = 16384 (80%) and MaxConcurrentConnection = 20480, the previous structure:

if len(map) > OverloadConnectionThreshold {
    SetOverload(true)
} else if len(map) >= MaxConcurrentConnection {  // unreachable
    reject
}

made the hard-cap branch unreachable once the table crossed 16384 entries — the first predicate always won. A UDP source-port rotation flood could therefore grow remoteConnectionMap past MaxConcurrentConnection without bound (memory exhaustion). Decoupling the two conditions in globalCapAdmits() fixes this.

Correctness confirmed

• globalCapAdmits() sets overload at n > OverloadConnectionThreshold and admits at n < MaxConcurrentConnection — reject boundary is n >= MaxConcurrentConnection, matching the relay-admission path in msghandler.HandleRelayForward (total >= MaxConcurrentConnection / total > OverloadConnectionThreshold). The two admission paths are now consistent.
• The whole len + SetOverload runs under remoteConnectionMapMutex (Lock + defer Unlock), preserving the serialization invariant documented in connectionRoutine's teardown block. The comment refresh at udpserver.go:780 accurately re-points the cross-references.
• Device.Overload is an atomic (.Load/.Store), so the 32-goroutine -race test is safe; each caller serializes on the mutex anyway.

Tests

TestGlobalCapAdmitsBoundaries covers empty / at-threshold / above-threshold / below-cap / at-cap / above-cap with correct expected admit+overload values, and each subtable case uses a fresh device so sticky overload state doesn't leak. TestGlobalCapAdmitsConcurrentRejectIsConsistent guards the rejection path under concurrency. Good regression coverage.

One minor, non-blocking note

globalCapAdmits() releases the mutex before the caller re-acquires it to insert the new entry (udpserver.go:687-689), so between the check and the insert the relay path (HandleRelayForward) can add entries — the cap remains a best-effort soft cap that can be transiently exceeded by a small margin. This is pre-existing behavior (the old code had the identical check-release-insert structure) and not a regression introduced here, so no action needed for this PR. Worth keeping in mind if the cap ever needs to be a hard guarantee.

Nice fix — clear rationale in the PR description and the explanatory comment on globalCapAdmits.

[github-actions]

✅ Looks Good - Code looks good

Reviewed the fix and tests thoroughly. This is a correct, well-scoped fix for a real bug.

Bug confirmed

OverloadConnectionThreshold = 16384 and MaxConcurrentConnection = 20480 (endpoints/server/constants.go:6-7). In the original structure:

if len(...) > OverloadConnectionThreshold {   // 16384
    s.device.SetOverload(true)
} else if len(...) >= MaxConcurrentConnection { // 20480 — unreachable
    // reject
}

Once the map crossed 16384 the first branch always won, so the hard-cap else if was dead code. remoteConnectionMap could grow past MaxConcurrentConnection without bound — a genuine memory-exhaustion / DoS vector under a UDP source-port rotation flood. Good catch.

Fix

globalCapAdmits() (endpoints/server/udpserver.go:698-711) correctly decouples the two independent conditions: it always evaluates overload (n > OverloadConnectionThreshold) and independently returns n < MaxConcurrentConnection. Reject semantics match the original intent (>= cap), and overload state now stays accurate on the rejection path.

Concurrency

The direct UDP path has a single recvPacketRoutine goroutine (udpserver.go:419), and the whole predicate runs under remoteConnectionMapMutex, so no new TOCTOU is introduced. The refreshed comment at udpserver.go:777-785 accurately lists the three serialized SetOverload sites (globalCapAdmits, relay admission in HandleRelayForward at msghandler.go:926, and the teardown SetOverload(false)); swapping brittle line numbers for function names is a nice touch.

Tests

The boundary table (empty / at / above overload threshold / below / at / above cap) traces correctly against the implementation, and the concurrent test correctly uses t.Error inside goroutines with t.Fatal only after wg.Wait().

Minor / non-blocking observations

• globalCapAdmits() mutates state (SetOverload(true)) despite a predicate-style name. The doc comment explains why it lives under a single lock, so this is fine — just noting it reads slightly surprisingly.
• Pre-existing and out of scope: the relay reject path (msghandler.go:904) returns without calling SetOverload(true) when total >= MaxConcurrentConnection. In practice overload was already set when the count crossed the threshold earlier, so it's harmless — mentioning only for symmetry with the consistency goal of this PR.

Note: I could not execute go test -race ./server in this sandboxed environment (go commands are blocked), so I relied on static review; the PR reports it was run locally.