Files
netbird/proxy/internal/debug/perf_test.go
T
Riccardo Manfrin a419e770d9 [client, proxy] Make the buffer-pool retune reachable while a device is stalled (#7452)
* [client] Track the WireGuard device on the engine as a lock-free handle

Add an atomic handle on the wg device next to wgInterface, stored once the
interface is up and cleared when it is closed. Nothing reads it yet, so this
is a pure addition with no behavior change; it exists so the next commit can
reach the device without taking syncMsgMux.

* [client] Retune the WireGuard buffer pool without the engine lock

SetPerformance took syncMsgMux before reaching the device. That lock is held
by handleSync while it adds and removes peers, and peer removal is exactly
what blocks when a device's buffer pool is exhausted: Peer.Stop waits on a
keepalive timer callback that is itself parked in WaitPool.Get. Raising the
cap is the way out of that state, so the call must not queue behind the lock
the stall is holding.

Read the device through the atomic handle instead. Device.SetPreallocatedBuffersPerPool
takes the pool's own lock and broadcasts, so the waiters wake up.

* [proxy] Extract the buffer-cap apply loop out of the perf handler

Pure move: the loop over the registered clients becomes applyBufferCap, with
the same sequential behavior and the same return values. Split out so the next
commit can change how it iterates without the diff also carrying the move.

* [proxy] Bound the perf endpoint so one wedged client cannot hold it

The apply loop was sequential and unbounded. embed.Client.SetPerformance goes
through the client lock, which Start holds for the whole of a startup, so a
single account that is busy or wedged delayed the new buffer cap for every
other account on the node -- on the endpoint whose whole purpose is to
un-wedge a node.

Apply to all clients concurrently and give the whole call a 5s budget.
Accounts that do not answer in time are reported in "failed" instead of
blocking the response.

* [client] Drop the device handle before closing the interface

close() cleared the atomic handle only after wgInterface.Close() returned, so a
concurrent SetPerformance could still load it, retune a device that is being
torn down, and report the change as applied for an engine that has stopped.
Clear it first, so the window closes before the teardown begins.

Reported by cubic on PR #7452.

* [proxy] Put the per-client retune behind a field

Pure refactor: applyBufferCap calls h.setPerformance instead of the client
method directly, and NewHandler wires it to setClientPerformance. Same call,
same behavior; the seam is what lets the next two commits be tested without a
live embedded client.

* [proxy] Do not report a finished retune as timed out

When the deadline fires, select chooses at random among the ready cases, so a
result already sitting in the buffered channel could be skipped and its account
reported as timed out even though the cap had been applied. Drain what is
buffered before declaring the rest pending.

Reported by cubic on PR #7452.

* [proxy] Keep one retune per account in flight

The 5s budget bounds how long the endpoint waits, not the work: SetPerformance
goes through the embedded client's lock, and on a wedged account Stop holds that
lock forever, so every retry left one more goroutine parked there.

Route each account through a single worker. A request that finds one already
running takes its result if it has landed, and otherwise reports the account
under "in_flight" instead of starting a second attempt. One stuck account now
costs one goroutine, no matter how often the endpoint is called.

Reported by CodeRabbit and cubic on PR #7452.

* [proxy] Make the retune budget a var

Pure refactor: perfApplyTimeout becomes a var so a test can shorten it instead
of waiting five seconds. Same value, same behavior in production.

* [proxy] Extract the buffered-result drain

Pure refactor: the loop that empties the results channel when the deadline
fires becomes collectBuffered. Same behavior; split out so it can be tested
on its own, which the inline version could not be without racing the deadline.

* [proxy] Cover the retune single-flight and the deadline drain

TestApplyBufferCapSingleFlightPerAccount fails without the worker registry:
five calls against a client stuck in its own lock start five blocked workers
instead of one.

TestCollectBufferedCountsResultsReadyAtTheDeadline pins the drain helper's
contract - buffered results counted, errors recorded, only unanswered accounts
left pending. It drives collectBuffered directly: through applyBufferCap the
two select cases race by construction, so an end-to-end version of it would
pass on the unfixed code about half the time.

* [proxy] Keep the worker alongside each pending account

Pure refactor: the pending set becomes a map to the account's worker instead of
an empty struct. Same membership and same behavior; the next commit needs the
worker to resolve an account whose result has not reached the channel yet.

* [proxy] Publish a retune result before releasing its slot

The worker sent its result last, after taking perfMu to remove itself from the
registry. That lock is taken once per account by every caller walking the fleet,
so a worker that finished on time could queue behind an apply over thousands of
accounts and land after the deadline. Send first, deregister after.

Reported by cubic on PR #7452.

* [proxy] Read the worker, not the clock, for a finished retune

Publishing earlier only narrows the window: a client that answers just before
the deadline can still be reported as timed out. At the deadline the workers
themselves are authoritative - a closed done channel means the retune finished
and w.err carries its outcome, ordered by the close. Consult them instead of
declaring every pending account timed out, and keep the timeout label for the
ones actually still running.

Reported by cubic on PR #7452.

* [proxy] Cover the finished-worker resolution at the deadline

Fails on the previous behavior with "applied = 0, want 1": every pending
account was labelled a timeout, including the one whose retune had already
completed.
2026-09-11 09:38:22 +02:00

159 lines
4.9 KiB
Go

package debug
import (
"context"
"errors"
"sync/atomic"
"testing"
"time"
nbembed "github.com/netbirdio/netbird/client/embed"
"github.com/netbirdio/netbird/proxy/internal/health"
"github.com/netbirdio/netbird/proxy/internal/roundtrip"
"github.com/netbirdio/netbird/proxy/internal/types"
)
// perfProvider serves a fixed set of accounts. The clients are nil: the tests
// drive Handler.setPerformance, which never dereferences them.
type perfProvider struct {
accounts []types.AccountID
}
func (p *perfProvider) GetClient(types.AccountID) (*nbembed.Client, bool) { return nil, false }
func (p *perfProvider) ListClientsForDebug() map[types.AccountID]roundtrip.ClientDebugInfo {
return nil
}
func (p *perfProvider) ListClientsForStartup() map[types.AccountID]*nbembed.Client {
out := make(map[types.AccountID]*nbembed.Client, len(p.accounts))
for _, id := range p.accounts {
out[id] = nil
}
return out
}
type stubHealth struct{}
func (stubHealth) ReadinessProbe() bool { return true }
func (stubHealth) StartupProbe(context.Context) bool { return true }
func (stubHealth) CheckClientsConnected(context.Context) (bool, map[types.AccountID]health.ClientHealth) {
return true, nil
}
func shortenPerfTimeout(t *testing.T, d time.Duration) {
t.Helper()
prev := perfApplyTimeout
perfApplyTimeout = d
t.Cleanup(func() { perfApplyTimeout = prev })
}
// TestCollectBufferedCountsResultsReadyAtTheDeadline covers the select-ordering
// trap: when the deadline fires, results already buffered must be counted, not
// reported as timeouts. Driving collectBuffered directly keeps it deterministic
// - through applyBufferCap the two select cases race by construction.
func TestCollectBufferedCountsResultsReadyAtTheDeadline(t *testing.T) {
results := make(chan perfResult, 3)
results <- perfResult{accountID: "ok"}
results <- perfResult{accountID: "broken", err: errors.New("boom")}
pending := map[types.AccountID]*perfWorker{
"ok": {done: make(chan struct{})},
"broken": {done: make(chan struct{})},
"wedged": {done: make(chan struct{})},
}
failed := map[string]string{}
applied := collectBuffered(results, pending, failed)
if applied != 1 {
t.Fatalf("applied = %d, want 1", applied)
}
if failed["broken"] != "boom" {
t.Fatalf("failed = %v, want the error recorded for \"broken\"", failed)
}
if _, ok := pending["wedged"]; !ok || len(pending) != 1 {
t.Fatalf("pending = %v, want only the account that never answered", pending)
}
}
// TestApplyBufferCapSingleFlightPerAccount covers the goroutine accumulation
// reported on PR #7452: repeated calls against a client stuck in its own lock
// must not start a second attempt for the same account.
func TestApplyBufferCapSingleFlightPerAccount(t *testing.T) {
shortenPerfTimeout(t, 50*time.Millisecond)
release := make(chan struct{})
t.Cleanup(func() { close(release) })
var calls atomic.Int32
h := &Handler{
provider: &perfProvider{accounts: []types.AccountID{"wedged"}},
health: stubHealth{},
setPerformance: func(_ *nbembed.Client, _ uint32) error {
calls.Add(1)
<-release
return nil
},
}
for i := range 5 {
applied, failed, inFlight := h.applyBufferCap(4096)
if applied != 0 {
t.Fatalf("call %d: applied = %d, want 0", i, applied)
}
if i == 0 {
if len(failed) != 1 {
t.Fatalf("first call: failed = %v, want the account reported as timed out", failed)
}
continue
}
if len(inFlight) != 1 {
t.Fatalf("call %d: inFlight = %v, want the account reported as still running", i, inFlight)
}
if len(failed) != 0 {
t.Fatalf("call %d: failed = %v, want empty while the retune is in flight", i, failed)
}
}
if got := calls.Load(); got != 1 {
t.Fatalf("setPerformance called %d times, want 1: each retry started another blocked worker", got)
}
}
// TestResolvePendingTrustsFinishedWorkers covers the reporting race cubic
// flagged on PR #7452: a retune that finished just before the deadline must be
// reported by its outcome, not as a timeout, whatever the results channel has
// delivered so far.
func TestResolvePendingTrustsFinishedWorkers(t *testing.T) {
ok := &perfWorker{done: make(chan struct{})}
close(ok.done)
broken := &perfWorker{done: make(chan struct{}), err: errors.New("boom")}
close(broken.done)
stillRunning := &perfWorker{done: make(chan struct{})}
pending := map[types.AccountID]*perfWorker{
"ok": ok,
"broken": broken,
"running": stillRunning,
}
failed := map[string]string{}
applied := resolvePending(pending, failed)
if applied != 1 {
t.Fatalf("applied = %d, want 1", applied)
}
if failed["broken"] != "boom" {
t.Fatalf("failed[broken] = %q, want the worker's own error", failed["broken"])
}
if _, ok := failed["ok"]; ok {
t.Fatalf("failed = %v, want no entry for the account that succeeded", failed)
}
if got := failed["running"]; got == "" || got == "boom" {
t.Fatalf("failed[running] = %q, want the timeout message", got)
}
}