From 3de889d529a366441788886cfc46970096fd28fd Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Tue, 30 Jun 2026 08:18:51 +0200 Subject: [PATCH] [client] bound system info / posture-check gathering with a timeout to prevent sync-loop freeze (#6512) * Wraps syestem info / posture checks into a goroutine with timeout e.checks = checks is set before doing the SyncMeta, so if it fails next time isCheckEquals compares true and bypasses the update. This is to avoid another repeating the 15 seconds hang. The checks will be synced on reconnect or posture checks changes push from mgmt. * Propagate context to OS calls that can leverage its cancellation / timeout * Distinguish timeout from cancellation in logs * Dont log twice * Block on timeout failure and reapply the exclude_ips * Refactor for complexity --- client/internal/engine.go | 59 ++++++++++++++++------------------- client/system/info.go | 44 +++++++++++++++++++++++++- client/system/info_android.go | 2 +- client/system/info_darwin.go | 2 +- client/system/info_ios.go | 2 +- client/system/info_js.go | 2 +- client/system/info_test.go | 15 +++++++++ client/system/process.go | 21 +++++++++---- client/system/process_test.go | 31 ++++++++++++++++-- 9 files changed, 133 insertions(+), 45 deletions(-) diff --git a/client/internal/engine.go b/client/internal/engine.go index f7c7e1862..de151592d 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -82,6 +82,12 @@ const ( PeerConnectionTimeoutMax = 45000 // ms PeerConnectionTimeoutMin = 30000 // ms disableAutoUpdate = "disabled" + + // systemInfoTimeout bounds how long the sync loop waits for system info / posture + // check gathering. The gathering runs uncancellable system calls (process scan, + // exec, os.Stat); without this bound a single stuck call freezes handleSync, and + // thus syncMsgMux, for as long as the call hangs (observed multi-minute freezes). + systemInfoTimeout = 15 * time.Second ) var ErrResetConnection = fmt.Errorf("reset connection") @@ -1084,11 +1090,22 @@ func (e *Engine) updateChecksIfNew(checks []*mgmProto.Checks) error { } e.checks = checks - info, err := system.GetInfoWithChecks(e.ctx, checks, e.overlayAddresses()...) - if err != nil { - log.Warnf("failed to get system info with checks: %v", err) - info = system.GetInfo(e.ctx) + info, ok := system.GetInfoWithChecksTimeout(e.ctx, systemInfoTimeout, checks, e.overlayAddresses()...) + if !ok { + // Gathering timed out; skip the meta sync this cycle rather than blocking the + // sync loop (and syncMsgMux) on a stuck system call. A later sync will retry. + return nil } + e.applyInfoFlags(info) + + if err := e.mgmClient.SyncMeta(info); err != nil { + return fmt.Errorf("could not sync meta: error %s", err) + } + return nil +} + +// applyInfoFlags sets the engine's config-derived feature flags on the gathered system info. +func (e *Engine) applyInfoFlags(info *system.Info) { info.SetFlags( e.config.RosenpassEnabled, e.config.RosenpassPermissive, @@ -1107,12 +1124,6 @@ func (e *Engine) updateChecksIfNew(checks []*mgmProto.Checks) error { e.config.EnableSSHRemotePortForwarding, e.config.DisableSSHAuth, ) - - if err := e.mgmClient.SyncMeta(info); err != nil { - log.Errorf("could not sync meta: error %s", err) - return err - } - return nil } // overlayAddresses returns our own WireGuard overlay address (v4 and v6) so it @@ -1272,31 +1283,15 @@ func (e *Engine) receiveManagementEvents() { e.shutdownWg.Add(1) go func() { defer e.shutdownWg.Done() - info, err := system.GetInfoWithChecks(e.ctx, e.checks, e.overlayAddresses()...) - if err != nil { - log.Warnf("failed to get system info with checks: %v", err) + info, ok := system.GetInfoWithChecksTimeout(e.ctx, systemInfoTimeout, e.checks, e.overlayAddresses()...) + if !ok { + // Gathering timed out; connect the stream with base info so management + // connectivity still comes up rather than blocking here. info = system.GetInfo(e.ctx) } - info.SetFlags( - e.config.RosenpassEnabled, - e.config.RosenpassPermissive, - &e.config.ServerSSHAllowed, - e.config.DisableClientRoutes, - e.config.DisableServerRoutes, - e.config.DisableDNS, - e.config.DisableFirewall, - e.config.BlockLANAccess, - e.config.BlockInbound, - e.config.DisableIPv6, - e.config.LazyConnectionEnabled, - e.config.EnableSSHRoot, - e.config.EnableSSHSFTP, - e.config.EnableSSHLocalPortForwarding, - e.config.EnableSSHRemotePortForwarding, - e.config.DisableSSHAuth, - ) + e.applyInfoFlags(info) - err = e.mgmClient.Sync(e.ctx, info, e.handleSync) + err := e.mgmClient.Sync(e.ctx, info, e.handleSync) if err != nil { // happens if management is unavailable for a long time. // We want to cancel the operation of the whole client diff --git a/client/system/info.go b/client/system/info.go index 27588859e..496b478a3 100644 --- a/client/system/info.go +++ b/client/system/info.go @@ -2,9 +2,11 @@ package system import ( "context" + "errors" "net/netip" "slices" "strings" + "time" log "github.com/sirupsen/logrus" "google.golang.org/grpc/metadata" @@ -174,7 +176,7 @@ func GetInfoWithChecks(ctx context.Context, checks []*proto.Checks, excludeIPs . processCheckPaths = append(processCheckPaths, check.GetFiles()...) } - files, err := checkFileAndProcess(processCheckPaths) + files, err := checkFileAndProcess(ctx, processCheckPaths) if err != nil { return nil, err } @@ -187,3 +189,43 @@ func GetInfoWithChecks(ctx context.Context, checks []*proto.Checks, excludeIPs . log.Debugf("all system information gathered successfully") return info, nil } + +// GetInfoWithChecksTimeout is GetInfoWithChecks bounded by timeout. Posture-check gathering +// runs uncancellable system calls (process enumeration, os.Stat), so calling it inline can +// block the caller for as long as such a call hangs. It runs in a goroutine instead: if it +// does not return within timeout the caller gets (nil, false) and should proceed with +// degraded behavior rather than block. On a gathering error it falls back to base GetInfo. +// +// The buffered channel lets the abandoned goroutine finish and exit once its blocking call +// returns, so it does not leak beyond the duration of that call. +func GetInfoWithChecksTimeout(ctx context.Context, timeout time.Duration, checks []*proto.Checks, excludeIPs ...netip.Addr) (*Info, bool) { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + infoCh := make(chan *Info, 1) + go func() { + info, err := GetInfoWithChecks(ctx, checks, excludeIPs...) + if err != nil { + if ctx.Err() != nil { + return + } + log.Warnf("failed to get system info with checks: %v", err) + info = GetInfo(ctx) + info.removeAddresses(excludeIPs...) + } + infoCh <- info + }() + + select { + case info := <-infoCh: + return info, true + case <-ctx.Done(): + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + log.Warnf("gathering system info with checks timed out after %s", timeout) + } else { + // Parent context canceled (e.g. shutdown), not a timeout. + log.Warnf("gathering system info with checks canceled: %v", ctx.Err()) + } + return nil, false + } +} diff --git a/client/system/info_android.go b/client/system/info_android.go index 794ff15ed..3c71573bb 100644 --- a/client/system/info_android.go +++ b/client/system/info_android.go @@ -50,7 +50,7 @@ func GetInfo(ctx context.Context) *Info { } // checkFileAndProcess checks if the file path exists and if a process is running at that path. -func checkFileAndProcess(paths []string) ([]File, error) { +func checkFileAndProcess(_ context.Context, _ []string) ([]File, error) { return []File{}, nil } diff --git a/client/system/info_darwin.go b/client/system/info_darwin.go index 4a31920ec..e7bf367f6 100644 --- a/client/system/info_darwin.go +++ b/client/system/info_darwin.go @@ -32,7 +32,7 @@ func GetInfo(ctx context.Context) *Info { sysName := string(bytes.Split(utsname.Sysname[:], []byte{0})[0]) machine := string(bytes.Split(utsname.Machine[:], []byte{0})[0]) release := string(bytes.Split(utsname.Release[:], []byte{0})[0]) - swVersion, err := exec.Command("sw_vers", "-productVersion").Output() + swVersion, err := exec.CommandContext(ctx, "sw_vers", "-productVersion").Output() if err != nil { log.Warnf("got an error while retrieving macOS version with sw_vers, error: %s. Using darwin version instead.\n", err) swVersion = []byte(release) diff --git a/client/system/info_ios.go b/client/system/info_ios.go index ad42b1edf..1b0c084b3 100644 --- a/client/system/info_ios.go +++ b/client/system/info_ios.go @@ -105,7 +105,7 @@ func isDuplicated(addresses []NetworkAddress, addr NetworkAddress) bool { } // checkFileAndProcess checks if the file path exists and if a process is running at that path. -func checkFileAndProcess(paths []string) ([]File, error) { +func checkFileAndProcess(_ context.Context, _ []string) ([]File, error) { return []File{}, nil } diff --git a/client/system/info_js.go b/client/system/info_js.go index 994d439a7..f32532881 100644 --- a/client/system/info_js.go +++ b/client/system/info_js.go @@ -103,7 +103,7 @@ func collectLocationInfo(info *Info) { } } -func checkFileAndProcess(_ []string) ([]File, error) { +func checkFileAndProcess(_ context.Context, _ []string) ([]File, error) { return []File{}, nil } diff --git a/client/system/info_test.go b/client/system/info_test.go index dcda18e61..a7fa02197 100644 --- a/client/system/info_test.go +++ b/client/system/info_test.go @@ -4,6 +4,7 @@ import ( "context" "net/netip" "testing" + "time" "github.com/stretchr/testify/assert" "google.golang.org/grpc/metadata" @@ -35,6 +36,20 @@ func Test_CustomHostname(t *testing.T) { assert.Equal(t, want, got.Hostname) } +func TestGetInfoWithChecksTimeout_Success(t *testing.T) { + info, ok := GetInfoWithChecksTimeout(context.Background(), 30*time.Second, nil) + assert.True(t, ok, "expected gathering to complete within the timeout") + assert.NotNil(t, info) +} + +func TestGetInfoWithChecksTimeout_Timeout(t *testing.T) { + // A 1ns budget expires before the (real) system-info gathering can finish, so the + // caller must get (nil, false) instead of blocking on the in-flight goroutine. + info, ok := GetInfoWithChecksTimeout(context.Background(), time.Nanosecond, nil) + assert.False(t, ok, "expected timeout to be reported") + assert.Nil(t, info) +} + func Test_NetAddresses(t *testing.T) { addr, err := networkAddresses() if err != nil { diff --git a/client/system/process.go b/client/system/process.go index 87e21eb9d..07f69a212 100644 --- a/client/system/process.go +++ b/client/system/process.go @@ -3,24 +3,30 @@ package system import ( + "context" "os" "slices" "github.com/shirou/gopsutil/v3/process" ) -// getRunningProcesses returns a list of running process paths. -func getRunningProcesses() ([]string, error) { - processIDs, err := process.Pids() +// getRunningProcesses returns a list of running process paths. The context bounds the work: +// the per-PID loop bails as soon as ctx is done, and the gopsutil calls honor it where they +// can, so a stuck enumeration cannot run unbounded. +func getRunningProcesses(ctx context.Context) ([]string, error) { + processIDs, err := process.PidsWithContext(ctx) if err != nil { return nil, err } processMap := make(map[string]bool) for _, pID := range processIDs { + if err := ctx.Err(); err != nil { + return nil, err + } p := &process.Process{Pid: pID} - path, _ := p.Exe() + path, _ := p.ExeWithContext(ctx) if path != "" { processMap[path] = false } @@ -35,18 +41,21 @@ func getRunningProcesses() ([]string, error) { } // checkFileAndProcess checks if the file path exists and if a process is running at that path. -func checkFileAndProcess(paths []string) ([]File, error) { +func checkFileAndProcess(ctx context.Context, paths []string) ([]File, error) { files := make([]File, len(paths)) if len(paths) == 0 { return files, nil } - runningProcesses, err := getRunningProcesses() + runningProcesses, err := getRunningProcesses(ctx) if err != nil { return nil, err } for i, path := range paths { + if err := ctx.Err(); err != nil { + return nil, err + } file := File{Path: path} _, err := os.Stat(path) diff --git a/client/system/process_test.go b/client/system/process_test.go index 505808a9e..44a1c8ba0 100644 --- a/client/system/process_test.go +++ b/client/system/process_test.go @@ -1,6 +1,7 @@ package system import ( + "context" "testing" "github.com/shirou/gopsutil/v3/process" @@ -9,7 +10,7 @@ import ( func Benchmark_getRunningProcesses(b *testing.B) { b.Run("getRunningProcesses new", func(b *testing.B) { for i := 0; i < b.N; i++ { - ps, err := getRunningProcesses() + ps, err := getRunningProcesses(context.Background()) if err != nil { b.Fatalf("unexpected error: %v", err) } @@ -29,12 +30,38 @@ func Benchmark_getRunningProcesses(b *testing.B) { } } }) - s, _ := getRunningProcesses() + s, _ := getRunningProcesses(context.Background()) b.Logf("getRunningProcesses returned %d processes", len(s)) s, _ = getRunningProcessesOld() b.Logf("getRunningProcessesOld returned %d processes", len(s)) } +func TestCheckFileAndProcess_ContextCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // With a canceled context and non-empty paths the gathering must bail with an error + // instead of running the (potentially blocking) process scan / stat loop. + if _, err := checkFileAndProcess(ctx, []string{"/does/not/exist"}); err == nil { + t.Fatal("expected error on canceled context, got nil") + } +} + +func TestCheckFileAndProcess_EmptyPaths(t *testing.T) { + // No check paths means no work to do: it must return immediately with no error, + // even on a canceled context (nothing to scan or stat). + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + files, err := checkFileAndProcess(ctx, nil) + if err != nil { + t.Fatalf("unexpected error for empty paths: %v", err) + } + if len(files) != 0 { + t.Fatalf("expected no files, got %d", len(files)) + } +} + func getRunningProcessesOld() ([]string, error) { processes, err := process.Processes() if err != nil {