From f2858ae4300a1815a57e865e6bd3a7a869c83825 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 22 Oct 2026 11:36:10 +0100 Subject: [PATCH] Fail connection-dropped commands with RedisConnectionException, never raw or cancelled (#3283) When a connection failed, most queued commands failed with RedisConnectionException but a few received the raw transport exception (IOException, or the RESPite writer's InvalidOperationException) or a TaskCanceledException with no token in play. Three sources, all confirmed with the reporter's repro: - RecordConnectionFailed builds its detailed RedisConnectionException only for the bridge's current connection and only on the first report; a repeat report (the write path and the read loop both reporting), or one against a connection the bridge has already replaced, drained the queue with the raw inner exception - or with null, which the result box turns into a cancellation (every EOF path in the read loop reports with no exception). - The write-lock catch fails the message properly and then reports the connection, which drained the same still-queued message again with the raw exception; last write wins. - FailDueToNoConnection, TimedOutBeforeWrite and the fail-fast bin completed the message via Cancel(); ExecuteAsyncImpl then races to fault the task with the real exception, and the box's thread-pool continuation sometimes wins. A batch has no caller translating the result at all. Fix: the drain always uses a RedisConnectionException (wrapping whatever it was given, with the failure type), and the bridge's refusals set the exception the caller path would have built instead of cancelling. Message.Cancel itself is unchanged; transactions rely on it for aborted conditions. ConnectionFailureExceptionTests covers the drain (null and IOException inner, on a bridge-less connection, i.e. the non-current path) and the no-connection refusal observed at the message's own task. Reporter's repro, 90 s: 884 TaskCanceledException + 415 InvalidOperationException before; none after. Fixes #3283 --- src/StackExchange.Redis/PhysicalBridge.cs | 22 +++--- src/StackExchange.Redis/PhysicalConnection.cs | 8 ++ .../ConnectionFailureExceptionTests.cs | 78 +++++++++++++++++++ 3 files changed, 98 insertions(+), 10 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index 7cf39edeb..eb81a8170 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -193,17 +193,18 @@ private WriteResult QueueOrFailMessage(Message message) } // Anything else goes in the bin - we're just not ready for you yet - message.Cancel(); - Multiplexer.OnMessageFaulted(message, null); - message.Complete(null); - return WriteResult.NoConnectionAvailable; + return FailDueToNoConnection(message); } private WriteResult FailDueToNoConnection(Message message) { - message.Cancel(); - Multiplexer.OnMessageFaulted(message, null); - message.Complete(null); + // fail with the exception the caller would otherwise construct from the result, rather than Cancel(): + // a cancelled box surfaces as TaskCanceledException, and although ExecuteAsyncImpl then tries to + // fault the task with the real exception, the box's continuation can get there first (#3283) - and a + // batch has no caller translating the result at all + var ex = ExceptionFactory.NoConnectionAvailable(Multiplexer, message, ServerEndPoint); + Multiplexer.OnMessageFaulted(message, ex); + message.SetExceptionAndComplete(ex, null); return WriteResult.NoConnectionAvailable; } @@ -1368,9 +1369,10 @@ public bool HasPendingCallerFacingItems() private WriteResult TimedOutBeforeWrite(Message message) { - message.Cancel(); - Multiplexer.OnMessageFaulted(message, null); - message.Complete(null); + // as FailDueToNoConnection: a real exception, not Cancel() + var ex = ExceptionFactory.Timeout(Multiplexer, null, message, ServerEndPoint, WriteResult.TimeoutBeforeWrite, this); + Multiplexer.OnMessageFaulted(message, ex); + message.SetExceptionAndComplete(ex, null); return WriteResult.TimeoutBeforeWrite; } diff --git a/src/StackExchange.Redis/PhysicalConnection.cs b/src/StackExchange.Redis/PhysicalConnection.cs index c960e4230..9e5eab6a1 100644 --- a/src/StackExchange.Redis/PhysicalConnection.cs +++ b/src/StackExchange.Redis/PhysicalConnection.cs @@ -696,6 +696,14 @@ void AddData(string? lk, string? sk, string? v) } var ex = innerException is RedisException ? innerException : outerException; + if (ex is not RedisException) + { + // the detailed exception above is built once, for the bridge's *current* connection; a repeat + // report for the same connection, or one the bridge has already replaced, must still fail + // whatever is queued - and with a RedisConnectionException, not the raw transport fault, nor + // (when there is no exception at all, e.g. EOF) a TaskCanceledException nobody asked for (#3283) + ex = new RedisConnectionException(failureType, CommandFlags.None, failureType + " on " + ToString(), innerException); + } nextMessage = Interlocked.Exchange(ref _awaitingToken, null); if (nextMessage is not null) diff --git a/tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs b/tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs new file mode 100644 index 000000000..79a8d84e1 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/ConnectionFailureExceptionTests.cs @@ -0,0 +1,78 @@ +using System; +using System.IO; +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// A command that fails because its connection failed must fail with a +/// carrying the transport fault as its inner exception: never the raw transport exception, and never a +/// when no caller token was involved (#3283). +/// +public class ConnectionFailureExceptionTests(ITestOutputHelper log) +{ + private static (Message Message, Task Task) CreateTrackedPing() + { + var message = Message.Create(-1, CommandFlags.None, RedisCommand.PING); + var box = TaskResultBox.Create(out var tcs, null); + message.SetSource(box, ResultProcessor.DemandOK); + return (message, tcs.Task); + } + + private static async Task AwaitFailure(Task task) + { + var completed = await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(10))); + Assert.Same(task, completed); + return await Assert.ThrowsAnyAsync(() => task); + } + + [Theory] + [InlineData(false)] // EOF-style report: no exception at all (used to surface as TaskCanceledException) + [InlineData(true)] // transport fault (used to surface raw when the detailed report was not built) + public async Task DrainedMessagesFailWithRedisConnectionException(bool withInner) + { + // a dummy connection has no bridge, so it is never the bridge's *current* connection and the detailed + // first-report exception is not built: this is exactly the path that handed out raw/null exceptions + using var stream = new MemoryStream(); + using var connection = PhysicalConnection.Dummy(stream); + var (message, task) = CreateTrackedPing(); + connection.EnqueueInsideWriteLock(message, enforceMuxer: false); + + var inner = withInner ? new IOException("simulated transport fault") : null; + connection.RecordConnectionFailed(ConnectionFailureType.SocketClosed, inner); + + var ex = await AwaitFailure(task); + log.WriteLine(ex.ToString()); + var conn = Assert.IsType(ex); + Assert.Equal(ConnectionFailureType.SocketClosed, conn.FailureType); + Assert.Same(inner, conn.InnerException); + } + + [Fact] + public async Task NoConnectionFailsWithRedisConnectionExceptionNotCancellation() + { + using var server = new InProcessTestServer(log); + var config = server.GetClientConfig(withPubSub: false, defaultOnly: true); + config.BacklogPolicy = BacklogPolicy.FailFast; + config.AbortOnConnectFail = false; + config.AllowSimulateConnectionFailure = true; + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + var endpoint = conn.GetServerSnapshot()[0]; + var bridge = endpoint.GetBridge(ConnectionType.Interactive)!; + + conn.AllowConnect = false; + endpoint.SimulateConnectionFailure(SimulatedFailureType.All); + Assert.False(conn.IsConnected); + + // write at the bridge, so we observe the message's own outcome rather than the caller's translation of + // the write result (which is what hid this for single commands - most of the time) + var (message, task) = CreateTrackedPing(); + Assert.Equal(WriteResult.NoConnectionAvailable, await bridge.TryWriteAsync(message, isReplica: false)); + + var ex = await AwaitFailure(task); + log.WriteLine(ex.ToString()); + Assert.IsType(ex); + Assert.False(task.IsCanceled); + } +}