From 65ccbdeea22ba5370b44adbe807cf16b36dc7ac6 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 31 Oct 2026 14:15:37 +0000 Subject: [PATCH] poll: tell peers a node left after 15 seconds, not 10 As SaaS. A router outage of 10 to 15 seconds moved the HA primary for good, and status flapped. A real crash now fails over 5 seconds later, as on SaaS. --- hscontrol/poll.go | 7 ++++--- hscontrol/servertest/ephemeral_test.go | 2 +- hscontrol/servertest/ha_health_test.go | 4 ++-- hscontrol/servertest/ha_property_test.go | 4 ++-- hscontrol/servertest/issues_test.go | 9 +++++---- hscontrol/servertest/lifecycle_compat_test.go | 3 ++- hscontrol/servertest/lifecycle_test.go | 8 ++++---- hscontrol/servertest/poll_race_test.go | 4 ++-- hscontrol/servertest/race_test.go | 2 +- hscontrol/servertest/routes_test.go | 2 +- hscontrol/servertest/weather_test.go | 2 +- 11 files changed, 25 insertions(+), 22 deletions(-) diff --git a/hscontrol/poll.go b/hscontrol/poll.go index a21bcc55c..6ed325a5c 100644 --- a/hscontrol/poll.go +++ b/hscontrol/poll.go @@ -213,13 +213,14 @@ func (m *mapSession) serveLongPoll() { node, nodeExists := m.h.state.GetNodeByID(m.node.ID()) if !stillConnected && nodeExists && !node.IsExpired() { - // Wait up to 10 seconds for the node to reconnect. - // 10 seconds was arbitrary chosen as a reasonable time to reconnect. + // Wait up to 15 seconds for the node to reconnect. SaaS tells peers + // a node left after about that long, so an outage this short moves + // no HA primary there either. ticker := time.NewTicker(time.Second) defer ticker.Stop() waitForReconnect: - for range 10 { + for range 15 { if m.h.mapBatcher.IsConnected(m.node.ID()) { break } diff --git a/hscontrol/servertest/ephemeral_test.go b/hscontrol/servertest/ephemeral_test.go index bd2afd182..c0f128ff2 100644 --- a/hscontrol/servertest/ephemeral_test.go +++ b/hscontrol/servertest/ephemeral_test.go @@ -80,7 +80,7 @@ func TestEphemeralNodes(t *testing.T) { // Disconnect the ephemeral node. ephemeral.Disconnect(t) - // After the grace period (10s) + ephemeral timeout (3s), + // After the grace period (15s) + ephemeral timeout (3s), // the ephemeral node should be deleted from the server and // disappear from the regular node's peer list entirely. // Unlike non-ephemeral nodes which go offline but stay in diff --git a/hscontrol/servertest/ha_health_test.go b/hscontrol/servertest/ha_health_test.go index ff16b777d..4d5222007 100644 --- a/hscontrol/servertest/ha_health_test.go +++ b/hscontrol/servertest/ha_health_test.go @@ -223,7 +223,7 @@ func TestHAHealthProbe_SetApprovedRoutesEmptyClearsUnhealthy(t *testing.T) { // Disconnect resets a stale Unhealthy bit. An offline node is not an // HA candidate; carrying the bit forward leaks into DebugRoutes. // -// The poll handler waits a 10s grace period before calling +// The poll handler waits a 15s grace period before calling // [state.State.Disconnect], so the assertion is wrapped in Eventually with a // generous timeout. func TestHAHealthProbe_DisconnectClearsUnhealthy(t *testing.T) { @@ -250,7 +250,7 @@ func TestHAHealthProbe_DisconnectClearsUnhealthy(t *testing.T) { assert.Eventually(t, func() bool { return srv.State().IsNodeHealthy(nodeID1) - }, 15*time.Second, 200*time.Millisecond, + }, 25*time.Second, 200*time.Millisecond, "disconnect should drop stale Unhealthy bit") } diff --git a/hscontrol/servertest/ha_property_test.go b/hscontrol/servertest/ha_property_test.go index 35a229203..031b3942d 100644 --- a/hscontrol/servertest/ha_property_test.go +++ b/hscontrol/servertest/ha_property_test.go @@ -213,7 +213,7 @@ func checkHAInvariants( } // Invariant 1: primary IsOnline in the NodeStore view while any - // candidate is healthy. state.Disconnect's 10s grace can leave a + // candidate is healthy. state.Disconnect's 15s grace can leave a // recently-disconnected node with IsOnline=true; the structural // rule still requires the bit before the snapshot may keep it as // primary over a healthy candidate. @@ -333,7 +333,7 @@ func snapshotPrimaries( // bypasses the prober entirely. // // - Disconnect/Reconnect goes through the real poll-session -// lifecycle: mapBatcher.RemoveNode flips IsConnected, the 10s +// lifecycle: mapBatcher.RemoveNode flips IsConnected, the 15s // grace period defers state.Disconnect, and state.Connect bumps // SessionEpoch on rejoin. The prober defers fresh-session probes // so a reconnect mid-cycle does not install a stale Unhealthy. diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index abd554a63..c2eb9d31a 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -774,7 +774,7 @@ func TestIssuesGracePeriod(t *testing.T) { h.Client(1).Disconnect(t) - h.Client(0).WaitForCondition(t, "peer offline", 20*time.Second, + h.Client(0).WaitForCondition(t, "peer offline", 25*time.Second, func(nm *netmap.NetworkMap) bool { for _, p := range nm.Peers { hi := p.Hostinfo() @@ -791,9 +791,10 @@ func TestIssuesGracePeriod(t *testing.T) { elapsed := time.Since(disconnectTime) t.Logf("offline status arrived after %v", elapsed) - assert.Greater(t, elapsed, 8*time.Second, + // SaaS tells peers after about 15 seconds. + assert.Greater(t, elapsed, 14*time.Second, "offline status arrived too quickly -- grace period may not be working") - assert.Less(t, elapsed, 20*time.Second, + assert.Less(t, elapsed, 25*time.Second, "offline status took too long -- propagation delay issue") }) @@ -824,7 +825,7 @@ func TestIssuesGracePeriod(t *testing.T) { ephemeral.Disconnect(t) - // Grace period (10s) + ephemeral GC timeout (3s) + propagation. + // Grace period (15s) + ephemeral GC timeout (3s) + propagation. // Use a generous timeout for CI environments under load. regular.WaitForCondition(t, "ephemeral peer removed", 60*time.Second, func(nm *netmap.NetworkMap) bool { diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index b40e9eca4..9fb597eba 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -302,7 +302,8 @@ func holdChecks( tick := time.NewTicker(100 * time.Millisecond) defer tick.Stop() - for deadline := time.Now().Add(15 * time.Second); held < holdTicks && time.Now().Before(deadline); <-tick.C { + // Past the reconnect grace, so a node that left can read offline. + for deadline := time.Now().Add(25 * time.Second); held < holdTicks && time.Now().Before(deadline); <-tick.C { settled := true for i, c := range checks { diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index 409fc9c23..d92582aef 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -69,7 +69,7 @@ func TestConnectionLifecycle(t *testing.T) { h.Client(2).Disconnect(t) - // After the 10-second grace period, the remaining clients + // After the 15-second grace period, the remaining clients // should see the departed node as offline. The peer stays // in the peer list (non-ephemeral nodes are not removed). h.Client(0).WaitForCondition(t, "peer goes offline", 30*time.Second, @@ -693,7 +693,7 @@ func TestNodeExpiryRouteFailover(t *testing.T) { // unknown peers) match the real Tailscale client. func TestLogoutReloginAllClientsConverge(t *testing.T) { if testing.Short() { - t.Skip("relogin convergence test includes 10s+ disconnect grace per iteration") + t.Skip("relogin convergence test includes 15s+ disconnect grace per iteration") } const ( @@ -758,7 +758,7 @@ func TestLogoutReloginAllClientsConverge(t *testing.T) { // older releases, so the rapid session replacement is the prime suspect. func TestLogoutReloginWithPollChurn(t *testing.T) { if testing.Short() { - t.Skip("relogin convergence test includes 10s+ disconnect grace per iteration") + t.Skip("relogin convergence test includes 15s+ disconnect grace per iteration") } const ( @@ -827,7 +827,7 @@ func TestLogoutReloginWithPollChurn(t *testing.T) { // logoutAllAndWaitOffline logs every client out concurrently, then blocks // until the server reports each node expired and offline — the integration -// tests' logout barrier, including the ~10s disconnect grace period. +// tests' logout barrier, including the ~15s disconnect grace period. func logoutAllAndWaitOffline(t *testing.T, h *servertest.TestHarness) { t.Helper() diff --git a/hscontrol/servertest/poll_race_test.go b/hscontrol/servertest/poll_race_test.go index beaf6fc42..beccc72ce 100644 --- a/hscontrol/servertest/poll_race_test.go +++ b/hscontrol/servertest/poll_race_test.go @@ -22,7 +22,7 @@ func TestPollRace(t *testing.T) { t.Parallel() // The core race: when a node disconnects, poll.go starts a - // grace period goroutine (10s ticker loop). If the node + // grace period goroutine (15s ticker loop). If the node // reconnects during this period, the new session calls // [state.State.Connect] to mark the node online. But the old grace period // goroutine is still running and may call [state.State.Disconnect] AFTER @@ -98,7 +98,7 @@ func TestPollRace(t *testing.T) { "server should STILL show node as online 2s after reconnect (grace period goroutine should not overwrite)") }) - // Wait the full grace period (10s) after reconnect. The old + // Wait the full grace period (15s) after reconnect. The old // grace period goroutine should have checked IsConnected // and found the node connected, so should NOT have called // [state.State.Disconnect]. diff --git a/hscontrol/servertest/race_test.go b/hscontrol/servertest/race_test.go index d742d43c6..d27dd8325 100644 --- a/hscontrol/servertest/race_test.go +++ b/hscontrol/servertest/race_test.go @@ -310,7 +310,7 @@ func TestRaceConcurrentServerMutations(t *testing.T) { func TestRaceConnectDuringGracePeriod(t *testing.T) { t.Parallel() - // A node disconnects, and during the 10-second grace period + // A node disconnects, and during the 15-second grace period // a new node joins. The new node should see the disconnecting // node as a peer (it hasn't been removed yet). t.Run("new_node_during_grace_period", func(t *testing.T) { diff --git a/hscontrol/servertest/routes_test.go b/hscontrol/servertest/routes_test.go index ad0e101a1..7afeec462 100644 --- a/hscontrol/servertest/routes_test.go +++ b/hscontrol/servertest/routes_test.go @@ -239,7 +239,7 @@ func TestRoutes(t *testing.T) { 10*time.Second, 100*time.Millisecond, "r1 should be primary initially") - // 1. Take r1 offline. After the 10s grace period, r2 should take over. + // 1. Take r1 offline. After the 15s grace period, r2 should take over. r1.Disconnect(t) assert.Eventually(t, func() bool { return hasRoute(r2ID) && !hasRoute(r1ID) }, 20*time.Second, 200*time.Millisecond, diff --git a/hscontrol/servertest/weather_test.go b/hscontrol/servertest/weather_test.go index 5a8e33ee0..073786319 100644 --- a/hscontrol/servertest/weather_test.go +++ b/hscontrol/servertest/weather_test.go @@ -35,7 +35,7 @@ func TestNetworkWeather(t *testing.T) { h.Client(0).Disconnect(t) - // Reconnect quickly (well within the 10-second grace period). + // Reconnect quickly (well within the 15-second grace period). h.Client(0).ReconnectAfter(t, 1*time.Second) h.WaitForMeshComplete(t, 15*time.Second)