From 9de00278680adb7c87fce922a94e60c1f66b6774 Mon Sep 17 00:00:00 2001
From: "cursor[bot]" <206951365+cursor[bot]@users.noreply.github.com>
Date: Tue, 28 Jul 2026 23:17:19 -0400
Subject: [PATCH] [MM-69982] Fix intermittent remote cluster ping failures from
 stale keep-alive connection reuse (#37694)

* [MM-69982] Fix intermittent remote cluster ping failures from stale keep-alive reuse

The remote cluster HTTP transport set IdleConnTimeout to 90s while pings fire
every PingFreq (60s) and peers close idle keep-alive connections after their own
IdleTimeout (default 60s). Because 90s > 60s, the pooled ping connection outlived
the peer's reaping, so each 60s ping reused a connection the peer had already
closed, racing the FIN and failing intermittently with EOF / connection reset.

Set IdleConnTimeout to PingFreq/2 so the pool always discards the connection
before the next ping fires, making the reuse race structurally impossible while
keeping the invariant (IdleConnTimeout < PingFreq) explicit and tied to PingFreq.

Co-authored-by: mattermost-code <matty-code@mattermost.com>

* chore: retrigger Server CI after check-style Docker Hub flake

Server CI check-style failed pulling buildenv (Docker Hub Client.Timeout /
missing buildenv-image artifact); unrelated to IdleConnTimeout changes.
Enterprise CI check-style already passed on the same commit.

Co-authored-by: mattermost-code <matty-code@mattermost.com>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: mattermost-code <matty-code@mattermost.com>
---
 .../services/remotecluster/service.go         | 12 ++++++----
 .../services/remotecluster/service_test.go    | 23 +++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/server/platform/services/remotecluster/service.go b/server/platform/services/remotecluster/service.go
index 9ffa20c057b..b389a813164 100644
--- a/server/platform/services/remotecluster/service.go
+++ b/server/platform/services/remotecluster/service.go
@@ -116,10 +116,14 @@ func NewRemoteClusterService(server ServerIface, app AppIface) (*Service, error)
 			KeepAlive: 30 * time.Second,
 			DualStack: true,
 		}).DialContext,
-		ForceAttemptHTTP2:     true,
-		MaxIdleConns:          200,
-		MaxIdleConnsPerHost:   2,
-		IdleConnTimeout:       90 * time.Second,
+		ForceAttemptHTTP2:   true,
+		MaxIdleConns:        200,
+		MaxIdleConnsPerHost: 2,
+		// Must stay strictly below PingFreq so the pool always discards a connection
+		// before the next ping reuses it. Otherwise pings race the peer reaping its own
+		// idle keep-alive connections (ServiceSettings.IdleTimeout, default 60s) and fail
+		// intermittently with stale-connection errors. See MM-69982.
+		IdleConnTimeout:       PingFreq / 2,
 		TLSHandshakeTimeout:   10 * time.Second,
 		ExpectContinueTimeout: 1 * time.Second,
 		DisableCompression:    false,
diff --git a/server/platform/services/remotecluster/service_test.go b/server/platform/services/remotecluster/service_test.go
index 7b03e9b7b40..8a124f493fa 100644
--- a/server/platform/services/remotecluster/service_test.go
+++ b/server/platform/services/remotecluster/service_test.go
@@ -83,6 +83,29 @@ func TestService_AddTopicListener(t *testing.T) {
 	assert.Empty(t, listeners)
 }
 
+func TestRemoteClusterTransportIdleConnTimeout(t *testing.T) {
+	mockServer := newMockServer(t, nil)
+	mockApp := newMockApp(t, nil)
+
+	service, err := NewRemoteClusterService(mockServer, mockApp)
+	require.NoError(t, err)
+
+	transport, ok := service.httpClient.Transport.(*http.Transport)
+	require.True(t, ok, "expected the remote cluster client to use an *http.Transport")
+
+	// Regression guard for MM-69982: the pooled-connection idle timeout must be
+	// strictly less than PingFreq. If it is >= PingFreq, a pooled connection
+	// survives from one ping to the next and every ping reuses a connection that
+	// the peer has likely already reaped (peers close idle keep-alives after their
+	// own IdleTimeout, default 60s), producing intermittent stale-connection ping
+	// failures. Keeping it below PingFreq means the pool discards the connection
+	// before the next ping, so each ping dials fresh and the race cannot occur.
+	require.Positive(t, transport.IdleConnTimeout, "IdleConnTimeout must be set")
+	assert.Less(t, transport.IdleConnTimeout, PingFreq,
+		"IdleConnTimeout (%s) must be strictly less than PingFreq (%s) to avoid stale keep-alive reuse on pings",
+		transport.IdleConnTimeout, PingFreq)
+}
+
 // leaderAwareMockServer is a mock server that supports toggling leader state
 // and firing leader-change listeners, allowing lifecycle tests to simulate
 // HA cluster leader transitions.