mirror of
https://github.com/mattermost/mattermost.git
synced 2026-09-19 02:06:37 +08:00
[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>
This commit is contained in:
co-authored by
mattermost-code
Cursor Agent
parent
75823b96c3
commit
9de0027868
@@ -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,
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user