diff --git a/cmd/webui4frpc/main.go b/cmd/webui4frpc/main.go index a014254..73df96d 100644 --- a/cmd/webui4frpc/main.go +++ b/cmd/webui4frpc/main.go @@ -561,17 +561,32 @@ func main() { // LocalOnly forwards are never in the topology and are handled by the // auto-start loop above. go func() { - for { - if ring.IsLeader() { - break - } + // No leader gate here. The reconcile body is per-NODE work: it re-claims + // the forwards THIS node owns, whose worker processes died with the + // restart. Every member owns forwards (a 3-node ring splits them), so + // gating on leadership meant non-leaders waited forever and never + // rebuilt their own workers — a node could come back up "in the cluster" + // while owning a forward with no process behind it. Observed live: after a + // rolling restart a non-leader rejoined, adopted state listing itself as + // the owner of a forward, and never started it, because it sat waiting to + // become leader. + // + // What actually needs settling is the ring, not the leadership: wait for + // the topology to show us (with a bounded fallback so a lone node that + // rejoined a ring whose leader is unreachable still makes progress). + deadline := time.After(30 * time.Second) + for !ring.HasSelfInTopology() { select { - case <-time.After(2 * time.Second): + case <-deadline: + log.Printf("ring[%s] startup reconcile: self not in topology after 30s, proceeding anyway", ring.ID) + case <-time.After(500 * time.Millisecond): + continue case <-clusterCtx.Done(): return } + break } - // Let the ring stabilize after leader election / rejoin. + // Let the ring stabilize after election / rejoin before acting on it. time.Sleep(3 * time.Second) snap := ring.Snapshot() reclaimed := 0 diff --git a/internal/cluster/ring_disabled_test.go b/internal/cluster/ring_disabled_test.go index 7280d0e..d3129a3 100644 --- a/internal/cluster/ring_disabled_test.go +++ b/internal/cluster/ring_disabled_test.go @@ -155,3 +155,41 @@ func TestLocalDecisionsDoNotAccumulate(t *testing.T) { t.Fatalf("decisions for forwards outside the topology must be swept, got %v", eng.localDisabled) } } + +// TestHasSelfInTopology is the wait condition for the startup reconcile: the +// reconcile rebuilds the workers for forwards THIS node owns, so a node can only +// act once the adopted state actually names it as an owner. +// +// It is deliberately independent of leadership — the bug it guards against was a +// reconcile that waited for leadership and therefore never ran on followers, even +// though followers own forwards. +func TestHasSelfInTopology(t *testing.T) { + // Two nodes; the forward is owned by n2, which is NOT the leader. A single-node + // ring would make this test pass under either implementation, because the only + // member is necessarily the leader — which is exactly how the first version of + // this test came to be a false green. + eng := newTestEngine("n2", false) + eng.state.AddPending(store.Local{Name: "web"}, store.Remote{Name: "frps1"}, store.Link{RemotePort: 18081}) + if _, err := eng.OnToken(context.Background(), &Token{Cycle: 1, State: eng.state, SentAt: 1}); err != nil { + t.Fatal(err) + } + if eng.IsLeader() { + t.Fatal("precondition: n2 must not be the leader in this test") + } + // n2 claimed it, so it owns a forward — despite not leading. + if !eng.HasSelfInTopology() { + t.Error("a non-leader that owns a forward reports false — the startup reconcile " + + "would skip it and leave its worker down") + } + + // A node that owns nothing must report false (so it does not act early on a + // stale or empty topology). + other := newTestEngine("n9", true) + if other.HasSelfInTopology() { + t.Error("a node owning nothing must not claim it appears in the topology") + } + // Guard against a regression where leadership is folded into the predicate. + if !eng.HasSelfInTopology() && eng.IsLeader() { + t.Error("predicate must not be gated on leadership") + } +} diff --git a/internal/cluster/ring_engine.go b/internal/cluster/ring_engine.go index 8e9f11b..3717940 100644 --- a/internal/cluster/ring_engine.go +++ b/internal/cluster/ring_engine.go @@ -1206,3 +1206,20 @@ func (e *Engine) LeaderAddr() string { } return e.state.Nodes[i].Addr } + +// HasSelfInTopology reports whether the current topology names this node as the +// owner of at least one active forward. +// +// The startup reconcile uses it as its wait condition: a node can only rebuild +// the workers for forwards it owns once the adopted state says it owns them. +// Leadership is deliberately NOT part of the test — followers own forwards too, +// and gating the reconcile on leadership left them permanently unable to restore +// their own workers. +func (e *Engine) HasSelfInTopology() bool { + for _, te := range e.state.TopologyList() { + if te.OwnerID == e.ID && te.Active { + return true + } + } + return false +}