From a419e770d9750caf4ea7c525056c9d2a60bd78b6 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:38:22 +0200 Subject: [PATCH] [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. --- client/internal/engine.go | 28 +++-- proxy/internal/debug/handler.go | 189 ++++++++++++++++++++++++++++-- proxy/internal/debug/perf_test.go | 158 +++++++++++++++++++++++++ 3 files changed, 355 insertions(+), 20 deletions(-) create mode 100644 proxy/internal/debug/perf_test.go diff --git a/client/internal/engine.go b/client/internal/engine.go index f8b65f7d8..d517d1d68 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -14,12 +14,14 @@ import ( "sort" "strings" "sync" + "sync/atomic" "time" "github.com/hashicorp/go-multierror" "github.com/pion/ice/v4" "github.com/pion/stun/v3" log "github.com/sirupsen/logrus" + wgdevice "golang.zx2c4.com/wireguard/device" "golang.zx2c4.com/wireguard/tun/netstack" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" @@ -236,6 +238,12 @@ type Engine struct { wgInterface WGIface + // wgDevice is a lock-free handle on the WireGuard device behind + // wgInterface. Reaching the device through wgInterface requires + // syncMsgMux, which handleSync holds while it adds and removes peers; + // SetPerformance must stay reachable exactly when that work is stuck. + wgDevice atomic.Pointer[wgdevice.Device] + udpMux *udpmux.UniversalUDPMuxDefault // networkSerial is the latest CurrentSerial (state ID) of the network sent by the Management service @@ -651,6 +659,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL) log.Errorf("failed to pull up wgInterface [%s]: %s", e.wgInterface.Name(), err.Error()) return fmt.Errorf("up wg interface: %w", err) } + e.wgDevice.Store(e.wgInterface.GetWGDevice()) // Set up notrack rules immediately after proxy is listening to prevent // conntrack entries from being created before the rules are in place @@ -2144,6 +2153,10 @@ func (e *Engine) close() { log.Debugf("removing Netbird interface %s", e.config.WgIfaceName) if e.wgInterface != nil { + // Drop the handle before the close starts: a retune that loads it + // afterwards would touch a device on its way out and report success + // for an engine that is already gone. + e.wgDevice.Store(nil) if err := e.wgInterface.Close(); err != nil { log.Errorf("failed closing Netbird interface %s %v", e.config.WgIfaceName, err) } @@ -2303,15 +2316,16 @@ type Performance struct { } // SetPerformance applies the given tuning to this engine's live Device. +// +// It deliberately does not take syncMsgMux. Raising the buffer pool cap is the +// recovery path for a device whose pool is exhausted, and an exhausted pool +// blocks peer removal inside handleSync, which holds syncMsgMux for as long as +// it stays blocked. Taking the lock here would make the retune unreachable in +// the one situation that needs it. func (e *Engine) SetPerformance(t Performance) error { - e.syncMsgMux.Lock() - defer e.syncMsgMux.Unlock() - if e.wgInterface == nil { - return fmt.Errorf("wg interface not initialized") - } - dev := e.wgInterface.GetWGDevice() + dev := e.wgDevice.Load() if dev == nil { - return fmt.Errorf("wg device not initialized") + return errors.New("wg device not initialized") } if t.PreallocatedBuffersPerPool != nil { dev.SetPreallocatedBuffersPerPool(*t.PreallocatedBuffersPerPool) diff --git a/proxy/internal/debug/handler.go b/proxy/internal/debug/handler.go index 6300228d7..960c3e089 100644 --- a/proxy/internal/debug/handler.go +++ b/proxy/internal/debug/handler.go @@ -105,6 +105,20 @@ type Handler struct { startTime time.Time templates *template.Template templateMu sync.RWMutex + + // setPerformance applies a buffer cap to one client. Held as a field so + // tests can drive applyBufferCap without a live embedded client. + setPerformance func(*nbembed.Client, uint32) error + + perfMu sync.Mutex + perfInflight map[types.AccountID]*perfWorker +} + +// perfWorker is the single in-flight retune for one account. err is valid once +// done is closed. +type perfWorker struct { + done chan struct{} + err error } // NewHandler creates a new debug handler. @@ -113,10 +127,11 @@ func NewHandler(provider clientProvider, healthChecker healthChecker, logger *lo logger = log.StandardLogger() } h := &Handler{ - provider: provider, - health: healthChecker, - logger: logger, - startTime: time.Now(), + provider: provider, + health: healthChecker, + logger: logger, + startTime: time.Now(), + setPerformance: setClientPerformance, } if err := h.loadTemplates(); err != nil { logger.Errorf("failed to load embedded templates: %v", err) @@ -716,15 +731,7 @@ func (h *Handler) handlePerf(w http.ResponseWriter, r *http.Request) { } capN := uint32(n) - applied := 0 - failed := map[string]string{} - for accountID, client := range h.provider.ListClientsForStartup() { - if err := client.SetPerformance(nbembed.Performance{PreallocatedBuffersPerPool: &capN}); err != nil { - failed[string(accountID)] = err.Error() - continue - } - applied++ - } + applied, failed, inFlight := h.applyBufferCap(capN) resp := map[string]any{ "success": true, @@ -734,9 +741,165 @@ func (h *Handler) handlePerf(w http.ResponseWriter, r *http.Request) { if len(failed) > 0 { resp["failed"] = failed } + if len(inFlight) > 0 { + resp["in_flight"] = inFlight + } h.writeJSON(w, resp) } +// perfApplyTimeout bounds the whole apply, however many clients are registered. +// A var, not a const, so tests can shorten the wait. +var perfApplyTimeout = 5 * time.Second + +type perfResult struct { + accountID types.AccountID + err error +} + +// setClientPerformance is the production implementation behind Handler.setPerformance. +func setClientPerformance(client *nbembed.Client, capN uint32) error { + return client.SetPerformance(nbembed.Performance{PreallocatedBuffersPerPool: &capN}) +} + +// collectBuffered takes every result already sitting in the channel, removing +// those accounts from pending, and returns how many of them succeeded. It is +// called when the deadline fires: select picks at random among ready cases, so +// a result that landed in time would otherwise be reported as a timeout. +func collectBuffered(results <-chan perfResult, pending map[types.AccountID]*perfWorker, failed map[string]string) int { + applied := 0 + for { + select { + case res := <-results: + delete(pending, res.accountID) + if res.err != nil { + failed[string(res.accountID)] = res.err.Error() + continue + } + applied++ + default: + return applied + } + } +} + +// resolvePending closes out the accounts still pending when the deadline fires. +// A worker whose done channel is closed has finished, whatever the results +// channel has managed to deliver, so its own error is the truth; the rest are +// genuinely still running and are reported as timed out. Returns how many of +// them had in fact succeeded. +func resolvePending(pending map[types.AccountID]*perfWorker, failed map[string]string) int { + applied := 0 + for accountID, w := range pending { + select { + case <-w.done: + if w.err != nil { + failed[string(accountID)] = w.err.Error() + continue + } + applied++ + default: + failed[string(accountID)] = fmt.Sprintf("timed out after %s waiting for the client", perfApplyTimeout) + } + } + return applied +} + +// startPerfWorker returns the in-flight retune for the account, starting one if +// there is none. The bool reports whether this call started it. +// +// At most one retune runs per account at a time. A client wedged inside its own +// lock never returns, so without this a caller could add one permanently blocked +// goroutine per request just by retrying the endpoint. +func (h *Handler) startPerfWorker(accountID types.AccountID, client *nbembed.Client, capN uint32, results chan<- perfResult) (*perfWorker, bool) { + h.perfMu.Lock() + defer h.perfMu.Unlock() + + if w, ok := h.perfInflight[accountID]; ok { + return w, false + } + + w := &perfWorker{done: make(chan struct{})} + if h.perfInflight == nil { + h.perfInflight = make(map[types.AccountID]*perfWorker) + } + h.perfInflight[accountID] = w + + go func() { + err := h.setPerformance(client, capN) + w.err = err + close(w.done) + + // Publish before touching the registry: perfMu is taken once per + // account by every caller walking the fleet, so a finishing worker + // can queue behind a long apply and miss its own deadline. + results <- perfResult{accountID: accountID, err: err} + + h.perfMu.Lock() + delete(h.perfInflight, accountID) + h.perfMu.Unlock() + }() + + return w, true +} + +// applyBufferCap sets the WireGuard buffer pool cap on every registered client +// and reports how many took it, a per-account error for those that did not, and +// the accounts whose earlier retune has not come back yet. +// +// Clients are handled concurrently and the wait is bounded: SetPerformance goes +// through the embedded client's lock, which Start and Stop hold for as long as +// they take - and on a wedged client Stop never returns. This endpoint is the +// recovery path for exactly that fleet, so one stuck account must neither delay +// the others nor accumulate goroutines across retries. +func (h *Handler) applyBufferCap(capN uint32) (int, map[string]string, []string) { + clients := h.provider.ListClientsForStartup() + results := make(chan perfResult, len(clients)) + + applied := 0 + failed := map[string]string{} + var inFlight []string + pending := make(map[types.AccountID]*perfWorker, len(clients)) + + for accountID, client := range clients { + w, started := h.startPerfWorker(accountID, client, capN, results) + if started { + pending[accountID] = w + continue + } + // Another request owns this account's retune. Take its result if it + // has already landed, otherwise report it as still running instead of + // waiting on it again. + select { + case <-w.done: + if w.err != nil { + failed[string(accountID)] = w.err.Error() + continue + } + applied++ + default: + inFlight = append(inFlight, string(accountID)) + } + } + + deadline := time.After(perfApplyTimeout) + for range len(pending) { + select { + case res := <-results: + delete(pending, res.accountID) + if res.err != nil { + failed[string(res.accountID)] = res.err.Error() + continue + } + applied++ + case <-deadline: + applied += collectBuffered(results, pending, failed) + applied += resolvePending(pending, failed) + return applied, failed, inFlight + } + } + return applied, failed, inFlight +} + // handleRuntime returns cheap runtime and process stats. Safe to hit on a // running proxy; does not read pprof profiles. func (h *Handler) handleRuntime(w http.ResponseWriter, _ *http.Request) { diff --git a/proxy/internal/debug/perf_test.go b/proxy/internal/debug/perf_test.go new file mode 100644 index 000000000..abcfccb50 --- /dev/null +++ b/proxy/internal/debug/perf_test.go @@ -0,0 +1,158 @@ +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) + } +}