diff --git a/controller/controller.go b/controller/controller.go index f56da723..7ee11202 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -32,6 +32,11 @@ const keepAliveGrace = 5 * time.Second // user list takes xray longer than that to load. const startTimeout = 3 * time.Minute +// 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() } @@ -160,7 +165,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. // @@ -180,21 +185,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.") @@ -213,6 +224,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()))