From 037b8e91b32cdb102ea20b8be3ae6ee89c0f33a0 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Wed, 22 Jul 2026 22:13:07 -0700 Subject: [PATCH] Fix TimeSpan component-vs-total misuse in gossip client timeout GarnetServerNode computed the gossip GarnetClient's per-operation timeout as `TimeSpan.FromSeconds(opts.ClusterTimeout).Milliseconds`, which returns the sub-second component (0-999), not the total. For any whole-second ClusterTimeout (default 60s) this yields 0, silently disabling the client's TimeoutChecker so gossip operations never time out. - GarnetServerNode: use TotalMilliseconds via a testable GetClientTimeoutMilliseconds helper. - Add GarnetServerNodeTimeoutTests (7 cases) covering the regression. - GarnetServerOptions: LoggingFrequency used TimeSpan.FromSeconds(5).Seconds (correct only because 5<60); simplify to the int-seconds literal. - RespAofTests: TTL assertions used .Milliseconds (component, always <8500), a silent no-op; use .TotalMilliseconds with the correct 10s ceiling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: de1c4e90-5293-4056-8783-1779a318e774 --- .../cluster/Server/Gossip/GarnetServerNode.cs | 5 +- libs/server/Servers/GarnetServerOptions.cs | 2 +- .../GarnetServerNodeTimeoutTests.cs | 47 +++++++++++++++++++ .../Garnet.test.scripting/RespAofTests.cs | 4 +- 4 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs diff --git a/libs/cluster/Server/Gossip/GarnetServerNode.cs b/libs/cluster/Server/Gossip/GarnetServerNode.cs index cf1dcd49564..6c477e95b45 100644 --- a/libs/cluster/Server/Gossip/GarnetServerNode.cs +++ b/libs/cluster/Server/Gossip/GarnetServerNode.cs @@ -66,6 +66,9 @@ internal sealed class GarnetServerNode /// const int defaultMaxOutstandingTask = 8; + internal static int GetClientTimeoutMilliseconds(int clusterTimeoutSeconds) + => clusterTimeoutSeconds <= 0 ? 0 : (int)Math.Min((long)clusterTimeoutSeconds * 1000, int.MaxValue); + /// /// GarnetServerNode constructor /// @@ -83,7 +86,7 @@ public GarnetServerNode(ClusterProvider clusterProvider, EndPoint endpoint, SslC tlsOptions, sendPageSize: opts.DisablePubSub ? defaultSendPageSize : Math.Max(defaultSendPageSize, (int)opts.PubSubPageSizeBytes()), maxOutstandingTasks: defaultMaxOutstandingTask, - timeoutMilliseconds: opts.ClusterTimeout <= 0 ? 0 : TimeSpan.FromSeconds(opts.ClusterTimeout).Milliseconds, + timeoutMilliseconds: GetClientTimeoutMilliseconds(opts.ClusterTimeout), authUsername: clusterProvider.clusterManager.clusterProvider.ClusterUsername, authPassword: clusterProvider.clusterManager.clusterProvider.ClusterPassword, epoch: epoch, diff --git a/libs/server/Servers/GarnetServerOptions.cs b/libs/server/Servers/GarnetServerOptions.cs index f847c9d7447..45f5f41174d 100644 --- a/libs/server/Servers/GarnetServerOptions.cs +++ b/libs/server/Servers/GarnetServerOptions.cs @@ -261,7 +261,7 @@ public class GarnetServerOptions : ServerOptions /// /// Frequency (in seconds) of logging (used for tracking progress of long running operations e.g. migration) /// - public int LoggingFrequency = TimeSpan.FromSeconds(5).Seconds; + public int LoggingFrequency = 5; /// /// Metrics sampling frequency diff --git a/test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs b/test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs new file mode 100644 index 00000000000..ee07dd3c47c --- /dev/null +++ b/test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs @@ -0,0 +1,47 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using Garnet.cluster; +using NUnit.Framework; +using NUnit.Framework.Legacy; + +namespace Garnet.test.cluster +{ + /// + /// Regression tests for the gossip-connection client timeout computation + /// (), which previously used + /// TimeSpan.Milliseconds (sub-second component) instead of the full millisecond total. + /// + [TestFixture] + public class GarnetServerNodeTimeoutTests : TestBase + { + [Test] + [TestCase(1, 1_000)] + [TestCase(5, 5_000)] + [TestCase(60, 60_000)] // production default; buggy code yielded 0 here + [TestCase(3600, 3_600_000)] + public void ClientTimeoutUsesTotalMilliseconds(int clusterTimeoutSeconds, int expectedMilliseconds) + { + var actual = GarnetServerNode.GetClientTimeoutMilliseconds(clusterTimeoutSeconds); + ClassicAssert.AreEqual(expectedMilliseconds, actual); + } + + [Test] + [TestCase(0)] + [TestCase(-1)] + public void NonPositiveClusterTimeoutDisablesClientTimeout(int clusterTimeoutSeconds) + { + var actual = GarnetServerNode.GetClientTimeoutMilliseconds(clusterTimeoutSeconds); + ClassicAssert.AreEqual(0, actual); + } + + [Test] + [TestCase(2_147_484)] // *1000 overflows int; naive cast would wrap negative and disable the timeout + [TestCase(int.MaxValue)] + public void LargeClusterTimeoutClampsToIntMaxValue(int clusterTimeoutSeconds) + { + var actual = GarnetServerNode.GetClientTimeoutMilliseconds(clusterTimeoutSeconds); + ClassicAssert.AreEqual(int.MaxValue, actual); + } + } +} \ No newline at end of file diff --git a/test/standalone/Garnet.test.scripting/RespAofTests.cs b/test/standalone/Garnet.test.scripting/RespAofTests.cs index 34899e7e108..567c0fa13d9 100644 --- a/test/standalone/Garnet.test.scripting/RespAofTests.cs +++ b/test/standalone/Garnet.test.scripting/RespAofTests.cs @@ -333,7 +333,7 @@ public async Task AofExpiryRMWStoreRecoverTestAsync() // Verify 2nd string ttl var recoveredValueTtl = db.KeyTimeToLive("AofExpiryRMWStoreRecoverTestKey2"); ClassicAssert.IsTrue(recoveredValueTtl.HasValue); - ClassicAssert.Less(recoveredValueTtl.Value.Milliseconds, 8500); + ClassicAssert.LessOrEqual(recoveredValueTtl.Value.TotalMilliseconds, 10_000); ClassicAssert.Greater(recoveredValueTtl.Value.TotalSeconds, 0); } @@ -409,7 +409,7 @@ public async Task AofExpiryUpsertStoreRecoverTestAsync() // Verify 2nd string ttl var recoveredValueTtl = db.KeyTimeToLive("AofExpiryUpsertStoreRecoverTestKey2"); ClassicAssert.IsTrue(recoveredValueTtl.HasValue); - ClassicAssert.Less(recoveredValueTtl.Value.Milliseconds, 8500); + ClassicAssert.LessOrEqual(recoveredValueTtl.Value.TotalMilliseconds, 10_000); ClassicAssert.Greater(recoveredValueTtl.Value.TotalSeconds, 0); }