From fca6ace255a0e1736a3055757625c9b49ab59d58 Mon Sep 17 00:00:00 2001 From: T3ST3ST3R0N Date: Thu, 22 Oct 2026 20:39:44 +0330 Subject: [PATCH] fix(controller): restart an unhealthy backend on a same-config Start StartOrAttach treated any Start with the running config as a heartbeat once the backend reported started. A WireGuard backend keeps reporting started after its interface is gone, so the panel's repair Start did nothing and the node kept flapping until someone sent Stop first. A same-config Start is now a heartbeat only if GetSysStats passes; otherwise the backend is restarted, and it is not offered the config live. The check has its own short timeout and ignores the caller's cancellation, since it runs under LockControl and an abandoned request must not restart a healthy core. --- controller/controller.go | 39 +++++++++++---- controller/controller_test.go | 89 +++++++++++++++++++++++++++++++++-- 2 files changed, 116 insertions(+), 12 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index f5280a28..40d8fd3b 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -25,6 +25,11 @@ const NodeVersion = "0.5.4" // health interval equals keep_alive does not trip the watchdog on jitter. const keepAliveGrace = 5 * time.Second +// heartbeatHealthTimeout bounds the health check that tells a heartbeat Start +// from a repair Start. It runs under LockControl, so a hung core API must not +// hold the lock for long. +var heartbeatHealthTimeout = 3 * time.Second + type Service interface { Disconnect() } @@ -135,7 +140,7 @@ func sameBackendConfig(active, incoming *common.Backend) bool { // StartOrAttach starts the core, or refreshes keep-alive if the same core is // already running (heartbeat case). It only restarts when: // - No backend is running, or -// - The running backend has stopped, or +// - The running backend has stopped or fails its health check, or // - The incoming config differs from what started the current backend, and // the backend cannot apply the difference live. // @@ -148,21 +153,27 @@ func (c *Controller) StartOrAttach(ctx context.Context, data *common.Backend) er c.mu.RUnlock() if sameBackendConfig(active, data) { - // Same config, backend healthy: this is a heartbeat. Just refresh. - log.Println("StartOrAttach: backend already running with same config, refreshing keep-alive.") - c.Connect(data.GetKeepAlive()) - return nil - } - if applyConfigLive(ctx, back, active, data) { + err := backendHealthy(ctx, back) + if err == nil { + // Same config, backend healthy: this is a heartbeat. Just refresh. + log.Println("StartOrAttach: backend already running with same config, refreshing keep-alive.") + c.Connect(data.GetKeepAlive()) + return nil + } + // A backend can report started while it is broken (a WireGuard + // interface that is gone), and the panel sends Start to repair it. + log.Printf("StartOrAttach: running backend with same config is unhealthy, restarting: %v", err) + } else if applyConfigLive(ctx, back, active, data) { log.Println("StartOrAttach: config changed, applied to the running backend without a restart.") c.mu.Lock() c.activeData = data c.mu.Unlock() c.Connect(data.GetKeepAlive()) return nil + } else { + // Config changed while core is running: panel is sending a new config. + log.Println("StartOrAttach: config changed, restarting backend.") } - // Config changed while core is running: panel is sending a new config. - log.Println("StartOrAttach: config changed, restarting backend.") } else if back != nil { // Backend object exists but has stopped. log.Println("StartOrAttach: replacing a backend that is no longer running.") @@ -178,6 +189,16 @@ func (c *Controller) StartOrAttach(ctx context.Context, data *common.Backend) er return nil } +// backendHealthy reports why a running backend cannot serve, or nil. It does not +// follow the caller's cancellation, so an abandoned request does not restart a +// healthy core. +func backendHealthy(ctx context.Context, back backend.Backend) error { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), heartbeatHealthTimeout) + defer cancel() + _, err := back.GetSysStats(ctx) + return err +} + // applyConfigLive asks a running backend to take a changed config without a // restart. It reports false when the backend has to be restarted instead. func applyConfigLive(ctx context.Context, back backend.Backend, active, incoming *common.Backend) bool { diff --git a/controller/controller_test.go b/controller/controller_test.go index d34a392d..ea4d85c2 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -9,13 +9,17 @@ import ( "github.com/google/uuid" + "github.com/pasarguard/node/backend" "github.com/pasarguard/node/common" "github.com/pasarguard/node/config" ) type stubBackend struct { - started atomic.Bool - shutdowns atomic.Int32 + started atomic.Bool + shutdowns atomic.Int32 + statsCalls atomic.Int32 + statsErr error + statsBlocks bool // GetSysStats waits for its context to end } func (s *stubBackend) Started() bool { return s.started.Load() } @@ -27,7 +31,15 @@ func (s *stubBackend) SyncUser(context.Context, *common.User) error { return nil func (s *stubBackend) SyncUsers(context.Context, []*common.User) error { return nil } func (s *stubBackend) UpdateUsers(context.Context, []*common.User) error { return nil } func (s *stubBackend) UpdateUsersAndRestart(context.Context, []*common.User) error { return nil } -func (s *stubBackend) GetSysStats(context.Context) (*common.BackendStatsResponse, error) { +func (s *stubBackend) GetSysStats(ctx context.Context) (*common.BackendStatsResponse, error) { + s.statsCalls.Add(1) + if s.statsBlocks { + <-ctx.Done() + return nil, ctx.Err() + } + if s.statsErr != nil { + return nil, s.statsErr + } return &common.BackendStatsResponse{}, nil } func (s *stubBackend) GetStats(context.Context, *common.StatRequest) (*common.StatResponse, error) { @@ -82,6 +94,61 @@ func TestStartOrAttachHeartbeat(t *testing.T) { if running.shutdowns.Load() != 0 { t.Fatal("heartbeat: StartOrAttach shut down a running backend") } + if running.statsCalls.Load() != 1 { + t.Fatalf("heartbeat: GetSysStats calls = %d, want 1", running.statsCalls.Load()) + } +} + +// startSameConfigOn sends a Start with the config the running stub was started with. +func startSameConfigOn(t *testing.T, running backend.Backend, started *atomic.Bool) (*Controller, error) { + t.Helper() + c := New(config.NewTestConfig(t.TempDir(), uuid.New())) + t.Cleanup(c.Disconnect) + + started.Store(true) + c.backend = running + c.activeData = &common.Backend{Type: common.BackendType_WIREGUARD, Config: "cfg1", KeepAlive: 30} + + c.LockControl() + defer c.UnlockControl() + return c, c.StartOrAttach(context.Background(), &common.Backend{Type: common.BackendType_WIREGUARD, Config: "cfg1", KeepAlive: 30}) +} + +// TestStartOrAttachRestartsAnUnhealthyBackendWithSameConfig: a WireGuard +// backend keeps reporting started after its interface is gone, so a repair +// Start with the same config must restart it instead of acting as a heartbeat. +func TestStartOrAttachRestartsAnUnhealthyBackendWithSameConfig(t *testing.T) { + broken := &stubBackend{statsErr: errors.New("wireguard interface not available")} + c, err := startSameConfigOn(t, broken, &broken.started) + if err == nil { + t.Fatal("expected StartBackend to fail with fake config") + } + if broken.shutdowns.Load() != 1 { + t.Fatalf("unhealthy backend shutdowns = %d, want 1", broken.shutdowns.Load()) + } + if c.Backend() == broken { + t.Fatal("unhealthy backend is still attached") + } +} + +// TestStartOrAttachBoundsTheHeartbeatHealthCheck: a hung core API must not +// hold LockControl for long; it counts as unhealthy. +func TestStartOrAttachBoundsTheHeartbeatHealthCheck(t *testing.T) { + previous := heartbeatHealthTimeout + heartbeatHealthTimeout = 50 * time.Millisecond + t.Cleanup(func() { heartbeatHealthTimeout = previous }) + + hung := &stubBackend{statsBlocks: true} + begin := time.Now() + if _, err := startSameConfigOn(t, hung, &hung.started); err == nil { + t.Fatal("expected StartBackend to fail with fake config") + } + if elapsed := time.Since(begin); elapsed > 5*time.Second { + t.Fatalf("StartOrAttach took %s with a hung health check", elapsed) + } + if hung.shutdowns.Load() != 1 { + t.Fatalf("hung backend shutdowns = %d, want 1", hung.shutdowns.Load()) + } } // TestStartOrAttachConfigChanged: different config + running backend → restart. @@ -153,6 +220,22 @@ func (s *liveStubBackend) ApplyConfigLive(context.Context, string, []string) (bo return s.applied, s.err } +// TestStartOrAttachDoesNotApplyLiveToAnUnhealthyBackend: an unhealthy backend +// with the same config is restarted, not handed the config again live. +func TestStartOrAttachDoesNotApplyLiveToAnUnhealthyBackend(t *testing.T) { + broken := &liveStubBackend{applied: true} + broken.statsErr = errors.New("xray api unavailable") + if _, err := startSameConfigOn(t, broken, &broken.started); err == nil { + t.Fatal("expected StartBackend to fail with fake config") + } + if broken.calls.Load() != 0 { + t.Fatalf("ApplyConfigLive calls = %d, want 0", broken.calls.Load()) + } + if broken.shutdowns.Load() != 1 { + t.Fatalf("unhealthy backend shutdowns = %d, want 1", broken.shutdowns.Load()) + } +} + func startOnLiveStub(t *testing.T, running *liveStubBackend) (*Controller, error) { t.Helper() c := New(config.NewTestConfig(t.TempDir(), uuid.New()))