From 88a0d456c010e1a64ca822acb6fe881d1410c8f3 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:43:22 +0100 Subject: [PATCH 01/10] Detect a dead transport at the interval RTN23a specifies RTN23a - implemented. The monitor measured idleness against connectionStateTtl, so a silently dead transport took 120s to notice instead of ~25s. RTN23b - implemented. heartbeats=true was never sent, so protocol HEARTBEATs relied on an undocumented server default that heartbeats=false would switch off. CD2h - implemented. maxIdleInterval was never parsed from connectionDetails. Scoped to the transport that carried it, so a new transport does not inherit it and an RTN24 update does not withdraw it. RTN21 - fixed. A CONNECTED with no connectionDetails emptied the connectionKey, leaving a live connection with nothing to resume with. RTN15g2 - completed, incidentally. Parsing maxIdleInterval supplies the term the freshness window was missing: the measure is now the gap between the last sign of activity and the sum of connectionStateTtl and maxIdleInterval, not connectionStateTtl alone. Note the clause is deleted as of spec 6.1.0 and replaced by RTN14h, which requires a resume to be attempted regardless of how long it has been - that is not adopted here, and is coupled to the SUSPENDED key clearing in RTN8d/RTN9d. Widening the window does move behaviour toward RTN14h, since connection state is discarded less often than before. Co-Authored-By: Claude Opus 5 --- ...Ably_ConnectionDetailsMessageSerializer.cs | 61 +++++- .../Realtime/Workflows/RealtimeCommands.cs | 27 +-- .../Realtime/Workflows/RealtimeState.cs | 74 +++++++- .../Realtime/Workflows/RealtimeWorkflow.cs | 178 ++++++++++++++++-- .../Transport/ConnectionInfo.cs | 7 + .../Transport/TransportParams.cs | 6 + src/IO.Ably.Shared/Types/ConnectionDetails.cs | 10 + src/IO.Ably.Shared/Types/ErrorInfo.cs | 12 ++ .../IO.Ably.Tests.Shared.projitems | 1 + .../ConnectionParameterSpecs.cs | 31 +++ .../Realtime/ConnectionStateFreshnessSpecs.cs | 142 ++++++++++++++ .../Realtime/ProtocolMessageTests.cs | 38 ++++ 12 files changed, 553 insertions(+), 34 deletions(-) create mode 100644 src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs diff --git a/src/IO.Ably.Shared.MsgPack/CustomSerialisers/GeneratedSerializers/IO_Ably_ConnectionDetailsMessageSerializer.cs b/src/IO.Ably.Shared.MsgPack/CustomSerialisers/GeneratedSerializers/IO_Ably_ConnectionDetailsMessageSerializer.cs index 250f33936..4b5648c42 100644 --- a/src/IO.Ably.Shared.MsgPack/CustomSerialisers/GeneratedSerializers/IO_Ably_ConnectionDetailsMessageSerializer.cs +++ b/src/IO.Ably.Shared.MsgPack/CustomSerialisers/GeneratedSerializers/IO_Ably_ConnectionDetailsMessageSerializer.cs @@ -35,7 +35,7 @@ public IO_Ably_ConnectionDetailsMessageSerializer(MsgPack.Serialization.Serializ } protected override void PackToCore(MsgPack.Packer packer, IO.Ably.ConnectionDetails objectTree) { - packer.PackMapHeader(7); + packer.PackMapHeader(8); this._serializer0.PackTo(packer, "clientId"); this._serializer0.PackTo(packer, objectTree.ClientId); this._serializer0.PackTo(packer, "connectionKey"); @@ -44,6 +44,8 @@ protected override void PackToCore(MsgPack.Packer packer, IO.Ably.ConnectionDeta this._serializer1.PackTo(packer, objectTree.ConnectionStateTtl); this._serializer0.PackTo(packer, "maxFrameSize"); this._serializer2.PackTo(packer, objectTree.MaxFrameSize); + this._serializer0.PackTo(packer, "maxIdleInterval"); + this._serializer1.PackTo(packer, objectTree.MaxIdleInterval); this._serializer0.PackTo(packer, "maxInboundRate"); this._serializer2.PackTo(packer, objectTree.MaxInboundRate); this._serializer0.PackTo(packer, "maxMessageSize"); @@ -112,6 +114,33 @@ protected override IO.Ably.ConnectionDetails UnpackFromCore(MsgPack.Unpacker unp result.MaxFrameSize = nullable2.Value; } unpacked = (unpacked + 1); + System.Nullable nullableMaxIdleInterval = default(System.Nullable); + if ((unpacked < itemsCount)) { + if ((unpacker.Read() == false)) { + throw MsgPack.Serialization.SerializationExceptions.NewMissingItem(4); + } + if (((unpacker.IsArrayHeader == false) + && (unpacker.IsMapHeader == false))) { + nullableMaxIdleInterval = this._serializer1.UnpackFrom(unpacker); + } + else { + MsgPack.Unpacker disposableMaxIdleInterval = default(MsgPack.Unpacker); + disposableMaxIdleInterval = unpacker.ReadSubtree(); + try { + nullableMaxIdleInterval = this._serializer1.UnpackFrom(disposableMaxIdleInterval); + } + finally { + if (((disposableMaxIdleInterval == null) + == false)) { + disposableMaxIdleInterval.Dispose(); + } + } + } + } + if (nullableMaxIdleInterval.HasValue) { + result.MaxIdleInterval = nullableMaxIdleInterval; + } + unpacked = (unpacked + 1); System.Nullable nullable3 = default(System.Nullable); if ((unpacked < itemsCount)) { nullable3 = MsgPack.Serialization.UnpackHelpers.UnpackNullableInt64Value(unpacker, typeof(IO.Ably.ConnectionDetails), "Int64 maxInboundRate"); @@ -230,7 +259,35 @@ protected override IO.Ably.ConnectionDetails UnpackFromCore(MsgPack.Unpacker unp } } else { - unpacker.Skip(); + if ((key == "maxIdleInterval")) { + System.Nullable nullableMaxIdle = default(System.Nullable); + if ((unpacker.Read() == false)) { + throw MsgPack.Serialization.SerializationExceptions.NewMissingItem(i); + } + if (((unpacker.IsArrayHeader == false) + && (unpacker.IsMapHeader == false))) { + nullableMaxIdle = this._serializer1.UnpackFrom(unpacker); + } + else { + MsgPack.Unpacker disposableMaxIdle = default(MsgPack.Unpacker); + disposableMaxIdle = unpacker.ReadSubtree(); + try { + nullableMaxIdle = this._serializer1.UnpackFrom(disposableMaxIdle); + } + finally { + if (((disposableMaxIdle == null) + == false)) { + disposableMaxIdle.Dispose(); + } + } + } + if (nullableMaxIdle.HasValue) { + result.MaxIdleInterval = nullableMaxIdle; + } + } + else { + unpacker.Skip(); + } } } } diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs index 84529ade3..2fd70e820 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs @@ -485,24 +485,27 @@ protected override string ExplainData() } } + /// + /// A periodic tick asking the workflow to check whether the current transport has gone idle for + /// longer than RTN23a allows. Carries no state: the handler reads RealtimeState on the workflow + /// thread rather than the timer sampling it off-thread. + /// internal class HeartbeatMonitorCommand : RealtimeCommand { - private HeartbeatMonitorCommand(DateTimeOffset? confirmedAliveAt, TimeSpan connectionStateTtl) + private HeartbeatMonitorCommand(DateTimeOffset queuedAt) { - ConfirmedAliveAt = confirmedAliveAt; - ConnectionStateTtl = connectionStateTtl; + QueuedAt = queuedAt; } - public DateTimeOffset? ConfirmedAliveAt { get; } + /// + /// When this tick was queued, which is the moment idleness is judged against. Reading the + /// clock in the handler would charge any wait behind a slow command to the transport. + /// + public DateTimeOffset QueuedAt { get; } - public TimeSpan ConnectionStateTtl { get; } + public static HeartbeatMonitorCommand Create(DateTimeOffset queuedAt) => + new HeartbeatMonitorCommand(queuedAt); - public static HeartbeatMonitorCommand Create(DateTimeOffset? confirmedAliveAt, TimeSpan connectionStateTtl) => - new HeartbeatMonitorCommand(confirmedAliveAt, connectionStateTtl); - - protected override string ExplainData() - { - return $"ConfirmedAliveAt: {ConfirmedAliveAt}. ConnectionStateTtl {ConnectionStateTtl}"; - } + protected override string ExplainData() => $"QueuedAt: {QueuedAt:O}"; } } diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs index 07515f861..78e881931 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs @@ -40,6 +40,14 @@ public ConnectionData(List fallbackHosts) public TimeSpan ConnectionStateTtl { get; internal set; } = Defaults.ConnectionStateTtl; + /// + /// The maximum period of inactivity the server promises in the server to client + /// direction, from the connectionDetails of the most recent Connected message. + /// Null when the server declines to make that promise, in which case no idle + /// timeout is applied (CD2h). + /// + public TimeSpan? MaxIdleInterval { get; internal set; } + /// /// Information relating to the transition to the current state, /// as an Ably ErrorInfo object. This contains an error code and @@ -74,17 +82,77 @@ public ConnectionStateChange UpdateState(ConnectionStateBase state, ILogger logg public bool HasConnectionStateTtlPassed(Func now) { - return ConfirmedAliveAt?.Add(ConnectionStateTtl) < now(); + if (ConfirmedAliveAt.HasValue == false) + { + // Nothing has ever been received on this client, so there is no connection + // state to consider stale. + return false; + } + + // RTN15g2 - the window is connectionStateTtl plus maxIdleInterval, measured from + // the last known sign of activity from Ably rather than from when we left the + // Connected state. A device that slept may only have left Connected moments ago + // having last actually heard from Ably hours earlier. + // Clamped at zero rather than coalesced, because nothing between the wire and here + // validates the sign: TimeSpanJsonConverter will hand back a negative TimeSpan for a + // negative number and Update assigns it as-is. A negative value would make the + // subtraction below throw OverflowException, which is precisely the failure this + // method was rewritten to remove - the throw escapes HandleSetStateCommand's + // AblyException-only catch, gets logged and dropped by the command loop, and leaves + // the client wedged in DISCONNECTED with no transport and no retry. The RTN23a + // monitor already treats a non-positive interval as no promise at all. + var maxIdleInterval = MaxIdleInterval > TimeSpan.Zero ? MaxIdleInterval.Value : TimeSpan.Zero; + + if (ConnectionStateTtl >= TimeSpan.MaxValue - maxIdleInterval) + { + // The window cannot be represented, so it can never elapse. + return false; + } + + // Deliberately a subtraction rather than ConfirmedAliveAt + window. Adding to a + // DateTimeOffset throws ArgumentOutOfRangeException once the result runs past + // DateTimeOffset.MaxValue, and that exception escaped into the command loop where + // it was logged and dropped - silently abandoning whichever state transition was + // in progress. Comparing two durations cannot fail that way. + return now() - ConfirmedAliveAt.Value > ConnectionStateTtl + maxIdleInterval; } - public void Update(ConnectionInfo info) + public void Update(ConnectionInfo info, bool isUpdate) { + // Guarded differently on purpose. connectionId is a top-level field and always + // meaningful, per RTN8b. connectionKey lives inside connectionDetails, and RTN21 + // scopes an override to "the attributes within ConnectionDetails" - so a CONNECTED + // carrying none overrides no key, and emptying it would leave a live connection with + // nothing to resume with under RTN15b. Clearing the key belongs to ClearKey and + // ClearKeyAndId, at the points that mean it. Id = info.ConnectionId; - Key = info.ConnectionKey; + + if (info.ConnectionKey.IsNotEmpty()) + { + Key = info.ConnectionKey; + } + if (info.ConnectionStateTtl.HasValue) { ConnectionStateTtl = info.ConnectionStateTtl.Value; } + + // RTN23a measures against the maxIdleInterval "sent in the connectionDetails of the + // most recent CONNECTED message received on that transport", so the promise belongs + // to the transport that carried it. Hence the two cases, which isUpdate separates: + // + // - A CONNECTED starting a new transport must not inherit the old threshold. An + // omitted field is Ably declining to promise anything, so detection stands down. + // Strictly unspecified - RTN23a says the field "will be sent" and CD2h licenses + // arbitrary inactivity for an explicit 0 - but failing open matches CD2h's + // outcome for 0, and ably-js. + // - A CONNECTED arriving while already CONNECTED is an RTN24 update on the + // transport we already hold, so an omitted field is not a withdrawal and the + // previous value stands. ably-js keeps it with the same guard. + if (isUpdate == false || info.MaxIdleInterval.HasValue) + { + MaxIdleInterval = info.MaxIdleInterval; + } } public void ClearKeyAndId() diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index c6fcdb02e..e3f040057 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -34,6 +34,11 @@ internal sealed class RealtimeWorkflow : IQueueCommand, IDisposable // way of figuring out when processing has finished private volatile bool _processingCommand; private bool _heartbeatMonitorDisconnectRequested; + + // Null until first asked. See ProtocolHeartbeatsNotRequestedByCaller. + private bool? _protocolHeartbeatsNotRequested; + + private bool _warnedIdleCheckInactive; private bool _disposedValue; private AblyRealtime Client { get; } @@ -108,7 +113,7 @@ public void Start() { while (true) { - QueueCommand(HeartbeatMonitorCommand.Create(Connection.ConfirmedAliveAt, Connection.ConnectionStateTtl).TriggeredBy("AblyRealtime.HeartbeatMonitor()")); + QueueCommand(HeartbeatMonitorCommand.Create(Now()).TriggeredBy("AblyRealtime.HeartbeatMonitor()")); await Task.Delay(Client.Options.HeartbeatMonitorDelay, _heartbeatMonitorCancellationTokenSource.Token); } }, @@ -196,7 +201,8 @@ private void DelayCommandHandler(TimeSpan delay, RealtimeCommand cmd) => internal async Task> ProcessCommand(RealtimeCommand command) { - bool shouldLogCommand = !((command is EmptyCommand) || (command is ListCommand)); + // Ticks every second, so logging each one would bury everything else at Debug. + bool shouldLogCommand = !((command is EmptyCommand) || (command is ListCommand) || (command is HeartbeatMonitorCommand)); try { if (Logger.IsDebug && shouldLogCommand) @@ -233,7 +239,7 @@ internal async Task> ProcessCommand(RealtimeCommand State.Connection.CurrentStateObject?.AbortTimer(); return Enumerable.Empty(); case HeartbeatMonitorCommand cmd: - return await HandleHeartbeatMonitorCommand(cmd); + return HandleHeartbeatMonitorCommand(cmd); default: var next = await ProcessCommandInner(command); return new[] @@ -251,31 +257,96 @@ internal async Task> ProcessCommand(RealtimeCommand } } - private async Task> HandleHeartbeatMonitorCommand(HeartbeatMonitorCommand command) + /// + /// RTN23a - a transport silent for longer than maxIdleInterval plus realtimeRequestTimeout + /// is treated as dead and disconnected. Any inbound message counts as activity, not just + /// Heartbeats, which is why ProcessMessage refreshes the timestamp rather than the Heartbeat + /// handler. Data we send does not count. + /// + private IEnumerable HandleHeartbeatMonitorCommand(HeartbeatMonitorCommand command) { - if (!command.ConfirmedAliveAt.HasValue) + var connection = State.Connection; + + // Only meaningful while Connected: elsewhere there is no live transport, and + // ConfirmedAliveAt may still hold a previous transport's timestamp. + if (connection.State != ConnectionState.Connected) { + _heartbeatMonitorDisconnectRequested = false; + + // Re-armed per transport, since maxIdleInterval is a per-transport promise. + _warnedIdleCheckInactive = false; return Enumerable.Empty(); } - TimeSpan delta = Now() - command.ConfirmedAliveAt.Value; - if (delta > command.ConnectionStateTtl) + // RTN23b - without protocol heartbeats Ably may satisfy maxIdleInterval with websocket + // ping frames, which this library cannot observe, leaving nothing to measure. + if (ProtocolHeartbeatsNotRequestedByCaller()) { - if (!_heartbeatMonitorDisconnectRequested) - { - _heartbeatMonitorDisconnectRequested = true; - return new RealtimeCommand[] { SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected).TriggeredBy(command) }; - } + return Enumerable.Empty(); } - else + + // No promised idle period to measure against. + var maxIdleInterval = connection.MaxIdleInterval; + if (maxIdleInterval.HasValue == false || maxIdleInterval.Value <= TimeSpan.Zero) { - if (_heartbeatMonitorDisconnectRequested) + if (_warnedIdleCheckInactive == false) { - _heartbeatMonitorDisconnectRequested = false; + _warnedIdleCheckInactive = true; + + // Logged because the two causes differ: absent means no CONNECTED carried the + // field, zero is CD2h's explicit "arbitrarily-long levels of inactivity". + Logger.Debug( + maxIdleInterval.HasValue + ? "Ably set maxIdleInterval to 0, so it guarantees no inactivity limit. Idle connection detection is off." + : "No maxIdleInterval received from Ably, so idle connection detection is off."); } + + return Enumerable.Empty(); + } + + if (connection.ConfirmedAliveAt.HasValue == false) + { + return Enumerable.Empty(); + } + + // Measured to when the tick was queued, not to now. The workflow is a single reader, so + // work queued ahead of the tick - an inbound AUTH awaiting the application's + // authCallback - would otherwise be charged to the transport. + var idleFor = command.QueuedAt - connection.ConfirmedAliveAt.Value; + + // A window that cannot be represented can never elapse. maxIdleInterval is unbounded off + // the wire, and an OverflowException here would be logged and dropped by the command + // loop, silently killing detection for the life of the connection. + if (maxIdleInterval.Value >= TimeSpan.MaxValue - Client.Options.RealtimeRequestTimeout) + { + return Enumerable.Empty(); + } + + var allowedIdleTime = maxIdleInterval.Value + Client.Options.RealtimeRequestTimeout; + + if (idleFor <= allowedIdleTime) + { + _heartbeatMonitorDisconnectRequested = false; + return Enumerable.Empty(); + } + + if (_heartbeatMonitorDisconnectRequested) + { + return Enumerable.Empty(); } - return Enumerable.Empty(); + _heartbeatMonitorDisconnectRequested = true; + + var error = ErrorInfo.NoActivityFrom(idleFor); + Logger.Warning($"{error.Message} The limit was {allowedIdleTime.TotalSeconds:0.#}s."); + + // RTN15a - a transport we have given up on counts as disconnected unexpectedly, so + // RTN15h3's immediate reconnect applies. Requested explicitly because NoActivityFrom + // carries 408, which the instant retry check does not recognise on its own. + return new RealtimeCommand[] + { + SetDisconnectedStateCommand.Create(error, retryInstantly: true).TriggeredBy(command), + }; } /// @@ -559,6 +630,79 @@ ErrorInfo GetErrorInfoFromTransportException(Exception ex, ErrorInfo @default) return EmptyCommand.Instance; } + /// + /// Whether the caller's own transportParams entry has displaced ours, leaving RTN23b's + /// protocol heartbeats unrequested. RTN23b guarantees them only for exactly + /// `heartbeats=true`; anything else lets Ably use transport-level pings, which + /// ClientWebSocket does not surface. + /// + /// true when protocol heartbeats have not been requested. + private bool ProtocolHeartbeatsNotRequestedByCaller() + { + // Answered once - TransportParams is fixed at client construction. + if (_protocolHeartbeatsNotRequested.HasValue) + { + return _protocolHeartbeatsNotRequested.Value; + } + + _protocolHeartbeatsNotRequested = ComputeProtocolHeartbeatsNotRequested(); + return _protocolHeartbeatsNotRequested.Value; + } + + private bool ComputeProtocolHeartbeatsNotRequested() + { + var transportParams = Client.Options.TransportParams; + if (transportParams == null) + { + return false; + } + + // Case-insensitive because DictionaryExtensions.Merge drops our own heartbeats param on a + // case-insensitive key match, so "Heartbeats" reaches the wire in place of ours. + var callerEntry = transportParams.FirstOrDefault(x => x.Key.EqualsTo("heartbeats")); + if (callerEntry.Key == null) + { + return false; + } + + // Two conditions, because Merge has already dropped ours on a case-insensitive key match: + // - the value must be "true"; RTN23b guarantees protocol heartbeats only for that, and + // treats false or unspecified as permission to use any transport-level mechanism. + // - the key must be exactly "heartbeats"; any other spelling is a param Ably ignores, + // which reads as unspecified. + string value; + try + { + value = callerEntry.Value?.ToString(); + } + catch (Exception ex) + { + // Unguarded, a throwing ToString would escape every tick and be dropped by the + // command loop, killing RTN23a silently. TransportParams.ConvertValue guards it too. + Logger.Error($"Could not read transportParams['{callerEntry.Key}'] as a string.", ex); + value = null; + } + + var keyIsExact = callerEntry.Key.EqualsTo("heartbeats", caseSensitive: true); + var valueIsTrue = value.EqualsTo("true"); + var disabled = keyIsExact == false || valueIsTrue == false; + + if (disabled) + { + // Named separately because the two halves need different fixes. + var reason = keyIsExact + ? $"transportParams sets heartbeats to '{value}', not 'true'." + : $"transportParams sets '{callerEntry.Key}'; Ably only reads 'heartbeats'."; + + Logger.Warning( + $"{reason} Ably may then keep this connection alive with websocket pings, which " + + "this library cannot see, so idle connection detection is off and a silently " + + "dropped connection will not be detected. Set heartbeats to 'true' to enable it."); + } + + return disabled; + } + private void SetNewHostInState(string newHost) { if (IsFallbackHost()) @@ -604,7 +748,7 @@ private void HandleConnectedCommand(SetConnectedStateCommand cmd) var failedResumeOrRecover = State.Connection.Id != info.ConnectionId && cmd.Message.Error != null; // RTN15c7, RTN16d - State.Connection.Update(info); // RTN16d, RTN15e + State.Connection.Update(info, cmd.IsUpdate); // RTN16d, RTN15e, RTN23a if (info.ClientId.IsNotEmpty()) { diff --git a/src/IO.Ably.Shared/Transport/ConnectionInfo.cs b/src/IO.Ably.Shared/Transport/ConnectionInfo.cs index d7f47269b..da36eb03b 100644 --- a/src/IO.Ably.Shared/Transport/ConnectionInfo.cs +++ b/src/IO.Ably.Shared/Transport/ConnectionInfo.cs @@ -33,6 +33,7 @@ public ConnectionInfo(ProtocolMessage message) ConnectionId = message.ConnectionId; ClientId = message.ConnectionDetails?.ClientId; ConnectionStateTtl = message.ConnectionDetails?.ConnectionStateTtl; + MaxIdleInterval = message.ConnectionDetails?.MaxIdleInterval; ConnectionKey = message.ConnectionDetails?.ConnectionKey; } @@ -41,6 +42,12 @@ public ConnectionInfo(ProtocolMessage message) /// public TimeSpan? ConnectionStateTtl { get; private set; } + /// + /// The maximum period of inactivity the server will allow in the server to client + /// direction before it sends a Heartbeat or transport level ping. See CD2h. + /// + public TimeSpan? MaxIdleInterval { get; private set; } + /// /// contains the client ID assigned to the connection. /// diff --git a/src/IO.Ably.Shared/Transport/TransportParams.cs b/src/IO.Ably.Shared/Transport/TransportParams.cs index 357a0943b..8b329573f 100644 --- a/src/IO.Ably.Shared/Transport/TransportParams.cs +++ b/src/IO.Ably.Shared/Transport/TransportParams.cs @@ -187,6 +187,12 @@ public Dictionary GetParams() result["format"] = UseBinaryProtocol ? "msgpack" : "json"; result["echo"] = EchoMessages.ToString().ToLower(); + // RTN23b - ask Ably to keep the connection alive with Heartbeat protocol messages rather + // than a transport level mechanism. ClientWebSocket cannot observe an incoming websocket + // ping frame, so protocol messages are the only activity RTN23a can measure. Overridable + // through ClientOptions.TransportParams, which take precedence in the merge below. + result["heartbeats"] = "true"; + // RTN15b - resume connection using connectionKey if (ConnectionKey.IsNotEmpty()) { diff --git a/src/IO.Ably.Shared/Types/ConnectionDetails.cs b/src/IO.Ably.Shared/Types/ConnectionDetails.cs index 095533fe3..772d40f5c 100644 --- a/src/IO.Ably.Shared/Types/ConnectionDetails.cs +++ b/src/IO.Ably.Shared/Types/ConnectionDetails.cs @@ -32,6 +32,16 @@ public class ConnectionDetails [JsonProperty("maxFrameSize")] public long MaxFrameSize { get; set; } + /// + /// The maximum length of time that the server will allow no activity to occur in the + /// server to client direction. After such a period of inactivity the server will send a + /// Heartbeat or a transport level ping. A value of zero means the server allows + /// arbitrarily long levels of inactivity and no idle timeout should be applied. + /// See CD2h - https://sdk.ably.com/builds/ably/specification/main/features/#CD2h. + /// + [JsonProperty("maxIdleInterval")] + public TimeSpan? MaxIdleInterval { get; set; } + /// /// Max inbound rate. /// diff --git a/src/IO.Ably.Shared/Types/ErrorInfo.cs b/src/IO.Ably.Shared/Types/ErrorInfo.cs index 03dfa9b59..cc8b4ea54 100644 --- a/src/IO.Ably.Shared/Types/ErrorInfo.cs +++ b/src/IO.Ably.Shared/Types/ErrorInfo.cs @@ -28,6 +28,18 @@ public class ErrorInfo internal const string ReasonPropertyName = "message"; internal const string HrefBase = "https://help.ably.io/error/"; + /// + /// The error used when a transport is disconnected for seeing no activity from Ably for + /// longer than RTN23a allows. The observed idle time is included to make it diagnosable. + /// + /// how long the transport has been idle. + /// a Disconnected ErrorInfo describing the idle period. + internal static ErrorInfo NoActivityFrom(TimeSpan idleFor) => + new ErrorInfo( + $"No activity from Ably for {idleFor.TotalSeconds:0.#}s, assuming the connection has dropped.", + ErrorCodes.Disconnected, + HttpStatusCode.RequestTimeout); + /// /// Ably error code (see https://github.com/ably/ably-common/blob/main/protocol/errors.json). /// diff --git a/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems b/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems index 2eace096f..ebc14b13b 100644 --- a/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems +++ b/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems @@ -89,6 +89,7 @@ + diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs index 740708b2a..3fef85a4e 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs @@ -52,6 +52,37 @@ public async Task WithEchoInClientOptions_ShouldSetTransportEchoCorrectly(bool e .WhoseValue.Should().Be(echo.ToString().ToLower()); } + // UTS: realtime/unit/RTN23a/heartbeats-true-query-param-0 + [Fact] + [Trait("spec", "RTN23b")] + public async Task ShouldRequestProtocolHeartbeats() + { + // RTN23b - ClientWebSocket cannot observe an incoming websocket ping frame, so protocol + // messages are the only activity RTN23a can measure. + _ = await GetConnectedClient(); + + LastCreatedTransport.Parameters.GetParams() + .Should().ContainKey("heartbeats") + .WhoseValue.Should().Be("true"); + } + + [Fact] + [Trait("spec", "RTN23b")] + [Trait("spec", "RTC1f1")] + public async Task WithHeartbeatsInTransportParams_ShouldLetTheCallerOverrideIt() + { + // RTN23b makes the param optional, so a caller can still turn it off. Pinned because it + // works by virtue of how AdditionalParameters are merged rather than anything explicit. + _ = await GetConnectedClient(options => options.TransportParams = new Dictionary + { + { "heartbeats", false }, + }); + + LastCreatedTransport.Parameters.GetParams() + .Should().ContainKey("heartbeats") + .WhoseValue.Should().Be("false"); + } + [Fact] [Trait("spec", "RTN2d")] public async Task WithClientId_ShouldSetTransportClientIdCorrectly() diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs new file mode 100644 index 000000000..bb1f6c84e --- /dev/null +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs @@ -0,0 +1,142 @@ +using System; +using FluentAssertions; +using IO.Ably.Realtime.Workflow; +using Xunit; +using Xunit.Abstractions; + +namespace IO.Ably.Tests.Realtime +{ + /// + /// Covers the check that decides whether locally held connection state is too old to resume + /// with - RTN15g and, since it must include the maxIdleInterval, RTN15g2. + /// + [Trait("spec", "RTN15g")] + [Trait("spec", "RTN15g2")] + public class ConnectionStateFreshnessSpecs : AblySpecs + { + private static readonly TimeSpan Ttl = TimeSpan.FromSeconds(120); + private static readonly TimeSpan MaxIdleInterval = TimeSpan.FromSeconds(15); + + private readonly Now _now = new Now(); + private readonly RealtimeState _state = new RealtimeState(); + + [Fact] + public void WhenNothingHasEverBeenReceived_ShouldNotBeStale() + { + // No ConfirmedAliveAt means there is no connection state to consider discarding. + Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: null); + + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); + } + + [Fact] + public void WhenWithinTheTtl_ShouldNotBeStale() + { + Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: _now.Value); + + Advance(TimeSpan.FromSeconds(119)); + + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); + } + + [Fact] + public void WhenPastTheTtlButWithinTheMaxIdleInterval_ShouldNotBeStale() + { + // The point of RTN15g2. At 130s we are past the 120s ttl, but the server may have been + // silent for up to maxIdleInterval before we would have noticed, so the real window is + // 135s and the state is still resumable. + Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: _now.Value); + + Advance(TimeSpan.FromSeconds(130)); + + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); + } + + [Fact] + public void WhenPastTheTtlPlusTheMaxIdleInterval_ShouldBeStale() + { + Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: _now.Value); + + Advance(TimeSpan.FromSeconds(136)); + + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeTrue(); + } + + [Fact] + public void WithNoMaxIdleInterval_ShouldMeasureAgainstTheTtlAlone() + { + Connection(ttl: Ttl, maxIdleInterval: null, aliveAt: _now.Value); + + Advance(TimeSpan.FromSeconds(121)); + + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeTrue(); + } + + [Theory] + [InlineData(0)] + [InlineData(15)] + [InlineData(-1)] + public void WithAnUnrepresentablyLargeTtl_ShouldNotThrow(int maxIdleIntervalSeconds) + { + // Regression. This used to be computed as ConfirmedAliveAt.Add(ttl), which throws + // ArgumentOutOfRangeException once the result runs past DateTimeOffset.MaxValue. The + // exception escaped into the command loop, where it was logged and dropped - silently + // abandoning whichever state transition was in progress. Reachable today from + // ConnectionSandboxOperatingSystemEventsForNetworkSpecs, which injects a MaxValue ttl + // through a Connected message to assert RTN21 override behaviour. + Connection( + ttl: TimeSpan.MaxValue, + maxIdleInterval: TimeSpan.FromSeconds(maxIdleIntervalSeconds), + aliveAt: _now.Value); + + var ex = Record.Exception(() => _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn)); + + ex.Should().BeNull(); + + // An unreachable window can never have elapsed. + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); + } + + [Theory] + [InlineData(-1)] + [InlineData(-120)] + public void WithANegativeMaxIdleInterval_ShouldNotThrow(int maxIdleIntervalSeconds) + { + // Nothing between the wire and here validates the sign - TimeSpanJsonConverter will hand + // back a negative TimeSpan for a negative number - and a negative made the overflow + // guard's own subtraction throw. That exception escapes HandleSetStateCommand and is + // dropped by the command loop, leaving the client wedged in DISCONNECTED with no + // transport: the same failure this method was rewritten to remove. + Connection( + ttl: Ttl, + maxIdleInterval: TimeSpan.FromSeconds(maxIdleIntervalSeconds), + aliveAt: _now.Value); + + Advance(TimeSpan.FromSeconds(130)); + + var ex = Record.Exception(() => _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn)); + ex.Should().BeNull(); + + // Treated as no promise at all, so the window is the ttl alone and 130s is past it. + _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeTrue(); + } + + private void Connection(TimeSpan ttl, TimeSpan? maxIdleInterval, DateTimeOffset? aliveAt) + { + _state.Connection.ConnectionStateTtl = ttl; + _state.Connection.MaxIdleInterval = maxIdleInterval; + + if (aliveAt.HasValue) + { + _state.Connection.SetConfirmedAlive(aliveAt.Value); + } + } + + private void Advance(TimeSpan by) => _now.Reset(_now.Value.Add(by)); + + public ConnectionStateFreshnessSpecs(ITestOutputHelper output) + : base(output) + { + } + } +} diff --git a/src/IO.Ably.Tests.Shared/Realtime/ProtocolMessageTests.cs b/src/IO.Ably.Tests.Shared/Realtime/ProtocolMessageTests.cs index d78469ad5..8c7d5f018 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ProtocolMessageTests.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ProtocolMessageTests.cs @@ -11,6 +11,44 @@ namespace IO.Ably.Tests.Shared.Realtime { public class ProtocolMessageTests { + [Fact] + [Trait("spec", "CD2h")] + public void ConnectionDetails_ShouldDeserialiseMaxIdleIntervalAsMilliseconds() + { + // Pinned against a literal payload rather than a round-trip, which would not catch the + // unit being wrong. 15000 on the wire is 15 seconds. + const string connected = @"{ + ""action"": 4, + ""connectionId"": ""abc"", + ""connectionDetails"": { + ""connectionKey"": ""key"", + ""connectionStateTtl"": 120000, + ""maxIdleInterval"": 15000 + } + }"; + + var message = JsonHelper.Deserialize(connected); + + message.ConnectionDetails.MaxIdleInterval.Should().Be(TimeSpan.FromSeconds(15)); + message.ConnectionDetails.ConnectionStateTtl.Should().Be(TimeSpan.FromSeconds(120)); + } + + [Fact] + [Trait("spec", "CD2h")] + public void ConnectionDetails_WithoutMaxIdleInterval_ShouldLeaveItNull() + { + // Absent is meaningfully different from zero, so it must not deserialise to zero. + const string connected = @"{ + ""action"": 4, + ""connectionId"": ""abc"", + ""connectionDetails"": { ""connectionKey"": ""key"" } + }"; + + var message = JsonHelper.Deserialize(connected); + + message.ConnectionDetails.MaxIdleInterval.Should().BeNull(); + } + [Fact] [Trait("spec", "TR3")] public void ProtocolMessageFlagHaveCorrectValues() From cb37a6b42141cbab1be803435fc682d091234884 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:44:16 +0100 Subject: [PATCH 02/10] Restart the message serial sequence when the connection does not continue RTN15c6, RTN15c7 - fixed. Continuation required an error on the message, so an RTN15g clear - which returns a new connectionId and no error - read as a continuation and the client kept counting while the server restarted at zero. RTN19a1, RTN19a2 - fixed. Renumbering left the stale WaitingForAck entries behind, so the next ACK matched them too and callbacks ran twice; the requeue appended rather than prepended, reversing publish order on the wire. RTN7e - fixed. Only the RTL6c1 queue was failed, not RTL6c2, so a publish made while disconnected got no callback at all. RTN7b - hardened. A failed transport write could leave one message in both queues. RTN16f - a successful recover keeps the counter it adopted. Co-Authored-By: Claude Opus 5 --- .../Realtime/Workflows/RealtimeWorkflow.cs | 123 +++++++++++++----- 1 file changed, 89 insertions(+), 34 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index e3f040057..8a1a0a9b5 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -404,7 +404,20 @@ private async Task ProcessCommandInner(RealtimeCommand command) if (State.Connection.CurrentStateObject.CanSend || cmd.Force) { var sendResult = SendMessage(cmd.ProtocolMessage, cmd.Callback); - if (sendResult.IsFailure && State.Connection.CurrentStateObject.CanQueue && Client.Options.QueueMessages) + + // Never queue a message already awaiting an ACK. One instance in both queues + // would be sent twice on reconnect, and SendMessage's second MsgSerial + // assignment would renumber the copy WaitingForAck reads live, leaving a hole + // in the sequence RTN7b requires to be unique and serially incrementing. + // + // Unreachable today only by coincidence - AckRequired implies CanSend, and + // CanQueue is false in CONNECTED - so the invariant is stated rather than + // left to three unrelated facts. ably-js keeps it deliberately, via + // MessageQueue's sendAttempted flag. + if (sendResult.IsFailure && + cmd.ProtocolMessage.AckRequired == false && + State.Connection.CurrentStateObject.CanQueue && + Client.Options.QueueMessages) { Logger.Debug("Failed to send message. Queuing it."); State.PendingMessages.Add(new MessageAndCallback( @@ -743,10 +756,29 @@ private void HandleConnectedCommand(SetConnectedStateCommand cmd) { var info = new ConnectionInfo(cmd.Message); - // recover is used when set via clientOptions#recover initially, resume will be used for all subsequent requests. - var isConnectionResumeOrRecoverAttempt = State.Connection.Key.IsNotEmpty() || Client.Options.Recover.IsNotEmpty(); - - var failedResumeOrRecover = State.Connection.Id != info.ConnectionId && cmd.Message.Error != null; // RTN15c7, RTN16d + // Whether this Connected continues the message serial sequence we are already part of. + // One that does not must restart at zero per RTN15c7, renumbering anything still + // awaiting an ACK. Three ways to continue: + // + // - an RTN24 update, which arrives on the connection we already hold; + // - a successful resume (RTN15c6), judged on the connectionId alone. RTN15c6's "and no + // error property" describes what Ably sends, while the reset belongs to RTN15c7, + // which is keyed on "a new connectionId". ably-js discriminates the same way, on + // connIdChanged; + // - a successful recover, which deliberately adopts a previous connection's counter. + // RTN16f initialises it from the recovery key, which carries no connectionId, so + // success is judged by the absence of an error. + // + // Broader than testing for an error on the message, which a connection cleared per + // RTN15g would fail: it reconnects fresh, and Ably answers with a new connectionId and + // no error - exactly the case that most needs a new sequence. + // + // Must be evaluated before Update below, which overwrites the id being compared, and + // before Options.Recover is cleared for RTN16k. + var isRecoverAttempt = Client.Options.Recover.IsNotEmpty(); + var connectionContinues = cmd.IsUpdate || + (isRecoverAttempt && cmd.Message.Error == null) || + (State.Connection.Id.IsNotEmpty() && State.Connection.Id == info.ConnectionId); State.Connection.Update(info, cmd.IsUpdate); // RTN16d, RTN15e, RTN23a @@ -765,25 +797,17 @@ private void HandleConnectedCommand(SetConnectedStateCommand cmd) Client.Options.Recover = null; // RTN16k, explicitly setting null so it won't be used for subsequent connection requests - // RTN15c7 - if (isConnectionResumeOrRecoverAttempt && failedResumeOrRecover) + // RTN15c7, RTN15g3, RTN11d - a connection that is not a continuation of the one we held + // restarts the message serial sequence at zero. + if (connectionContinues == false) { State.Connection.MessageSerial = 0; } - // RTN15g3, RTN15c6, RTN15c7, RTN16l - for resume/recovered or when connection ttl passed, re-attach channels - if (State.Connection.HasConnectionStateTtlPassed(Now) || isConnectionResumeOrRecoverAttempt) - { - foreach (var channel in Channels) - { - if (channel.State == ChannelState.Attaching || channel.State == ChannelState.Attached || channel.State == ChannelState.Suspended) - { - ((RealtimeChannel)channel).Attach(null, null, null, true); // state changes as per RTL2g - } - } - } - - SendPendingMessagesOnConnected(failedResumeOrRecover); // RTN19a + // The RTL3d reattach lives in RealtimeChannel.ConnectionStateChanged, not here: that + // handler runs inside NotifyUpdate's internal handlers, so the channel transitions land + // before CONNECTED reaches external listeners, as RTL3d1 requires. + SendPendingMessagesOnConnected(connectionContinues); // RTN19a } private void HandlePingTimer(PingTimerCommand cmd) @@ -1121,26 +1145,42 @@ private void UpdateStateAndNotifyConnection(ConnectionStateBase newState) } } - private void SendPendingMessagesOnConnected(bool failedResumeOrRecover) + private void SendPendingMessagesOnConnected(bool connectionContinues) { - // RTN19a1 - if (failedResumeOrRecover) + if (connectionContinues) { - foreach (var messageAndCallback in State.WaitingForAck) + // RTN19a2 - the same connection is still expecting the serials these messages were + // originally given, so resend them unchanged and leave them awaiting their ACK. + foreach (var message in State.WaitingForAck.Select(x => x.Message)) { - State.PendingMessages.Add(new MessageAndCallback( - messageAndCallback.Message, - messageAndCallback.Callback, - messageAndCallback.Logger)); + ConnectionManager.SendToTransport(message); } } else { - // RTN19a2 - successful resume, msgSerial doesn't change - foreach (var message in State.WaitingForAck.Select(x => x.Message)) - { - ConnectionManager.SendToTransport(message); - } + // RTN19a1, RTN19a2 - a different connection means a fresh serial sequence, so + // requeue rather than resend. The loop below hands each message to SendMessage, + // which assigns a serial from the counter that HandleConnectedCommand has just + // reset and re-registers it for its ACK. + // + // Resending these unchanged would leave the server's sequence sitting at the old + // high water mark while ours restarted at zero, and Ably silently discards a + // message whose serial is below what it has already seen - no ACK, no NACK, so the + // publish callback would never be called at all. + // + // WaitingForAck is cleared because SendMessage re-registers each message as it + // goes; stale entries would hold serials of the old sequence that the next ACK also + // matches, running their callbacks twice. + // + // Inserted at the front, not appended: PendingMessages is the RTL6c2 queue and + // already holds anything published while disconnected, which happened *after* these. + // Appending would give the newer messages the lower serials and reverse publish + // order. ably-js prepends for the same reason. + State.PendingMessages.InsertRange( + 0, + State.WaitingForAck.Select(x => new MessageAndCallback(x.Message, x.Callback, x.Logger))); + + State.WaitingForAck.Clear(); } if (Logger.IsDebug && State.PendingMessages.Count > 0) @@ -1160,15 +1200,30 @@ private void SendPendingMessagesOnConnected(bool failedResumeOrRecover) State.PendingMessages.Clear(); } + /// + /// RTN7e - when the connection enters SUSPENDED, CLOSED or FAILED, everything that has not + /// been acknowledged has failed and must be reported as such. + /// private void ClearAckQueueAndFailMessages(ErrorInfo error) { + var messageError = error ?? ErrorInfo.ReasonUnknown; + foreach (var item in State.WaitingForAck.Where(x => x.Callback != null)) { - var messageError = error ?? ErrorInfo.ReasonUnknown; item.SafeExecute(false, messageError); } State.WaitingForAck.Clear(); + + // RTN7e covers RTL6c2 as well as RTL6c1: a message submitted via either "should be + // considered failed ... and removed from any RTN19a retry queue". PendingMessages is the + // RTL6c2 queue. + foreach (var item in State.PendingMessages.Where(x => x.Callback != null)) + { + item.SafeExecute(false, messageError); + } + + State.PendingMessages.Clear(); } public void QueueAck(ProtocolMessage message, Action callback) From 12616d6527f222b4b8cd29fa3204db80542c7bad Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:44:39 +0100 Subject: [PATCH 03/10] Reattach channels on entering CONNECTED, and detach them when the connection goes away RTL3d - fixed. The reattach was gated on connectionId having changed, which RTN15g empties before CONNECTING, so the channel stayed locally ATTACHED on a new connection with no server-side attachment - permanently silent, no error. RTL3d1 - fixed. The reattach ran after external listeners had seen CONNECTED. RTN24 - fixed. An update churned channels: four spurious protocol messages per reauth, plus UPDATE events RTL2g does not permit. RTN11b, RTL3b, RTP5a - fixed. Channels never passed through DETACHED on a close, so presence members from the abandoned connection survived into the next one. RTL3c - fixed. A DETACHING channel was left stranded when the connection suspended. RTL5l - fixed. Enumerating the non-connected states let DISCONNECTED through, where the DETACH was queued for the next connection and the callback never fired. RTL11 - fixed. A null reason faulted the task with a bare Exception. RTL15b2 - fixed. channelSerial was cleared on SUSPENDED. Co-Authored-By: Claude Opus 5 --- src/IO.Ably.Shared/Realtime/Presence.cs | 13 +- .../Realtime/RealtimeChannel.cs | 164 +++++++++++++----- .../Realtime/RealtimeChannels.cs | 18 +- .../Realtime/ChannelSpecs.cs | 65 ++++++- 4 files changed, 206 insertions(+), 54 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Presence.cs b/src/IO.Ably.Shared/Realtime/Presence.cs index 42358fd7c..fab293ca3 100644 --- a/src/IO.Ably.Shared/Realtime/Presence.cs +++ b/src/IO.Ably.Shared/Realtime/Presence.cs @@ -792,11 +792,22 @@ private void SendQueuedMessages() private void FailQueuedMessages(ErrorInfo reason) { + // RTL11 wants "an ErrorInfo indicating the failure". TaskWrapper has no branch for + // (false, null) and faults the task with a bare Exception, so the reason is coalesced + // here rather than at each call site. + var error = reason ?? ErrorInfo.ReasonUnknown; + while (!PendingPresenceQueue.IsEmpty) { if (PendingPresenceQueue.TryDequeue(out var queuedPresenceMessage)) { - queuedPresenceMessage.Callback?.Invoke(false, reason); + // Guarded like every other callback site: a throwing application callback would + // otherwise abort the rest of the queue and skip the RTP5a map clearing in + // ChannelDetachedOrFailed, which RealtimeChannels then swallows per channel. + ActionUtils.SafeExecute( + () => queuedPresenceMessage.Callback?.Invoke(false, error), + Logger, + nameof(FailQueuedMessages)); } } } diff --git a/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs b/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs index c9159ab2b..a2b717bab 100644 --- a/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs +++ b/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs @@ -58,8 +58,6 @@ internal bool DecodeRecovery internal AblyRealtime RealtimeClient { get; } - private string PreviousConnectionId { get; set; } - private ConnectionState ConnectionState => Connection.State; private IConnectionManager ConnectionManager => RealtimeClient.ConnectionManager; @@ -147,68 +145,103 @@ internal RealtimeChannel( internal void ConnectionStateChanged(ConnectionStateChange connectionStateChange) { - var connectionRefreshed = PreviousConnectionId != Connection.Id; - if (connectionRefreshed) - { - PreviousConnectionId = Connection.Id; - } - switch (connectionStateChange.Current) { case ConnectionState.Connected: - if (State == ChannelState.Suspended) - { - Attach(); - } - if (State == ChannelState.Attaching && AttachedAwaiter.Waiting == false) + /* + * (RTN24) An update arrives on the connection we already hold, so it must not + * touch channel state at all - RTL3d governs *entering* CONNECTED. ably-js + * touches no channel on a mid-connection CONNECTED either. + */ + if (connectionStateChange.Event == ConnectionEvent.Update) { - Attach(null, emitUpdate: true); + break; } /* - * Connection state is only maintained server-side for a brief period, - * given by the connectionStateTtl in the connectionDetails (2 minutes at time of writing, see CD2f). - * If a client has been disconnected for longer than the connectionStateTtl - * it should clear the local connection state and any connection attempts should be made as for a fresh connection + * (RTL3d, RTN19b, RTN15c6, RTN15c7, RTN15g3) On entering CONNECTED, every + * channel that was attached or pending needs its ATTACH - or its DETACH - sent + * again, because the previous transport will never answer. + * + * The two branches have different owners. RTL3d names ATTACHING, ATTACHED and + * SUSPENDED and asks for an RTL4c attach. DETACHING is not in that list and is + * not a state transition at all - it is RTN19b, "if there are any pending + * channels i.e. in the ATTACHING or DETACHING state, the respective ATTACH or + * DETACH message should be resent". ably-js splits it the same way, with + * checkPendingState handling both pending operations and notifyState the + * reattach. * - * (RTN15g3) When a connection attempt succeeds after the connection state has been cleared in this way, - * channels that were previously ATTACHED, ATTACHING, or SUSPENDED must be automatically reattached, - * just as if the connection was a resume attempt which failed per RTN15c3 + * RTL3d1 requires all of this to be applied before CONNECTED reaches external + * listeners, and is why it lives here rather than in HandleConnectedCommand: + * Connection.NotifyUpdate runs the internal handlers, which is what calls this, + * before handing the emit to NotifyExternalClients. * - * Given the above, if the channel is ATTACHED and the connection is fresh - * then set the channel to ATTACHING to trigger an ATTACH attempt + * Unconditional, deliberately. This used to be gated on the connectionId having + * changed, which silently skipped the RTN15g3 reattach: RTN15g empties + * Connection.Id *before* the CONNECTING transition, so by the time CONNECTED + * arrived the id being compared against had already gone and the channel + * concluded nothing had changed. The channel stayed locally ATTACHED on a brand + * new connection with no server-side attachment - permanently silent. + * + * Whether the connection was resumed belongs inside the ATTACH, in channelSerial + * and the ATTACH_RESUME flag, rather than in a decision about whether to send + * one at all. ably-js reattaches unconditionally for the same reason. */ - if (State == ChannelState.Attached && connectionRefreshed) - { - Attach(null, force: true, emitUpdate: false); - } - - if (State == ChannelState.Detaching && DetachedAwaiter.Waiting == false) + switch (State) { - Detach(null, force: true, emitUpdate: true); + case ChannelState.Suspended: + case ChannelState.Attaching: + case ChannelState.Attached: + Attach(null, force: true, emitUpdate: false); + break; + case ChannelState.Detaching: + Detach(null, force: true, emitUpdate: true); + break; } break; case ConnectionState.Disconnected: AttachedAwaiter.Fail(new ErrorInfo("Connection is Disconnected")); DetachedAwaiter.Fail(new ErrorInfo("Connection is Disconnected")); + break; + case ConnectionState.Closing: + + // RTN11b - on connect() while CLOSING, channels "first transition to DETACHED, + // following RTL3b, and then reinitialize ... per RTN11d". Done at the connection + // transition rather than inside RTN11d so a close() with no connect() behind it + // is covered too. See DetachForConnectionGoingAway for the states involved. + DetachForConnectionGoingAway( + new ErrorInfo("Connection is closing", ErrorCodes.ChannelOperationFailed)); + break; case ConnectionState.Closed: - AttachedAwaiter.Fail(new ErrorInfo("Connection is closed")); - DetachedAwaiter.Fail(new ErrorInfo("Connection is closed")); - if (State == ChannelState.Attached || State == ChannelState.Attaching) - { - Detach(); - } + + // The same four states as CLOSING, for the same RTP5a reason. CLOSED is + // reachable without CLOSING: ConnectionSuspendedState.Close() queues + // SetClosedStateCommand directly. + DetachForConnectionGoingAway( + new ErrorInfo("Connection is closed", ErrorCodes.ChannelOperationFailed)); break; case ConnectionState.Suspended: AttachedAwaiter.Fail(new ErrorInfo("Connection is suspended")); DetachedAwaiter.Fail(new ErrorInfo("Connection is suspended")); - if (State == ChannelState.Attached || State == ChannelState.Attaching) + + // RTL3c names only ATTACHING and ATTACHED; DETACHING is ably-js parity rather + // than a spec requirement - propogateConnectionInterruption maps suspended over + // ['attaching','attached','detaching','suspended']. It matters because the lines + // above have just failed the awaiter that would have finished a detach, leaving + // the channel in DETACHING with no DETACHED coming and no callback. + // + // The connection's own reason is carried rather than a constant, as ably-js does + // by passing change.reason into notifyState. + if (State == ChannelState.Attached || State == ChannelState.Attaching || + State == ChannelState.Detaching) { - SetChannelState(ChannelState.Suspended, ErrorInfo.ReasonSuspended); + SetChannelState( + ChannelState.Suspended, + connectionStateChange.Reason ?? ErrorInfo.ReasonSuspended); } break; @@ -223,6 +256,39 @@ internal void ConnectionStateChanged(ConnectionStateChange connectionStateChange } } + /// + /// RTN11b/RTL3b - the connection is going away, so a channel that is attached or pending has + /// to detach. RTP5a clears the presence maps on entering DETACHED, which is why all four + /// pending states are covered and not just the two RTL3b names: a channel left SUSPENDED or + /// DETACHING carries its members from the abandoned connection into the next one. + /// + /// reported to anyone waiting on an attach or detach that this + /// transition abandons. Deliberately not carried into the channel state: RTL24 lists RTN11d, + /// RTL3a, RTL4g and RTL14 as the sources of RealtimeChannel#errorReason, and a connection + /// close is none of them. + private void DetachForConnectionGoingAway(ErrorInfo awaiterError) + { + AttachedAwaiter.Fail(awaiterError); + + if (State == ChannelState.Attached || State == ChannelState.Attaching || + State == ChannelState.Suspended) + { + DetachedAwaiter.Fail(awaiterError); + Detach(null, force: false, emitUpdate: false); + } + else if (State == ChannelState.Detaching) + { + // DetachedAwaiter is deliberately left alone: the detach the caller asked for is + // about to complete, so the DETACHED transition below finishes it as a success. + // ably-js resolves detach() here for the same reason. + SetChannelState(ChannelState.Detached); + } + else + { + DetachedAwaiter.Fail(awaiterError); + } + } + public void Attach(Action callback = null) { Attach(null, null, callback); @@ -396,18 +462,17 @@ private void Detach(Action callback, bool force, bool emitUpdat { SetChannelState(ChannelState.Detaching, emitUpdate); - if (ConnectionState == ConnectionState.Closed || ConnectionState == ConnectionState.Connecting || - ConnectionState == ConnectionState.Suspended) + // RTL5l - "if the connection state is anything other than CONNECTED and none of the + // preceding channel state conditions apply, the channel transitions immediately to + // the DETACHED state". Tested as anything-but-CONNECTED rather than enumerated, so + // DISCONNECTED cannot fall through to the RTL6c2 queue. + if (ConnectionState != ConnectionState.Connected) { - SetChannelState(ChannelState.Detached); - } - else if (ConnectionState != ConnectionState.Failed) - { - SendMessage(new ProtocolMessage(ProtocolMessage.MessageAction.Detach, Name)); + SetChannelState(ChannelState.Detached); } else { - Logger.Warning($"#{Name}. Cannot send Detach messages when connection is in Failed State"); + SendMessage(new ProtocolMessage(ProtocolMessage.MessageAction.Detach, Name)); } } else @@ -664,8 +729,11 @@ private void HandleStateChange(ChannelState state, ErrorInfo error, ProtocolMess Logger.Debug($"HandleStateChange state change from {State} to {state}"); } - // RTP5a1 - if (state == ChannelState.Detached || state == ChannelState.Suspended || state == ChannelState.Failed) + // RTL15b2 - "If the channel enters the DETACHED or FAILED state, it must clear its + // channelSerial. (Unlike previous spec versions, it must not clear it when entering the + // SUSPENDED state)." Retaining it in SUSPENDED lets a channel suspended by an RTL4f + // attach timeout carry its serial on the reattach RTL4c1 asks for. + if (state == ChannelState.Detached || state == ChannelState.Failed) { Properties.ChannelSerial = null; } diff --git a/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs b/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs index eb7601a99..e3ca4cc85 100644 --- a/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs +++ b/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs @@ -237,12 +237,24 @@ private void HandleInitialiseFailedChannelsCommand(RealtimeChannel channel) { switch (_realtimeClient.Connection.State) { + case ConnectionState.Closing: case ConnectionState.Closed: case ConnectionState.Failed: - /* (RTN11d) - * If the [Connection] state is FAILED, - * transitions all the channels to INITIALIZED */ + /* (RTN11d) From CLOSED or FAILED, every channel goes to INITIALIZED with its + * errorReason unset (RTL24). CLOSING is included because RTN11b routes connect() + * in that state through RTN11d. + * + * Passing no error is what unsets it: SetChannelState hands it to OnError, which + * assigns either way, and does so before the same-state early return. + * + * The connection half of RTN11d - Connection.errorReason and msgSerial - is done + * once in the workflow's ConnectCommand handler. */ channel.SetChannelState(ChannelState.Initialized); + + // RTN11d's "clear all internal connection data". RTL15b2 only nulls the serial + // for Detached and Failed, and on a close an ATTACHED channel is left in + // DETACHING with no DETACHED coming, so it needs clearing here. + channel.Properties.ChannelSerial = null; break; } } diff --git a/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs index 863ba6628..17ecbc7c7 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs @@ -388,6 +388,65 @@ public async Task WhenChannelIsSuspended_WhenConnectionBecomesConnectedAttemptAt completed.Should().BeTrue("channel should have become Suspended again"); } + [Fact] + [Trait("spec", "RTL3d1")] + [Trait("spec", "RTL3d")] + public async Task WhenConnectedIsEmittedExternally_TheRTL3dTransitionsShouldAlreadyBeApplied() + { + // RTL3d1: "The RTL3d channel state transitions must be applied before the CONNECTED + // connection state change is emitted to external listeners." Nothing implements it + // directly - it holds because Connection.NotifyUpdate runs the internal handlers, + // where RealtimeChannels fans the change out to every channel, before the emit. + // + // An ATTACHED channel on a successful resume is the case that needs it, and the one + // a conditional reattach would miss. With no SynchronizationContext - the default, + // and what every server-side host has - NotifyExternalClients invokes inline, so + // anything done after SetState is visible to the application too late. + var client = await GetConnectedClient(opts => + opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + + var channel = (RealtimeChannel)client.Channels.Get("test".AddRandomSuffix()); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.WaitForState(ConnectionState.Disconnected); + await client.ProcessCommands(); + + ChannelState? seenByExternalListener = null; + client.Connection.On(ConnectionEvent.Connected, _ => seenByExternalListener = channel.State); + + // The same connectionId the client already holds, so this is an RTN15c6 resume. + client.Workflow.QueueCommand(SetConnectedStateCommand.Create(ConnectedProtocolMessage, false)); + await client.WaitForState(ConnectionState.Connected); + await client.ProcessCommands(); + + seenByExternalListener.Should().Be(ChannelState.Attaching); + } + + [Fact] + [Trait("spec", "RTL3c")] + public async Task WhenConnectionIsSuspended_ADetachingChannelShouldNotBeStranded() + { + // RTL3c names only ATTACHING and ATTACHED, so DETACHING is ably-js parity rather than + // a spec requirement - propogateConnectionInterruption maps suspended over + // ['attaching','attached','detaching','suspended']. It matters because the handler + // fails the DetachedAwaiter first, leaving the channel with no DETACHED coming. + var (client, channel) = await GetClientAndChannel(); + + ((RealtimeChannel)channel).SetChannelState(ChannelState.Detaching); + await client.ProcessCommands(); + + client.Workflow.QueueCommand(SetSuspendedStateCommand.Create(new ErrorInfo("why it suspended", 12345))); + await client.WaitForState(ConnectionState.Suspended); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Suspended); + + // And the connection's own reason, not a constant - ably-js passes change.reason. + channel.ErrorReason.Message.Should().Be("why it suspended"); + } + [Theory] [InlineData(ChannelState.Attached)] [InlineData(ChannelState.Attaching)] @@ -399,10 +458,12 @@ public async Task WhenConnectionIsSuspended_AttachingOrAttachedChannelsShouldTra ((RealtimeChannel)channel).SetChannelState(state); - client.Close(); - + // Driven straight to SUSPENDED, with no Close() first: CLOSING detaches channels per + // RTN11b/RTL3b, so closing would race the suspend for the outcome, and RTL3c is + // about entering SUSPENDED anyway. client.Workflow.QueueCommand(SetSuspendedStateCommand.Create(null)); await client.WaitForState(ConnectionState.Suspended); + await client.ProcessCommands(); // Assert channel.State.Should().Be(ChannelState.Suspended); From b28751ac65f79af8fccf2097305b7f0386b3d3fd Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:45:01 +0100 Subject: [PATCH 04/10] Report the retry delay we actually wait, and suspend on every disconnect path RTN14d - fixed. retryIn reported the nominal disconnectedRetryTimeout while the timer was started with the RTB1 delay, and reported a wait at all on the skipAttach path where there is none. RTB1a - fixed. The attempt count read `?? 0 + 1`, which C# parses as `?? (0 + 1)`, so a non-null collection was never incremented. RTB1 - fixed. The connectionStateTtl clamp subtracted unguarded operands and overflowed on a backward clock step. RTN14e - fixed. Only the two connection-attempt handlers checked the deadline, and the token and auth retry paths pass through neither, so a client whose token source kept failing never suspended. Co-Authored-By: Claude Opus 5 --- .../Realtime/Workflows/RealtimeWorkflow.cs | 55 +++++++++------ .../Connection/ConnectionDisconnectedState.cs | 68 +++++++++++++++++-- .../States/Connection/ConnectionStateBase.cs | 2 +- .../Realtime/ConnectionSandBoxSpecs.cs | 21 ++++-- .../ConnectionSpecs/ConnectionFailureSpecs.cs | 20 ++++-- .../DisconnectedStateSpecs.cs | 41 +++++++++++ 6 files changed, 170 insertions(+), 37 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index 8a1a0a9b5..c8cb3dc70 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -493,20 +493,12 @@ async Task AttemptANewConnection() } case HandleConnectingDisconnectedCommand cmd: - if (State.ShouldSuspend(Now)) - { - return SetSuspendedStateCommand.Create( - cmd.Error ?? ErrorInfo.ReasonSuspended, - clearConnectionKey: true) - .TriggeredBy(cmd); - } - else - { - return SetDisconnectedStateCommand.Create( - cmd.Error ?? ErrorInfo.ReasonDisconnected, - clearConnectionKey: true) - .TriggeredBy(cmd); - } + + // Suspending is decided in the SetDisconnectedStateCommand handler, for every path. + return SetDisconnectedStateCommand.Create( + cmd.Error ?? ErrorInfo.ReasonDisconnected, + clearConnectionKey: true) + .TriggeredBy(cmd); case HandleConnectingErrorCommand cmd: var error = cmd.Error ?? cmd.Exception?.ErrorInfo ?? ErrorInfo.ReasonUnknown; @@ -519,14 +511,6 @@ async Task AttemptANewConnection() if (error.IsRetryableStatusCode()) { - if (State.ShouldSuspend(Now)) - { - return SetSuspendedStateCommand.Create( - error, - clearConnectionKey: true) - .TriggeredBy(cmd); - } - return SetDisconnectedStateCommand.Create( error, clearConnectionKey: true) @@ -965,11 +949,30 @@ ErrorInfo TransformIfTokenErrorAndNotRetryable() break; case SetDisconnectedStateCommand cmd: + // RTN14e - measured here because this is the one place every path into + // DISCONNECTED converges. Checking only the two connection-attempt failure + // handlers misses the token and auth retry paths, which do not pass through + // either: a client whose token source keeps failing would loop CONNECTING and + // DISCONNECTED indefinitely without ever suspending. + // + // Ordered before CheckInstantRetryFlag so that suspending beats retrying. + // + // SkipAttach is excluded: the caller has already queued the next command, so + // diverting would emit SUSPENDED and then immediately CONNECTING. + if (cmd.SkipAttach == false && State.ShouldSuspend(Now)) + { + return SetSuspendedStateCommand.Create( + cmd.Error ?? ErrorInfo.ReasonSuspended, + clearConnectionKey: true) + .TriggeredBy(command); + } + if (cmd.ClearConnectionKey) { State.Connection.ClearKey(); } + bool? connectivityAnswer = null; var retryInstantly = await CheckInstantRetryFlag(); var disconnectedState = new ConnectionDisconnectedState(ConnectionManager, cmd.Error, Logger) @@ -978,6 +981,14 @@ ErrorInfo TransformIfTokenErrorAndNotRetryable() Exception = cmd.Exception, }; + if (cmd.SkipAttach) + { + // RTN14d - retryIn must be the delay actually waited. skipAttach means + // the caller has already queued the next command, so there is no wait, + // and StartTimer, which records the real figure, never runs. + disconnectedState.RetryIn = TimeSpan.Zero; + } + SetState(disconnectedState, skipTimer: cmd.SkipAttach); // RTN7d diff --git a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionDisconnectedState.cs b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionDisconnectedState.cs index a8d4bd624..e5aae50dc 100644 --- a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionDisconnectedState.cs +++ b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionDisconnectedState.cs @@ -49,14 +49,74 @@ public override void AbortTimer() // RTN14d public override void StartTimer() { + if (RetryInstantly) + { + // RTN14d - there is no wait, so say so rather than advertising the nominal timeout. + RetryIn = TimeSpan.Zero; + return; + } + + var state = Context.Connection.RealtimeClient?.State; var retryInterval = Context.RetryTimeout.TotalMilliseconds; - var noOfAttempts = Context.Connection.RealtimeClient?.State?.AttemptsInfo?.NumberOfAttempts ?? 0 + 1; // First attempt should start with 1 instead of 0. - var retryIn = TimeSpan.FromMilliseconds(ReconnectionStrategy.GetRetryTime(retryInterval, noOfAttempts)); - if (RetryInstantly == false) + // RTB1a's coefficient sequence is indexed from the first attempt, so a count of zero + // would apply 2/3 where the first retry should get 1. On a drop straight out of + // CONNECTED there is no recorded attempt, because entering CONNECTED cleared them. + var noOfAttempts = Math.Max(state?.AttemptsInfo?.NumberOfAttempts ?? 0, 1); + + var retryIn = ClampToStateTtl( + TimeSpan.FromMilliseconds(ReconnectionStrategy.GetRetryTime(retryInterval, noOfAttempts)), + state); + + // RTN14d - retryIn must be "the time in milliseconds until the next connection attempt", + // so report the delay we are about to wait rather than the nominal + // disconnectedRetryTimeout, which ignores the RTB1 coefficient, its jitter and the clamp + // above. SetState calls this before emitting, so this is what the application sees. + RetryIn = retryIn; + + _timer.Start(retryIn, OnTimeOut); + } + + /// + /// RTN14e requires the move to SUSPENDED once the connection state ttl has elapsed, and that + /// decision is taken when an attempt fails. Sleeping past the deadline would delay SUSPENDED + /// by however long we slept, unbounded because disconnectedRetryTimeout is a client option. + /// Waking at the deadline lets the attempt fail there and be converted, keeping one timer. + /// + private TimeSpan ClampToStateTtl(TimeSpan retryIn, RealtimeState state) + { + var firstAttempt = state?.AttemptsInfo?.FirstAttempt; + if (firstAttempt.HasValue == false) { - _timer.Start(retryIn, OnTimeOut); + return retryIn; } + + var elapsed = Context.Connection.Now() - firstAttempt.Value; + + // Both operands clamped before subtracting: a backwards clock step makes elapsed + // negative, and a ttl near TimeSpan.MaxValue then overflows. SetState calls StartTimer + // inside a catch that only handles AblyException, so the throw would be dropped and the + // whole transition abandoned - no DISCONNECTED, no transport, no timer. + if (elapsed < TimeSpan.Zero) + { + elapsed = TimeSpan.Zero; + } + + if (state.Connection.ConnectionStateTtl >= TimeSpan.MaxValue - elapsed) + { + return retryIn; + } + + var remaining = state.Connection.ConnectionStateTtl - elapsed; + + if (remaining <= TimeSpan.Zero || remaining >= retryIn) + { + // The deadline has passed, so the next attempt converts to SUSPENDED whenever it + // happens, or it is further out than the backoff and there is nothing to clamp. + return retryIn; + } + + return remaining; } private void OnTimeOut() diff --git a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionStateBase.cs b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionStateBase.cs index 5c1b0a612..011221f28 100644 --- a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionStateBase.cs +++ b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionStateBase.cs @@ -26,7 +26,7 @@ protected ConnectionStateBase(IConnectionContext context, ILogger logger) public Exception Exception { get; set; } - public TimeSpan? RetryIn { get; protected set; } + public TimeSpan? RetryIn { get; internal set; } public virtual bool CanQueue => false; diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs index e7e303bca..cbbf8f4c4 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -962,7 +962,7 @@ public async Task WhenDisconnectedMessageContainsTokenError_IfTokenRenewFails_Sh [ProtocolData] [Trait("spec", "RTN15g")] [Trait("spec", "RTN15g1")] - // "RTN15g2" It can't implement that spec item because RTN23a is not even implemented + [Trait("spec", "RTN15g2")] [Trait("spec", "RTN15g3")] public async Task WhenDisconnectedPastTTL_ShouldNotResume_ShouldClearConnectionStateAndAttemptNewConnection(Protocol protocol) { @@ -974,9 +974,14 @@ public async Task WhenDisconnectedPastTTL_ShouldNotResume_ShouldClearConnectionS await client.WaitForState(ConnectionState.Connected); - client.State.Connection.ConnectionStateTtl = TimeSpan.FromSeconds(1); + // No ttl override: winding ConfirmedAliveAt back below is enough to force the stale + // path against the real 120s ttl plus the server's 15s maxIdleInterval. Shortening the + // ttl as well made this test fail, because the RTN14e clamp in + // ConnectionDisconnectedState.StartTimer then clamps the retry backoff down to the + // remaining ttl - about a second - so the reconnect happened far sooner than the + // DisconnectedRetryTimeout this test asserts against. A 1s ttl would also trip + // ShouldSuspend on the retry and land in SUSPENDED instead of CONNECTED. string initialConnectionId = client.Connection.Id; - TimeSpan connectionStateTtl = client.Connection.ConnectionStateTtl; var aliveAt1 = client.Connection.ConfirmedAliveAt; var aliveAt2 = aliveAt1; @@ -1006,6 +1011,15 @@ await WaitFor(60000, done => client.Connection.Once(ConnectionEvent.Disconnected, change2 => { disconnectedAt = DateTime.UtcNow; + + // RTN15g2 - the staleness window is connectionStateTtl plus the real + // maxIdleInterval the server sent, which is 15s against a live endpoint. A + // shortened ttl alone is therefore not enough to push us outside it before the + // retry fires, so wind the last known activity back instead. Deliberately done + // here rather than while still Connected: from Connected this would trip the + // RTN23a idle monitor and race the disconnect this test is arranging. + client.State.Connection.SetConfirmedAlive(DateTimeOffset.UtcNow.AddMinutes(-30)); + channels[1].Attach(); client.Connection.Once(ConnectionEvent.Connecting, change3 => { @@ -1030,7 +1044,6 @@ await WaitFor(60000, done => initialConnectionId.Should().NotBeNullOrEmpty(); initialConnectionId.Should().NotBe(newConnectionId); - connectionStateTtl.Should().Be(TimeSpan.FromSeconds(1)); aliveAt1.Value.Should().BeBefore(aliveAt2.Value); await channels[0].WaitForAttachedState(); diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailureSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailureSpecs.cs index 32794695d..18c74b4fe 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailureSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailureSpecs.cs @@ -183,13 +183,10 @@ public async Task WhenTransportFails_ShouldTransitionToDisconnectedAndEmitErrorW await client.WaitForState(ConnectionState.Connecting); - client.Connection.On((args) => + ConnectionStateChange firstChange = null; + client.Connection.Once(ConnectionEvent.Disconnected, args => { - args.Current.Should().Be(ConnectionState.Disconnected); - args.Previous.Should().Be(ConnectionState.Connecting); - args.Event.Should().Be(ConnectionEvent.Disconnected); - args.RetryIn.Should().Be(options.DisconnectedRetryTimeout); - args.Reason.Should().NotBeNull(); + firstChange = args; Done(); }); @@ -199,6 +196,17 @@ public async Task WhenTransportFails_ShouldTransitionToDisconnectedAndEmitErrorW LastCreatedTransport.Listener.OnTransportEvent(LastCreatedTransport.Id, TransportState.Closing, new Exception()); WaitOne(); + + firstChange.Should().NotBeNull(); + firstChange.Previous.Should().Be(ConnectionState.Connecting); + firstChange.Event.Should().Be(ConnectionEvent.Disconnected); + firstChange.Reason.Should().NotBeNull(); + + // RTN15a - a transport disconnected unexpectedly is treated as a non-token DISCONNECTED + // per RTN15h3, which reconnects immediately. So RTN14d's "time until the next connection + // attempt" is zero here, not the disconnectedRetryTimeout. The backoff reporting itself + // is covered deterministically by DisconnectedStateSpecs. + firstChange.RetryIn.Should().Be(TimeSpan.Zero); } [Fact(Skip = "Requires a SandBox Spec")] diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs index 5bd5e8ae9..274381ff1 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs @@ -1,3 +1,4 @@ +using System; using System.Threading.Tasks; using FluentAssertions; using IO.Ably.Realtime; @@ -105,6 +106,46 @@ public async Task AfterAnInterval_ShouldRetryConnection() _context.ShouldQueueCommand(); } + [Fact] + [Trait("spec", "RTN14d")] + public void StartTimer_ShouldReportTheDelayItActuallyWaits() + { + // RTN14d - retryIn is "the time in milliseconds until the next connection attempt", so + // the value handed to the application is the one the timer was started with, RTB1 backoff + // and jitter included. + _state.StartTimer(); + + _timer.LastDelay.Should().BeGreaterThan(TimeSpan.Zero); + _state.RetryIn.Should().Be(_timer.LastDelay); + } + + // UTS: realtime/unit/RTB1/disconnected-retry-delay-0 + [Fact] + [Trait("spec", "RTN14d")] + [Trait("spec", "RTB1")] + public void StartTimer_ShouldApplyTheBackoffAndJitter() + { + _state.StartTimer(); + + // RTB1a's coefficient is 1 for the first retry and RTB1b's jitter is 0.8 to 1.0. + var nominal = _context.RetryTimeout; + _timer.LastDelay.Should().BeGreaterOrEqualTo(TimeSpan.FromMilliseconds(nominal.TotalMilliseconds * 0.8)); + _timer.LastDelay.Should().BeLessOrEqualTo(nominal); + } + + [Fact] + [Trait("spec", "RTN14d")] + public void WhenRetryingInstantly_ShouldReportNoWaitAndNotStartTheTimer() + { + var state = GetState(); + state.RetryInstantly = true; + + state.StartTimer(); + + state.RetryIn.Should().Be(TimeSpan.Zero); + _timer.StartedWithAction.Should().BeFalse(); + } + private ConnectionDisconnectedState GetState(ErrorInfo error = null) { return new ConnectionDisconnectedState(_context, error, _timer, Logger); From 627153f2b568d6928228ab18b73a8138e67fdcc5 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:45:24 +0100 Subject: [PATCH 05/10] Consult the fallback hosts on every attempt, and bound the immediate retry RTN17j - fixed. The immediate retry was unbounded: each failed attempt produced another qualifying DISCONNECTED, so RTB1 was never reached and retryIn told the application it would wait while the client retried as fast as the connectivity check allowed. Now bounded by the number of domains there are to traverse. RTN17, RTN17i - fixed. Host selection skipped GetHost for timer-driven retries, so once the retry budget was spent every attempt was pinned to the primary and a client whose primary datacenter was down could never reach a fallback. RTN17j - the connectivity answer is carried on the CONNECTING command, so a cycle takes one check rather than two serialised on the reader thread. RTN15a, RTN15h3 - fixed. The immediate reconnect recognised only an exception or a 500-504 status, which a plain DISCONNECTED carries neither of. Token errors are excluded, since RTN15h2 owns those and already reconnects. RTN14e, RTN17 - fixed. A transport dropping out of CONNECTED recorded no failed attempt: entering CONNECTED clears the attempt collection, and the exception path discarded the failure whenever it was empty. FirstAttempt stayed null so the suspend clock started late, and DisconnectedCount stayed at zero, which feeds fallback host selection. Divergences, all deliberate: - The host list is not swept within a single attempt, as ably-js does. RTN17i's first sentence favours it. - The immediate retry is bounded by domain count and then hands over to RTB1. ably-js instead rate-limits it to one per second and never stops granting it. - The immediate retry stays gated on the connectivity check, as it was before this change. RTN15h3 mandates it unconditionally and RTN17j scopes the check to fallback use, so with no internet we wait out RTB1 where RTN15h3 says reconnect. Pre-existing, widened here to the RTN15h3 case. ably-js does not gate on it. Co-Authored-By: Claude Opus 5 --- .../Realtime/Workflows/RealtimeCommands.cs | 15 ++- .../Realtime/Workflows/RealtimeWorkflow.cs | 85 ++++++++++++-- .../Transport/ConnectionAttemptsInfo.cs | 25 +++- .../Connection/ConnectionConnectedState.cs | 3 + .../ConnectionFailuresOnceConnectedSpecs.cs | 69 +++++++++++ .../ConnectionFallbackSpecs.cs | 111 ++++++++++++++++++ 6 files changed, 295 insertions(+), 13 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs index 2fd70e820..dc144265e 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs @@ -154,21 +154,30 @@ private CloseConnectionCommand() internal class SetConnectingStateCommand : RealtimeCommand { - private SetConnectingStateCommand(bool clearConnectionKey, bool retryAuth) + private SetConnectingStateCommand(bool clearConnectionKey, bool retryAuth, bool? connectivityConfirmed) { ClearConnectionKey = clearConnectionKey; RetryAuth = retryAuth; + ConnectivityConfirmed = connectivityConfirmed; } public bool ClearConnectionKey { get; } public bool RetryAuth { get; } - public static SetConnectingStateCommand Create(bool clearConnectionKey = false, bool retryAuth = false) => new SetConnectingStateCommand(clearConnectionKey, retryAuth); + /// + /// The connectivity answer already obtained by the DISCONNECTED that queued this command, so + /// the RTN17j check is not paid for twice in one cycle. Null when there is no answer on hand + /// - a timer driven retry, or a caller's Connect() - and those take their own. + /// + public bool? ConnectivityConfirmed { get; } + + public static SetConnectingStateCommand Create(bool clearConnectionKey = false, bool retryAuth = false, bool? connectivityConfirmed = null) => + new SetConnectingStateCommand(clearConnectionKey, retryAuth, connectivityConfirmed); protected override string ExplainData() { - return string.Empty; + return ConnectivityConfirmed.HasValue ? $"ConnectivityConfirmed: {ConnectivityConfirmed}" : string.Empty; } } diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index c8cb3dc70..32581c80b 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -880,13 +880,37 @@ private async Task HandleSetStateCommand(RealtimeCommand comman var defaultRealtimeHost = Client.Options.FullRealtimeHost(); - // Always retry on defaultPrimaryHost first when connecting command triggered by Disconnected/Suspended state timeout. + // RTN17 - every attempt considers a fallback, including the timer driven + // ones. Excluding those would lock a client out once the immediate retry + // budget is spent, since every remaining attempt is timer driven and so + // pinned to the primary. + // + // Asked speculatively, which costs nothing: GetHost only reads state, and + // RTN17i is its job - it returns to the primary whenever the last host + // was a fallback. + var candidateHost = AttemptsHelpers.GetHost(State, defaultRealtimeHost); var connectingHost = defaultRealtimeHost; - // Otherwise use host fallbacks if connecting command triggered by other commands - if (cmd.TriggeredByMessage.Contains("OnTimeOut()") == false) + // RTN17j - the connectivity check comes before the decision to use an + // alternative host. If the internet is unreachable the problem is not this + // host, so stay on the primary rather than working through fallbacks that + // cannot answer either. + // + // The answer is carried on the command, so it cannot outlive the decision + // it was taken for or be picked up by a CONNECTING another path queued. + // Held on the workflow it would have no bound, because the command loop + // abandons a nested batch once its depth guard trips. + // + // Note HandleConnectingTokenError reaches CreateTransport through + // AttemptANewConnection without a check - an RTN17j hole this does not + // close. + var alreadyConfirmed = cmd.ConnectivityConfirmed; + + if (candidateHost == defaultRealtimeHost || + alreadyConfirmed == true || + (alreadyConfirmed == null && await Client.RestClient.CanConnectToAbly())) { - connectingHost = AttemptsHelpers.GetHost(State, defaultRealtimeHost); + connectingHost = candidateHost; } SetNewHostInState(connectingHost); @@ -1010,7 +1034,21 @@ ErrorInfo TransformIfTokenErrorAndNotRetryable() if (retryInstantly) { - return SetConnectingStateCommand.Create().TriggeredBy(command); + State.AttemptsInfo.RecordInstantRetry(); + + // Handed to the command returned on the next line, and only to that one. + // Both this handler and the CONNECTING one need to know whether the + // internet is reachable - one to decide whether to retry now, the other + // whether to accept a fallback - and both were asking, on the workflow's + // single reader thread, one after the other. Two checks at up to + // MaxHttpOpenTimeout each is a loop held for twice as long as it needs to + // be on every failing attempt, and it eats into the RTN14e budget this + // series worked to make punctual. + // + // Carried on the command so it cannot go stale or be consumed by anything + // else. A timer driven retry carries no answer and takes its own check. + return SetConnectingStateCommand.Create(connectivityConfirmed: connectivityAnswer) + .TriggeredBy(command); } async Task CheckInstantRetryFlag() @@ -1020,9 +1058,42 @@ async Task CheckInstantRetryFlag() return true; } - if ((cmd.Error != null && cmd.Error.IsRetryableStatusCode()) || cmd.Exception != null) + // RTN17j sanctions reconnecting immediately, rather than waiting out the + // disconnected retry timeout, to work through the fallback domains. It + // does not sanction doing so without end, and every failed attempt + // produces another DISCONNECTED carrying an exception that qualifies + // again - so the traversal is bounded by the number of domains to + // traverse. Past that we are in RTN14d, where attempts are periodic and + // spaced per RTB1, and host selection continues at that slower pace. + var domainCount = 1 + State.Connection.FallbackHosts.Count; + if (State.AttemptsInfo.InstantRetryCount >= domainCount) + { + return false; + } + + // RTN15a and RTN15h3 - an unexpected transport drop or a non-token + // DISCONNECTED both earn an immediate reconnect. The first two tests + // cover the drop, the third the DISCONNECTED, which carries no status + // code of its own. + // + // Token errors are excluded because RTN15h3 is the "error other than a + // token error" clause: RTN15h2 owns them and has already queued its own + // CONNECTING behind this command, so granting a retry here too gives two + // overlapping attempts. + // + // Gated on the connectivity check, because the retry this grants is + // where host selection happens and RTN17j requires a check before an + // alternative host is used. + var reconnectImmediately = cmd.Exception != null + || (cmd.Error != null && cmd.Error.IsRetryableStatusCode()) + || (State.Connection.State == ConnectionState.Connected + && cmd.Error?.IsTokenError != true); + + if (reconnectImmediately) { - return await Client.RestClient.CanConnectToAbly(); + // Remembered so the CONNECTING behind this command does not repeat it. + connectivityAnswer = await Client.RestClient.CanConnectToAbly(); + return connectivityAnswer.Value; } return false; diff --git a/src/IO.Ably.Shared/Transport/ConnectionAttemptsInfo.cs b/src/IO.Ably.Shared/Transport/ConnectionAttemptsInfo.cs index 032796196..2ce7544b9 100644 --- a/src/IO.Ably.Shared/Transport/ConnectionAttemptsInfo.cs +++ b/src/IO.Ably.Shared/Transport/ConnectionAttemptsInfo.cs @@ -23,10 +23,23 @@ public ConnectionAttemptsInfo(Func now = null) internal bool TriedToRenewToken { get; private set; } + /// + /// How many times we have skipped the disconnected retry timeout and reconnected straight + /// away since last being connected. RTN17j sanctions retrying immediately to work through the + /// fallback domains, but the traversal must be bounded or RTB1 is never reached. + /// + internal int InstantRetryCount { get; private set; } + public void Reset() { Attempts.Clear(); TriedToRenewToken = false; + InstantRetryCount = 0; + } + + public void RecordInstantRetry() + { + InstantRetryCount++; } public void RecordTokenRetry() @@ -82,10 +95,16 @@ private void RecordAttemptFailure(ConnectionState state, ErrorInfo error) private void RecordAttemptFailure(ConnectionState state, Exception ex) { - if (Attempts.Any()) + // Mirrors the ErrorInfo overload above, including the empty-collection case, which is + // the normal one here: the only caller passing an exception is the transport dropping out + // of CONNECTED, and entering CONNECTED has just cleared the collection. Dropping it would + // leave FirstAttempt null, delaying the RTN14e clock, and DisconnectedCount at zero, + // which feeds RTN17 host selection. + var attempt = Attempts.LastOrDefault() ?? new ConnectionAttempt(_now()); + attempt.FailedStates.Add(new AttemptFailedState(state, ex)); + if (Attempts.Count == 0) { - var attempt = Attempts.Last(); - attempt.FailedStates.Add(new AttemptFailedState(state, ex)); + Attempts.Add(attempt); } } } diff --git a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionConnectedState.cs b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionConnectedState.cs index c61f182f2..3d03f136b 100644 --- a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionConnectedState.cs +++ b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionConnectedState.cs @@ -55,6 +55,9 @@ public override async Task OnMessageReceived(ProtocolMessage message, Real return true; } + // RTN15h3's immediate reconnect is granted by the SetDisconnectedStateCommand + // handler, which recognises a disconnect arriving while connected. Not requested + // here, which would bypass the RTN17j connectivity check. Context.ExecuteCommand(SetDisconnectedStateCommand.Create(message.Error).TriggeredBy("ConnectedState.OnMessageReceived()")); return true; case ProtocolMessage.MessageAction.Error: diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs index e0da14480..211c43da9 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs @@ -254,6 +254,75 @@ public async Task AckMessagesAreResentWhenConnectionIsDroppedAndResumed() client.State.WaitingForAck.Should().HaveCount(2); } + [Fact] + [Trait("spec", "RTN15h2")] + [Trait("spec", "RTN15h3")] + public async Task WithTokenError_ShouldNotAlsoGrantTheNonTokenImmediateReconnect() + { + // RTN15h3's immediate reconnect is for "an error other than a token error". RTN15h2 owns + // token errors and reconnects of its own accord, so granting a retry here as well gives + // two overlapping attempts - and the second reaches FAILED where RTN15h2 requires + // DISCONNECTED. + var client = await SetupConnectedClient(ConnectedClientErrors.FailRenewal); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = _tokenErrorInfo, + }); + + await client.ProcessCommands(); + + client.State.AttemptsInfo.InstantRetryCount.Should().Be(0); + client.Connection.State.Should().Be(ConnectionState.Disconnected); + } + + // UTS: realtime/unit/RTN15h3/non-token-error-resume-0 + [Fact] + [Trait("spec", "RTN15h3")] + public async Task WithNonTokenDisconnected_ShouldReconnectImmediately() + { + var client = await GetConnectedClient(opts => + opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = new ErrorInfo("Something else went wrong", 50000), + }); + + await client.ProcessCommands(); + + // Immediately, not in ten minutes. + client.State.AttemptsInfo.InstantRetryCount.Should().Be(1); + client.Connection.State.Should().Be(ConnectionState.Connecting); + } + + [Fact] + [Trait("spec", "RTN15h3")] + [Trait("spec", "RTN17j")] + public async Task WhenAConnectionSucceeds_ShouldClearTheImmediateRetryBudget() + { + // The budget that bounds RTN17j's traversal is per failure run, cleared by + // UpdateAttemptState's Connected case. Without that a client which spent its retries once + // would never get an immediate reconnect again, leaving RTN15h3 unimplemented from the + // second disconnect onwards. + var client = await GetConnectedClient(opts => + opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = new ErrorInfo("Something else went wrong", 50000), + }); + await client.ProcessCommands(); + + client.State.AttemptsInfo.InstantRetryCount.Should().Be(1); + + client.FakeProtocolMessageReceived(ConnectedProtocolMessage); + await client.WaitForState(ConnectionState.Connected); + await client.ProcessCommands(); + + client.State.AttemptsInfo.InstantRetryCount.Should().Be(0); + } + [Flags] private enum ConnectedClientErrors { diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs index 3100d12f7..a20cb0ed3 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs @@ -110,6 +110,117 @@ public async Task WhenPreviousAttemptFailed_ShouldGoToDefaultHostFirst() realtimeHosts.Last().Should().Be(Defaults.RealtimeHost); } + [Fact] + [Trait("spec", "RTN17")] + [Trait("spec", "RTN17j")] + public async Task WhenTheImmediateRetriesAreSpent_ShouldStillReachAFallbackHost() + { + // RTN17 - every attempt considers a fallback, including the timer driven ones. Skipping + // them would lock a client out entirely once the immediate retry budget is spent, since + // every remaining attempt is timer driven and would be pinned to the primary. + var client = await GetConnectedClient(opts => opts.DisconnectedRetryTimeout = TimeSpan.FromMilliseconds(10)); + + var hostsTried = new List(); + FakeTransportFactory.InitialiseFakeTransport = t => hostsTried.Add(t.Parameters.Host); + + // Spend the immediate retry budget and then some, so the later attempts are all + // timer driven. + var domainCount = 1 + client.State.Connection.FallbackHosts.Count; + for (var i = 0; i < domainCount + 3; i++) + { + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = new ErrorInfo { StatusCode = HttpStatusCode.GatewayTimeout }, + }); + + await client.ProcessCommands(); + + if (client.Connection.State != ConnectionState.Connecting) + { + await Task.Delay(50); + } + } + + client.State.AttemptsInfo.InstantRetryCount.Should().Be(domainCount); + hostsTried.Should().Contain(x => client.State.Connection.FallbackHosts.Contains(x)); + } + + [Fact] + [Trait("spec", "RTN17j")] + public async Task WhenAnImmediateRetryIsGranted_ShouldCheckConnectivityOnceForTheCycle() + { + // RTN17j asks for a connectivity check before an alternative host is used. Two decisions + // in the same cycle need the answer - whether to retry now, and whether to accept a + // fallback - and asking twice holds the workflow's single reader thread for up to two + // MaxHttpOpenTimeouts on every failing attempt. + var response = new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(Defaults.InternetCheckOkMessage), + }; + + var handler = new FakeHttpMessageHandler(response); + var client = GetClientWithFakeTransportAndMessageHandler(messageHandler: handler); + client.Options.SkipInternetCheck = false; + + await client.ConnectClient(); + await client.ProcessCommands(); + + handler.Requests.Clear(); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = new ErrorInfo { StatusCode = HttpStatusCode.GatewayTimeout }, + }); + + await client.ProcessCommands(); + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + var checks = handler.Requests + .Count(x => x.RequestUri.ToString().EqualsTo(Defaults.InternetCheckUrl)); + + checks.Should().Be(1); + } + + [Fact] + [Trait("spec", "RTN17j")] + public async Task WhenAConnectingCommandIsAbandoned_ShouldNotLeaveAnAnswerBehindForALaterAttempt() + { + // The answer is carried on the command so it cannot outlive the decision it was taken + // for. Held on the workflow it would have no bound: the command loop abandons a nested + // batch once its depth guard trips, so a dropped CONNECTING would leave the answer set + // for a later attempt to consume without any check of its own - which is exactly what + // RTN17j requires before using an alternative host. + var response = new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(Defaults.InternetCheckOkMessage), + }; + + var handler = new FakeHttpMessageHandler(response); + var client = GetClientWithFakeTransportAndMessageHandler(messageHandler: handler); + client.Options.SkipInternetCheck = false; + + await client.ConnectClient(); + await client.ProcessCommands(); + + // Grant an immediate retry, then abandon the CONNECTING it produced by consuming it + // directly rather than letting the loop deliver it. + var next = await client.Workflow.ProcessCommand(SetDisconnectedStateCommand.Create( + new ErrorInfo { StatusCode = HttpStatusCode.GatewayTimeout })); + + next.Should().ContainSingle().Which.Should().BeOfType(); + + handler.Requests.Clear(); + + // A fresh attempt that carries no answer of its own must take its own check. + await client.Workflow.ProcessCommand(SetConnectingStateCommand.Create()); + + var checks = handler.Requests + .Count(x => x.RequestUri.ToString().EqualsTo(Defaults.InternetCheckUrl)); + + checks.Should().Be(1); + } + [Fact] [Trait("spec", "RTN17e")] public async Task WithFallbackHost_ShouldMakeRestRequestsOnSameHost() From 0f3ef619036f978da7fc37478698aef368105176 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:45:59 +0100 Subject: [PATCH 06/10] Reinitialise on connect, bound the auth callback, and complete the teardown RTN11d - fixed. Only the channel half ran, so Connection.errorReason, msgSerial and the channel's channelSerial all survived into the new connection. RTN11b - fixed. connect() while CLOSING applied no part of RTN11d. RTN12b - fixed. The close timeout was a hardcoded 1s, not realtimeRequestTimeout, so on a slow link close() could reach CLOSED unacknowledged. TO3l11 - implemented. realtimeRequestTimeout was internal and could not be set. Validated at both ends; the upper bound is Int32.MaxValue ms, the tightest limit across every timer sink on every framework this package ships. RSA4c - fixed. The bound was applied to the task the callback returned, so a callback whose body runs synchronously was never bounded at all. RSA4c1 - fixed. cause was set as InnerException, which is not the spec's field. RTN8d, RTN9d - fixed. A throwing transition skipped the key clear and the transport teardown, leaving a terminal state holding a resumable key and a live transport. Also carries RealtimeWorkflowSpecs.cs in full: its additions are one contiguous insertion of complete test classes and cannot be split across the commits whose spec points they cover. Co-Authored-By: Claude Opus 5 --- src/IO.Ably.Shared/AblyAuth.cs | 27 +- src/IO.Ably.Shared/AuthOptions.cs | 7 + src/IO.Ably.Shared/ClientOptions.cs | 47 +- .../Realtime/Workflows/RealtimeWorkflow.cs | 112 +- .../Transport/ConnectionManager.cs | 6 +- .../Connection/ConnectionClosingState.cs | 5 +- .../ConnectionFailuresOnceConnectedSpecs.cs | 99 ++ .../ConnectionStateTests/ClosingStateSpecs.cs | 18 + .../Realtime/RealtimeWorkflowSpecs.cs | 1188 ++++++++++++++++- 9 files changed, 1477 insertions(+), 32 deletions(-) diff --git a/src/IO.Ably.Shared/AblyAuth.cs b/src/IO.Ably.Shared/AblyAuth.cs index 67cbbc7b3..1df81a98e 100644 --- a/src/IO.Ably.Shared/AblyAuth.cs +++ b/src/IO.Ably.Shared/AblyAuth.cs @@ -305,11 +305,27 @@ public virtual async Task RequestTokenAsync(TokenParams tokenParam bool shouldCatch = true; try { - var callbackResult = await authOptions.AuthCallback(tokenParams); + // RSA4c bounds an auth attempt by realtimeRequestTimeout. This one is awaited on + // the realtime workflow's single reader thread, so an authCallback that never + // returns would stall channel state, inbound messages and the idle monitor. + // + // Task.Run rather than awaiting the delegate directly: TimeoutAfter extends an + // already-created Task, so a callback whose body runs synchronously - the most + // ordinary shape in C# - would block before there is anything to bound. It + // cannot cancel the callback; an abandoned one runs to completion. + var callbackResult = await Task.Run(() => authOptions.AuthCallback(tokenParams)) + .TimeoutAfter(Options.RealtimeRequestTimeout, null); if (callbackResult == null) { - throw new AblyException("AuthCallback returned null", ErrorCodes.ClientAuthProviderRequestFailed); + // Covers a callback that returned null and one that timed out - TimeoutAfter + // yields null for both, and the wrapper below replaces the message anyway. + // ClientCallbackError, not ClientAuthProviderRequestFailed: the wrapper + // rethrows as 80019 per RSA4c1 with this as the cause, so 80019 here would + // duplicate its own parent. ably-js uses 40170 for the same case. + throw new AblyException( + $"AuthCallback returned null or did not return within {Options.RealtimeRequestTimeout.TotalSeconds}s", + ErrorCodes.ClientCallbackError); } if (callbackResult is TokenDetails) @@ -341,12 +357,17 @@ public virtual async Task RequestTokenAsync(TokenParams tokenParam : HttpStatusCode.Unauthorized; } + // RSA4c1 wants the cause "set to the underlying cause". The four argument overload + // assigns InnerException instead, which is not the spec's field and is not + // serialised as one. throw new AblyException( new ErrorInfo( "Error calling AuthCallback, token request failed. See inner exception for details.", ErrorCodes.ClientAuthProviderRequestFailed, statusCode, - ex), + href: null, + cause: (ex as AblyException)?.ErrorInfo ?? new ErrorInfo(ex.Message), + innerException: ex), ex); } } diff --git a/src/IO.Ably.Shared/AuthOptions.cs b/src/IO.Ably.Shared/AuthOptions.cs index 9ffa81271..c72acef36 100644 --- a/src/IO.Ably.Shared/AuthOptions.cs +++ b/src/IO.Ably.Shared/AuthOptions.cs @@ -13,6 +13,13 @@ public class AuthOptions /// /// The callback used to get a new or . /// AuthCallback is used by internally by .RequestTokenAsync. + /// + /// The callback is bounded by per RSA4c, + /// but cannot be cancelled: one that overruns is abandoned and keeps running, so a later auth + /// attempt may invoke it again concurrently and implementations must tolerate that. A result + /// returned after the bound is discarded, and each abandoned invocation holds a thread pool + /// worker until it returns. + /// /// public Func> AuthCallback { get; set; } diff --git a/src/IO.Ably.Shared/ClientOptions.cs b/src/IO.Ably.Shared/ClientOptions.cs index b93a57e4f..5c9e83591 100644 --- a/src/IO.Ably.Shared/ClientOptions.cs +++ b/src/IO.Ably.Shared/ClientOptions.cs @@ -295,6 +295,51 @@ public bool UseBinaryProtocol /// public TimeSpan ChannelRetryTimeout { get; set; } = Defaults.ChannelRetryTimeout; + private TimeSpan _realtimeRequestTimeout = Defaults.RealtimeRequestTimeout; + + /// + /// How long the library waits for Ably to answer before treating a realtime request as + /// having failed. Applies while establishing a connection, while awaiting a response to a + /// Heartbeat, Connect, Attach, Detach or Close, and as part of the RTN23a idle timeout. + /// Default: 10s. Must be a positive interval, and values beyond a minute or so are rarely + /// useful. Disabling the timeout is not supported - the timeouts this value drives are all + /// required to fire - so both Timeout.InfiniteTimeSpan and TimeSpan.MaxValue are rejected. + /// TO3l11 - https://sdk.ably.com/builds/ably/specification/main/features/#TO3l11. + /// + /// when set to zero, a negative value, or an + /// interval too large for the underlying timers to fire. + public TimeSpan RealtimeRequestTimeout + { + get => _realtimeRequestTimeout; + + set + { + // A non-positive value fails quietly rather than loudly: zero turns the RTN14c + // connect timeout into a hot reconnect loop, and a negative one stops the timer + // firing at all. Timeout.InfiniteTimeSpan is -1ms, so the lower bound catches it - + // which matters, because both Task.Delay and System.Threading.Timer accept it as + // genuine infinity and it would disable RTN14c, RTN12b, RTL4f, RTL5f and RSA4c. + // + // The upper bound is the tightest limit across the sinks this value reaches on every + // framework shipped: Task.Delay allows uint.MaxValue - 1 ms on .NET 6+ but only + // Int32.MaxValue on .NET Framework, Mono and Xamarin, and CountdownTimer casts to + // int for System.Threading.Timer. So Int32.MaxValue ms - a bound on the arithmetic, + // not a supported configuration, which is why the message names the useful range. + if (value <= TimeSpan.Zero || value.TotalMilliseconds > int.MaxValue) + { + throw new ArgumentOutOfRangeException( + nameof(RealtimeRequestTimeout), + value, + "RealtimeRequestTimeout must be a positive interval. The default is 10s; " + + "values beyond a minute or so are rarely useful. Disabling the timeout is " + + "not supported - the connect, close, attach, detach and auth timeouts it " + + "drives are all required to fire."); + } + + _realtimeRequestTimeout = value; + } + } + /// /// Timeout for opening an http request. /// Default: 4s. @@ -436,8 +481,6 @@ internal Func NowFunc internal bool SkipInternetCheck { get; set; } - internal TimeSpan RealtimeRequestTimeout { get; set; } = Defaults.RealtimeRequestTimeout; - /// /// Default constructor for ClientOptions. /// diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index 32581c80b..73ef3c2c8 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -360,6 +360,17 @@ private async Task ProcessCommandInner(RealtimeCommand command) switch (command) { case ConnectCommand _: + + // RTN11d - connect() out of CLOSED or FAILED starts afresh. The channel half, + // back to INITIALIZED with errorReason unset, is done per channel by the command + // queued below; Id and Key are already emptied on entering CLOSED or FAILED. + if (State.Connection.State == ConnectionState.Closed || + State.Connection.State == ConnectionState.Failed) + { + State.Connection.ErrorReason = null; + State.Connection.MessageSerial = 0; + } + var nextCommand = ConnectionManager.Connect(); var initFailedChannelsOnConnect = ChannelCommand.CreateForAllChannels(InitialiseFailedChannelsOnConnect.Create().TriggeredBy(command)); @@ -949,14 +960,36 @@ private async Task HandleSetStateCommand(RealtimeCommand comman case SetFailedStateCommand cmd: - ClearAckQueueAndFailMessages(ErrorInfo.ReasonFailed); - var error = TransformIfTokenErrorAndNotRetryable(); var failedState = new ConnectionFailedState(ConnectionManager, error, Logger); - SetState(failedState); - State.Connection.ClearKeyAndId(); // RTN8c, RTN9c - ConnectionManager.DestroyTransport(); + // RTN7e - the queued messages are failed with "an error representing the + // reason for the state change", taken off the state object so it is this + // transition's reason even if SetState early-returns. In the finally, after + // the transition, so a publisher's callback sees the state it is being told + // about and a throwing transition cannot strand the messages uncalled. + // ably-js orders it the same way: enactStateChange then failQueuedMessages. + // + // RTN7e - the queued messages are failed with "an error representing the + // reason for the state change", taken off the state object so it is this + // transition's reason even if SetState early-returns. In the finally, after + // the transition, so a publisher's callback sees the state it is being told + // about and a throwing transition cannot strand the messages uncalled. + // ably-js orders it the same way: enactStateChange then failQueuedMessages. + // + // RTN8d and RTN9d share that finally: the connection has entered the state + // by the time SetState rethrows, so a throw must not leave it reporting a + // terminal state while still holding a resumable key and a live transport. + try + { + SetState(failedState); + } + finally + { + ClearAckQueueAndFailMessages(failedState.Error); + State.Connection.ClearKeyAndId(); // RTN8d, RTN9d + ConnectionManager.DestroyTransport(); + } ErrorInfo TransformIfTokenErrorAndNotRetryable() { @@ -1108,7 +1141,7 @@ async Task CheckInstantRetryFlag() var closingState = new ConnectionClosingState(ConnectionManager, connectedTransport, Logger); SetState(closingState); - State.Connection.ClearKeyAndId(); // RTN8c, RTN9c + State.Connection.ClearKeyAndId(); // RTN8d, RTN9d if (connectedTransport) { @@ -1126,27 +1159,49 @@ async Task CheckInstantRetryFlag() State.Connection.ClearKey(); } - ClearAckQueueAndFailMessages(ErrorInfo.ReasonSuspended); - var suspendedState = new ConnectionSuspendedState(ConnectionManager, cmd.Error, Logger); - SetState(suspendedState); - State.Connection.ClearKeyAndId(); // RTN8c, RTN9c + + // RTN7e and the teardown - see the note on the FAILED case. + try + { + SetState(suspendedState); + } + finally + { + ClearAckQueueAndFailMessages(suspendedState.Error); + + // Deliberately NOT RTN8d/RTN9d, which name only CLOSED, CLOSING and + // FAILED. Clearing here is this library's pre-6.1.0 behaviour, kept + // because it is coupled to the connectionStateTtl freshness check that + // also predates 6.1.0. + State.Connection.ClearKeyAndId(); + + // Needed here as well as in the DISCONNECTED handler, which diverts to + // this case before reaching its own DestroyTransport. A surviving + // transport keeps its listener for up to suspendedRetryTimeout. + ConnectionManager.DestroyTransport(); + } break; case SetClosedStateCommand cmd: - ClearAckQueueAndFailMessages(ErrorInfo.ReasonClosed); - var closedState = new ConnectionClosedState(ConnectionManager, cmd.Error, Logger) { Exception = cmd.Exception, }; - SetState(closedState); - State.Connection.ClearKeyAndId(); // RTN8c, RTN9c - - ConnectionManager.DestroyTransport(); + // RTN7e and the teardown - see the note on the FAILED case. + try + { + SetState(closedState); + } + finally + { + ClearAckQueueAndFailMessages(closedState.Error); + State.Connection.ClearKeyAndId(); // RTN8d, RTN9d + ConnectionManager.DestroyTransport(); + } break; } @@ -1177,6 +1232,8 @@ public void SetState(ConnectionStateBase newState, bool skipTimer = false) Logger.Debug(message); } + var notified = false; + try { if (newState.IsUpdate == false) @@ -1204,13 +1261,28 @@ public void SetState(ConnectionStateBase newState, bool skipTimer = false) Logger.Debug($"xx {newState.State}: Skipping attaching."); } + notified = true; UpdateStateAndNotifyConnection(newState); } - catch (AblyException ex) + catch (Exception ex) { - Logger.Error("Error attaching to context", ex); - - UpdateStateAndNotifyConnection(newState); + // Everything, not just AblyException: anything else thrown by StartTimer or the + // state object would reach the command loop, which logs and drops it, leaving the + // connection with no transport, no timer and no state change emitted. The transition + // is still completed below and the exception still rethrown. + Logger.Error($"Error attaching to context while changing state to {newState.State}", ex); + + // Only if the notify has not already happened. A throw during the transition lands + // here after the state change has been emitted - StartTimer is one source, and a + // negative retry timeout reaches System.Threading.Timer. Not a channel's + // ConnectionStateChanged, which RealtimeChannels guards per channel. Re-emitting is harmless for an ordinary transition, which the + // same-state check swallows, but an RTN24 update has no such check and was emitted + // twice. The flag is set before the call so a throw from inside it does not trigger + // a second attempt either. + if (notified == false) + { + UpdateStateAndNotifyConnection(newState); + } newState.AbortTimer(); diff --git a/src/IO.Ably.Shared/Transport/ConnectionManager.cs b/src/IO.Ably.Shared/Transport/ConnectionManager.cs index d2a2869f2..ed699c7f8 100644 --- a/src/IO.Ably.Shared/Transport/ConnectionManager.cs +++ b/src/IO.Ably.Shared/Transport/ConnectionManager.cs @@ -172,12 +172,14 @@ internal async Task OnAuthUpdated(TokenDetails tokenDetails, bool wait) { while (true) { - var (success, newState) = await waiter.Wait(Defaults.RealtimeRequestTimeout); + // Options rather than Defaults, since TO3l11 makes realtimeRequestTimeout a + // client option. + var (success, newState) = await waiter.Wait(Options.RealtimeRequestTimeout); if (success == false) { throw new AblyException( new ErrorInfo( - $"Connection state didn't change after Auth updated within {Defaults.RealtimeRequestTimeout}", + $"Connection state didn't change after Auth updated within {Options.RealtimeRequestTimeout}", 40140)); } diff --git a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs index 7a7a854ab..f6b63c906 100644 --- a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs +++ b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs @@ -10,7 +10,6 @@ internal class ConnectionClosingState : ConnectionStateBase { public override ErrorInfo DefaultErrorInfo => ErrorInfo.ReasonClosed; - private const int CloseTimeout = 1000; private readonly bool _connectedTransport; private readonly ICountdownTimer _timer; @@ -62,7 +61,9 @@ public override void StartTimer() { if (_connectedTransport) { - _timer.Start(TimeSpan.FromMilliseconds(CloseTimeout), OnTimeOut); + // RTN12b - the wait for the CLOSED message is realtimeRequestTimeout, which TO3l11 + // makes a client option. + _timer.Start(Context.DefaultTimeout, OnTimeOut); } } diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs index 211c43da9..c2022d6fc 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Linq; using System.Net; +using System.Threading; using System.Threading.Tasks; using IO.Ably.Realtime; @@ -323,6 +324,104 @@ public async Task WhenAConnectionSucceeds_ShouldClearTheImmediateRetryBudget() client.State.AttemptsInfo.InstantRetryCount.Should().Be(0); } + [Fact] + [Trait("spec", "RSA4c")] + public async Task WithASynchronouslyBlockingAuthCallback_ShouldStillBoundTheAttempt() + { + // RSA4c - TimeoutAfter extends an already-created Task, so the callback has to be invoked + // through Task.Run to be bounded at all. A callback whose body runs synchronously - the + // most ordinary C# shape - would otherwise block before there is anything to bound and + // hold the workflow's single reader thread for as long as it takes. + var released = new ManualResetEventSlim(false); + var client = GetClientWithFakeTransport(opts => + { + opts.Key = ValidKey; + opts.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(200); + opts.AuthCallback = _ => + { + released.Wait(TimeSpan.FromSeconds(30)); + return Task.FromResult(new TokenDetails("blocked")); + }; + }); + + try + { + var sw = Stopwatch.StartNew(); + var error = await Assert.ThrowsAsync(() => client.Auth.AuthorizeAsync()); + sw.Stop(); + + // The specific failure, not merely any failure - a bare IsFaulted check would also + // have been satisfied by an unrelated fast fault. + error.ErrorInfo.Code.Should().Be(ErrorCodes.ClientAuthProviderRequestFailed); + error.ErrorInfo.Cause.Should().NotBeNull(); + error.ErrorInfo.Cause.Code.Should().Be(ErrorCodes.ClientCallbackError); + + // Comfortably inside the 30s the callback blocks for, and comfortably outside the + // 200ms bound so a loaded run does not flake. + sw.Elapsed.Should().BeLessThan(TimeSpan.FromSeconds(5)); + } + finally + { + released.Set(); + } + } + + [Fact] + [Trait("spec", "RSA4c1")] + public async Task WhenTheAuthCallbackFails_ShouldSetTheCauseNotOnlyTheInnerException() + { + // RSA4c1 wants an ErrorInfo "with code 80019, statusCode 401, and cause set to the + // underlying cause". The four argument overload assigns InnerException instead, which is + // not the spec's field and is not serialised as one. + var client = GetClientWithFakeTransport(opts => + { + opts.Key = ValidKey; + opts.AuthCallback = _ => throw new AblyException(new ErrorInfo("the underlying cause", 40100)); + }); + + var error = await Assert.ThrowsAsync(() => client.Auth.AuthorizeAsync()); + + error.ErrorInfo.Code.Should().Be(ErrorCodes.ClientAuthProviderRequestFailed); + error.ErrorInfo.StatusCode.Should().Be(HttpStatusCode.Unauthorized); + error.ErrorInfo.Cause.Should().NotBeNull(); + error.ErrorInfo.Cause.Message.Should().Be("the underlying cause"); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + [Trait("spec", "TO3l11")] + public void WithANonPositiveRealtimeRequestTimeout_ShouldReject(int seconds) + { + var options = new ClientOptions(ValidKey); + + Assert.Throws( + () => options.RealtimeRequestTimeout = TimeSpan.FromSeconds(seconds)); + } + + [Fact] + [Trait("spec", "TO3l11")] + public void WithARealtimeRequestTimeoutTooLargeForATimer_ShouldReject() + { + // TimeSpan.MaxValue is the idiomatic "never time out", and it reaches Task.Delay through + // TimeoutAfter, which rejects anything over uint.MaxValue - 1 ms - surfacing as a code-0 + // error out of Authorize(). Rejected up front instead. + var options = new ClientOptions(ValidKey); + + Assert.Throws( + () => options.RealtimeRequestTimeout = TimeSpan.MaxValue); + + // uint.MaxValue - 1 is Task.Delay's limit on .NET 6+ only: it is above what net46, Mono + // and Xamarin accept, and above what CountdownTimer's cast to int can carry into + // System.Threading.Timer on any framework. + Assert.Throws( + () => options.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(uint.MaxValue - 1)); + + // Just inside the limit is still allowed. + options.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(int.MaxValue); + options.RealtimeRequestTimeout.TotalMilliseconds.Should().Be(int.MaxValue); + } + [Flags] private enum ConnectedClientErrors { diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/ClosingStateSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/ClosingStateSpecs.cs index 1ca7d1139..85c82b985 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/ClosingStateSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/ClosingStateSpecs.cs @@ -1,3 +1,4 @@ +using System; using System.Threading.Tasks; using FluentAssertions; using IO.Ably.Realtime; @@ -96,6 +97,23 @@ public async Task ShouldHandleInboundDisconnectedMessageAndGoToDisconnectedState _context.ShouldQueueCommand(); } + [Fact] + [Trait("spec", "RTN12b")] + [Trait("spec", "TO3l11")] + public void StartTimer_ShouldWaitRealtimeRequestTimeout() + { + // RTN12b names the duration: "If the CLOSED ProtocolMessage is not received within + // realtimeRequestTimeout, the transport will be disconnected and the connection will + // automatically transition to the CLOSED state". The test below covers what happens when + // the timer fires; this pins how long it waits. + _context.DefaultTimeout = TimeSpan.FromMilliseconds(1234); + + var state = GetState(connectedTransport: true); + state.StartTimer(); + + _timer.LastDelay.Should().Be(TimeSpan.FromMilliseconds(1234)); + } + [Fact] [Trait("spec", "RTN12b")] public async Task AfterTimeoutExpires_ShouldForceStateToClosed() diff --git a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs index 4d07db7e8..1f91288c3 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs @@ -1,10 +1,13 @@ using System; using System.Collections.Generic; +using System.Linq; +using System.Net; using System.Threading.Tasks; using IO.Ably.Realtime; using IO.Ably.Realtime.Workflow; using IO.Ably.Tests.Infrastructure; +using IO.Ably.Tests.Realtime; using IO.Ably.Transport; using IO.Ably.Types; @@ -70,15 +73,29 @@ public class ConnectingStateSpecs : AblyRealtimeSpecs [Trait("spec", "RTN14g")] public async Task WithInboundErrorMessage_WhenNotTokenErrorAndChannelsEmpty_GoesToFailed() { - var client = GetRealtimeClient(opts => opts.RealtimeHost = "non-default.ably.io"); // Force no fallback + // A custom host means no fallbacks. The transport is faked and held short of + // Connected, and realtimeRequestTimeout put out of reach, so neither a real DNS + // failure nor the CONNECTING timeout can reach Disconnected before the injected + // error does. + FakeTransportFactory.InitialiseFakeTransport = + transport => transport.OnConnectChangeStateToConnected = false; + + var client = GetClientWithFakeTransport(opts => + { + opts.RealtimeHost = "non-default.ably.io"; + opts.RealtimeRequestTimeout = TimeSpan.FromMinutes(1); + }); await client.WaitForState(ConnectionState.Connecting); // Arrange ErrorInfo targetError = new ErrorInfo("test", 123); - // Act - client.Workflow.ProcessCommand(ProcessMessageCommand.Create(new ProtocolMessage(ProtocolMessage.MessageAction.Error) { Error = targetError })); + // Queued and drained rather than fired and forgotten: an un-awaited ProcessCommand + // can have its continuation delayed past the wait below under a parallel suite. + client.ExecuteCommand(ProcessMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Error) { Error = targetError })); + await client.ProcessCommands(); // Assert await client.WaitForState(ConnectionState.Failed); @@ -148,6 +165,45 @@ public async Task WhenDisconnectedWithFallback_ShouldRetryConnectionImmediately( states.Should().BeEquivalentTo(new[] { ConnectionState.Disconnected, ConnectionState.Connecting }); } + [Fact] + [Trait("spec", "RTN17j")] + [Trait("spec", "RTN14d")] + public async Task InstantRetries_ShouldBeBoundedByTheNumberOfDomains() + { + // RTN17j sanctions reconnecting immediately to work through the fallback domains, but + // the traversal is bounded - every failure carries an exception and so qualifies + // again, which would mean RTB1 is never reached. + var client = GetClientWithFakeTransport(opts => opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + var domainCount = 1 + client.State.Connection.FallbackHosts.Count; + domainCount.Should().BeGreaterThan(1); + + var connectingCount = 0; + client.Connection.On(x => + { + if (x.Current == ConnectionState.Connecting) + { + connectingCount++; + } + }); + + // Each failure carries an exception, so each one qualifies for an instant retry. + for (var i = 0; i < domainCount + 3; i++) + { + client.ExecuteCommand(SetDisconnectedStateCommand.Create( + ErrorInfo.ReasonDisconnected, exception: new Exception("transport gone"))); + await client.ProcessCommands(); + } + + // One per domain. The long retry timeout means nothing else can have produced a + // CONNECTING, so anything past the bound went to the timer instead. + connectingCount.Should().Be(domainCount); + client.State.AttemptsInfo.InstantRetryCount.Should().Be(domainCount); + client.Connection.State.Should().Be(ConnectionState.Disconnected); + } + [Fact] public async Task ShouldCreateTransport() { @@ -570,12 +626,1138 @@ void Callback(bool ack, ErrorInfo err) Assert.True(callbacks.TrueForAll(c => ReferenceEquals(c.Item2, error))); // Error } + [Fact] + [Trait("spec", "RTN7b")] + [Trait("spec", "RTN19a")] + public async Task WhenAnAckedMessageFailsToSend_ShouldHoldItInOneQueueOnly() + { + // A message can be awaiting an ACK or queued for a later connection, never both. + // RTN19a resends WaitingForAck on reconnect, so a message in both queues is sent + // twice and the second msgSerial assignment renumbers the copy WaitingForAck holds, + // leaving a hole in the sequence Ably is tracking. + // + // No caller produces this shape today - AckRequired implies CanSend and so + // CONNECTED, where CanQueue is false - so the invariant is asserted directly. + var client = GetClientWithFakeTransport(); + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + // CONNECTING can queue, so forcing an ack-requiring message there reaches both + // branches. A throwing write is what SendToTransport turns into a failure. + LastCreatedTransport.SetSendAction(_ => throw new Exception("socket gone")); + + var message = new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"); + client.ExecuteCommand(SendMessageCommand.Create(message, (_, __) => { }, force: true)); + + await client.ProcessCommands(); + + client.State.WaitingForAck.Should().HaveCount(1); + client.State.PendingMessages.Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "RTN7b")] + [Trait("spec", "RTN19a")] + public async Task WhenANonAckedMessageFailsToSend_ShouldStillQueueItForTheNextConnection() + { + // The other half: a message not awaiting an ACK is tracked nowhere else, so dropping + // it here would lose it silently. CLOSE is what the two force:true callers send. + var client = GetClientWithFakeTransport(); + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + LastCreatedTransport.SetSendAction(_ => throw new Exception("socket gone")); + + var message = new ProtocolMessage(ProtocolMessage.MessageAction.Close); + client.ExecuteCommand(SendMessageCommand.Create(message, (_, __) => { }, force: true)); + + await client.ProcessCommands(); + + client.State.WaitingForAck.Should().BeEmpty(); + client.State.PendingMessages.Should().HaveCount(1); + } + + // UTS: realtime/unit/RTN7e/error-represents-reason-4 + [Theory] + [InlineData(ConnectionState.Failed)] + [InlineData(ConnectionState.Suspended)] + [InlineData(ConnectionState.Closed)] + [Trait("spec", "RTN7e")] + public async Task WhenTheConnectionFailsAMessage_ShouldReportTheReasonForTheStateChange( + ConnectionState state) + { + // RTN7e: the callback "should be called with an error representing the reason for + // the state change" - this transition's reason, not a module level constant. + var client = await GetConnectedClient(); + var reason = new ErrorInfo("the actual reason", 12345); + + var errors = new List(); + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"), + (_, error) => errors.Add(error))); + await client.ProcessCommands(); + + RealtimeCommand command = state switch + { + ConnectionState.Failed => SetFailedStateCommand.Create(reason), + ConnectionState.Suspended => SetSuspendedStateCommand.Create(reason), + _ => SetClosedStateCommand.Create(reason), + }; + + client.ExecuteCommand(command); + await client.ProcessCommands(); + + errors.Should().ContainSingle(); + errors[0].Message.Should().Be("the actual reason"); + } + + [Fact] + [Trait("spec", "RTN7e")] + public async Task WhenTheConnectionFailsAMessage_ShouldHaveAlreadyEnteredTheNewState() + { + // An application inspecting the connection from inside its publish callback sees the + // state it is being told about, not the previous one. ably-js orders + // enactStateChange before failQueuedMessages for the same reason. + var client = await GetConnectedClient(); + + var seen = new List(); + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"), + (_, __) => seen.Add(client.Connection.State))); + await client.ProcessCommands(); + + client.ExecuteCommand(SetSuspendedStateCommand.Create(new ErrorInfo("gone", 1))); + await client.ProcessCommands(); + + seen.Should().Equal(new[] { ConnectionState.Suspended }); + } + + [Theory] + [InlineData(ConnectionState.Suspended)] + [Trait("spec", "RTN7e")] + public async Task WhenTheTransitionThrows_ShouldStillFailTheMessage(ConnectionState state) + { + // The connection enters the state before the throw, so RTN7e applies whether or not + // the transition threw - hence the finally. Reachable from application code: + // SuspendedRetryTimeout is public and unvalidated, and a negative value makes + // System.Threading.Timer throw. + // + // SUSPENDED only, because ConnectionSuspendedState.StartTimer is the one state whose + // timer takes a caller-supplied delay. + var client = await GetConnectedClient(opts => + opts.SuspendedRetryTimeout = TimeSpan.FromSeconds(-5)); + + var errors = new List(); + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"), + (_, error) => errors.Add(error))); + await client.ProcessCommands(); + client.State.WaitingForAck.Should().HaveCount(1); + + RealtimeCommand command = state switch + { + ConnectionState.Failed => SetFailedStateCommand.Create(new ErrorInfo("gone", 1)), + ConnectionState.Suspended => SetSuspendedStateCommand.Create(new ErrorInfo("gone", 1)), + _ => SetClosedStateCommand.Create(new ErrorInfo("gone", 1)), + }; + + client.ExecuteCommand(command); + await client.ProcessCommands(); + + client.Connection.State.Should().Be(state); + errors.Should().ContainSingle(); + client.State.WaitingForAck.Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "RTN7e")] + public async Task WhenAlreadyInTheState_ShouldNotReportThePreviousReason() + { + // SetState early-returns when already in the target state, before + // Connection.ErrorReason is updated - so the reason is read off the state object + // being entered, which cannot go stale. + var client = await GetConnectedClient(); + + client.ExecuteCommand(SetSuspendedStateCommand.Create(new ErrorInfo("first reason", 1))); + await client.ProcessCommands(); + + var errors = new List(); + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"), + (_, error) => errors.Add(error), + force: true)); + await client.ProcessCommands(); + + client.ExecuteCommand(SetSuspendedStateCommand.Create(new ErrorInfo("second reason", 2))); + await client.ProcessCommands(); + + errors.Should().ContainSingle(); + errors[0].Message.Should().Be("second reason"); + } + public AckProtocolTests(ITestOutputHelper output) : base(output) { } } + [Trait("spec", "RTN11d")] + public class ReinitialiseOnConnectSpecs : AblyRealtimeSpecs + { + [Theory] + [InlineData(ConnectionState.Closed)] + [InlineData(ConnectionState.Failed)] + public async Task WhenConnectingFromClosedOrFailed_ShouldReinitialiseEverythingRTN11dNames(ConnectionState from) + { + var client = await GetClientWithHistory(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + + await MoveTo(client, from); + + // Preconditions: there is something to clear on all four counts. + channel.State.Should().NotBe(ChannelState.Initialized); + channel.ErrorReason.Should().NotBeNull(); + client.Connection.ErrorReason.Should().NotBeNull(); + client.State.Connection.MessageSerial.Should().Be(3); + + client.Connect(); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Initialized); + channel.ErrorReason.Should().BeNull(); + client.Connection.ErrorReason.Should().BeNull(); + client.State.Connection.MessageSerial.Should().Be(0); + } + + [Fact] + public async Task WhenConnectingFromClosed_ShouldResetTheSerialAtConnectTime() + { + // RTN11d asks for the reset at connect(), not at the next CONNECTED. Not framed + // around Connection.RecoveryKey, which returns empty for the whole of that window. + var client = await GetClientWithHistory(); + await MoveTo(client, ConnectionState.Closed); + + client.Connect(); + await client.ProcessCommands(); + + client.State.Connection.MessageSerial.Should().Be(0); + } + + [Fact] + public async Task WhenConnectingFromDisconnected_ShouldLeaveTheSerialAlone() + { + // RTN11d applies only to CLOSED and FAILED. From DISCONNECTED the connection may + // still be resumable, and RTN19a2 needs the sequence intact to resend with it. + var client = await GetClientWithHistory(); + await MoveTo(client, ConnectionState.Disconnected); + + client.Connect(); + await client.ProcessCommands(); + + client.State.Connection.MessageSerial.Should().Be(3); + } + + [Fact] + [Trait("spec", "RTN11b")] + [Trait("spec", "RTL3b")] + public async Task WhenTheConnectionCloses_ShouldDetachChannelsBeforeReinitialisingThem() + { + // RTN11b: "the client should ensure that all channels first transition to DETACHED, + // following RTL3b, and then reinitialize channels per RTN11d". The DETACHED branch + // is the only caller of Presence.ChannelDetachedOrFailed, so skipping it would carry + // presence members from the abandoned connection into the next one. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + var states = new List(); + channel.On(args => states.Add(args.Current)); + + client.Close(); + await client.WaitForState(ConnectionState.Closing); + await client.ProcessCommands(); + + states.Should().Contain(ChannelState.Detached); + + client.Connect(); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Initialized); + } + + [Fact] + [Trait("spec", "RTN11b")] + [Trait("spec", "RTL4f")] + public async Task WhenTheConnectionCloses_ShouldNotLetAStaleAwaiterSuspendAReinitialisedChannel() + { + // The awaiters are failed on CLOSING, so an attach still in flight cannot keep its + // timer and later apply RTL4f's SUSPENDED to a channel RTN11d has already reset. + var client = await GetConnectedClient(opts => + opts.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(200)); + + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.Attach(); + await client.ProcessCommands(); + channel.State.Should().Be(ChannelState.Attaching); + + client.Close(); + await client.WaitForState(ConnectionState.Closing); + await client.ProcessCommands(); + + channel.AttachedAwaiter.Waiting.Should().BeFalse(); + + client.Connect(); + await client.ProcessCommands(); + channel.State.Should().Be(ChannelState.Initialized); + + // Long enough that the original attach timer would have fired. + await Task.Delay(500); + + channel.State.Should().NotBe(ChannelState.Suspended); + } + + [Theory] + [InlineData(ChannelState.Attached)] + [InlineData(ChannelState.Suspended)] + [InlineData(ChannelState.Detaching)] + [Trait("spec", "RTN11b")] + [Trait("spec", "RTP5a")] + public async Task WhenTheConnectionCloses_ShouldClearPresenceForEveryPendingChannel( + ChannelState from) + { + // RTP5a clears the presence maps on entering DETACHED, so any channel left in + // another state would carry its members from the abandoned connection into the next + // one - hence all four pending states, not just the two RTL3b names. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + channel.Presence.MembersMap.Put( + new PresenceMessage(PresenceAction.Enter, "ghost") { ConnectionId = "old" }); + channel.Presence.InternalMembersMap.Put( + new PresenceMessage(PresenceAction.Enter, "ghost") { ConnectionId = "old" }); + + channel.SetChannelState(from); + await client.ProcessCommands(); + channel.Presence.MembersMap.Values.Should().NotBeEmpty(); + + client.Close(); + await client.WaitForState(ConnectionState.Closing); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Detached); + channel.Presence.MembersMap.Values.Should().BeEmpty(); + channel.Presence.InternalMembersMap.Values.Should().BeEmpty(); + } + + [Theory] + [InlineData(ChannelState.Attached)] + [InlineData(ChannelState.Suspended)] + [InlineData(ChannelState.Detaching)] + [Trait("spec", "RTP5a")] + public async Task WhenTheConnectionClosesFromSuspended_ShouldStillClearPresence(ChannelState from) + { + // ConnectionSuspendedState.Close() queues SetClosedStateCommand directly, so CLOSING + // never happens and the CLOSED branch has to cover the same RTP5a teardown. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + channel.Presence.MembersMap.Put( + new PresenceMessage(PresenceAction.Enter, "ghost") { ConnectionId = "old" }); + channel.Presence.InternalMembersMap.Put( + new PresenceMessage(PresenceAction.Enter, "ghost") { ConnectionId = "old" }); + + client.ExecuteCommand(SetSuspendedStateCommand.Create(ErrorInfo.ReasonSuspended)); + await client.WaitForState(ConnectionState.Suspended); + channel.SetChannelState(from); + await client.ProcessCommands(); + + client.Close(); + await client.WaitForState(ConnectionState.Closed); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Detached); + channel.Presence.MembersMap.Values.Should().BeEmpty(); + channel.Presence.InternalMembersMap.Values.Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "RTL24")] + public async Task AfterACleanClose_ShouldNotStampAFabricatedChannelError() + { + // RTL24 lists RTN11d, RTL3a, RTL4g and RTL14 as the sources of channel errorReason. A + // clean close is none of them, so the ErrorInfo minted for RTL11's presence callbacks + // must not reach the channel. ably-js passes change.reason, which is null here. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + client.Close(); + await client.WaitForState(ConnectionState.Closed); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Detached); + channel.ErrorReason.Should().BeNull(); + } + + [Fact] + [Trait("spec", "RTL5e")] + public async Task WhenAPendingDetachCompletesOnClose_ShouldReportSuccess() + { + // The detach the caller asked for does complete - CLOSING takes the channel to + // DETACHED - so the awaiter must report success, not failure. ably-js resolves + // detach() on that transition too. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + var detach = channel.DetachAsync(); + await client.ProcessCommands(); + channel.State.Should().Be(ChannelState.Detaching); + + client.Close(); + await client.WaitForState(ConnectionState.Closing); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Detached); + (await detach).IsSuccess.Should().BeTrue(); + } + + // UTS: realtime/unit/RTL5l/detach-attached-when-disconnected-1 + [Fact] + [Trait("spec", "RTL5l")] + public async Task WhenDetachingWhileDisconnected_ShouldDetachImmediately() + { + // RTL5l is "anything other than CONNECTED", which includes DISCONNECTED - where an + // enumerated check would park the DETACH in the RTL6c2 queue and never call back. + var client = await GetConnectedClient(opts => + opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.WaitForState(ConnectionState.Disconnected); + await client.ProcessCommands(); + + var detach = await channel.DetachAsync(); + + detach.IsSuccess.Should().BeTrue(); + channel.State.Should().Be(ChannelState.Detached); + client.State.PendingMessages.Should().BeEmpty(); + } + + [Theory] + [InlineData(ConnectionState.Failed)] + [InlineData(ConnectionState.Closed)] + [InlineData(ConnectionState.Suspended)] + [Trait("spec", "RTN7e")] + [Trait("spec", "RTN8d")] + [Trait("spec", "RTN9d")] + public async Task WhenTheTransitionThrows_ShouldStillClearTheKeyAndDestroyTheTransport( + ConnectionState state) + { + // RTN8d and RTN9d: connectionId and connectionKey are null in CLOSED, CLOSING and + // FAILED. The connection has entered the state by the time SetState rethrows, so + // leaving the clear outside the finally meant a throwing transition reporting the + // terminal state while still holding a resumable key, and left a live transport + // whose listener kept refreshing the activity timestamp behind an RTN23a monitor + // gated on Connected. RTN7e's failure of the ack queue was skipped the same way, + // stranding those messages with no callback at all. + // + // All three sites carry the same finally, so all three are driven here. + var client = await GetConnectedClient(); + client.State.WaitingForAck.Add(new MessageAndCallback(new ProtocolMessage(), null)); + + // Connection.NotifyUpdate invokes internal handlers unguarded, which puts the throw + // where the bug needed it: after the connection has entered the state, and before + // the teardown. A plain Exception rather than an AblyException, so the workflow's + // own catch does not convert the outcome into FAILED and hide the state under test. + client.Connection.InternalStateChanged += (_, change) => + { + if (change.Current == state) + { + throw new Exception("thrown from a state change handler"); + } + }; + + client.ExecuteCommand(state switch + { + ConnectionState.Failed => SetFailedStateCommand.Create(new ErrorInfo("gone", 1)), + ConnectionState.Closed => SetClosedStateCommand.Create(), + _ => SetSuspendedStateCommand.Create(new ErrorInfo("gone", 1)), + }); + await client.ProcessCommands(); + + client.Connection.State.Should().Be(state); + client.State.WaitingForAck.Should().BeEmpty(); + client.State.Connection.Key.Should().BeEmpty(); + client.State.Connection.Id.Should().BeEmpty(); + client.ConnectionManager.Transport.Should().BeNull(); + } + + [Fact] + [Trait("spec", "RTN21")] + [Trait("spec", "RTN15b")] + [Trait("spec", "RTN8b")] + public async Task WhenAConnectedCarriesNoConnectionDetails_ShouldKeepTheKeyAndTakeTheId() + { + // The two fields are guarded differently on purpose. connectionId is top-level and + // always meaningful per RTN8b - ConnectionIdSpecs pins that a CONNECTED carrying only + // an id sets it. The key lives inside connectionDetails, which is what RTN21 scopes + // its override to, so a message carrying none must leave the key alone or a live + // connection is left with nothing to resume with under RTN15b. + var client = await GetConnectedClient(); + client.State.Connection.Key.Should().Be("connectionKey"); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "a-different-id", + }); + + await client.ProcessCommands(); + + client.State.Connection.Key.Should().Be("connectionKey"); + client.State.Connection.Id.Should().Be("a-different-id"); + } + + private async Task GetClientWithHistory() + { + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + + for (var i = 0; i < 3; i++) + { + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"))); + } + + await client.ProcessCommands(); + client.State.Connection.MessageSerial.Should().Be(3); + return client; + } + + private static async Task MoveTo(AblyRealtime client, ConnectionState state) + { + var error = new ErrorInfo("something went wrong", 50000); + + switch (state) + { + case ConnectionState.Closed: + // Closed carries no error of its own, so put one on the channel and the + // connection first - RTN11d has to clear both. + client.ExecuteCommand(SetFailedStateCommand.Create(error)); + await client.ProcessCommands(); + client.ExecuteCommand(SetClosedStateCommand.Create()); + break; + case ConnectionState.Failed: + client.ExecuteCommand(SetFailedStateCommand.Create(error)); + break; + default: + client.ExecuteCommand(SetDisconnectedStateCommand.Create(error)); + break; + } + + await client.ProcessCommands(); + } + + public ReinitialiseOnConnectSpecs(ITestOutputHelper output) + : base(output) + { + } + } + + [Trait("spec", "RTN19a2")] + public class ConnectionContinuitySpecs : AblyRealtimeSpecs + { + [Fact] + [Trait("spec", "RTN15c6")] + public async Task OnASuccessfulResume_ShouldKeepTheSerialSequence() + { + var client = await GetClientWithOneUnackedMessage(); + + await Reconnect(client, connectionId: "1"); + + // The same connection is still expecting the serial this message was given. + client.State.Connection.MessageSerial.Should().Be(3); + SentSerials(client).Should().Equal(2L); + } + + [Fact] + [Trait("spec", "RTN24")] + public async Task OnAnUpdate_ShouldKeepTheSerialSequence() + { + var client = await GetClientWithOneUnackedMessage(); + + await Reconnect(client, connectionId: "1", isUpdate: true); + + client.State.Connection.MessageSerial.Should().Be(3); + } + + [Fact] + [Trait("spec", "RTN15g3")] + public async Task OnAFreshConnectionWithoutAnError_ShouldRestartTheSerialSequence() + { + // The RTN15g case, and the one that was broken: connection state was cleared, so + // Ably answers with a new connectionId and no error. Both of the guards this used + // to depend on were false here, which is precisely when a restart is needed. + var client = await GetClientWithOneUnackedMessage(); + + await Reconnect(client, connectionId: "different"); + + SentSerials(client).Should().Equal(0L); + client.State.Connection.MessageSerial.Should().Be(1); + } + + // UTS: realtime/unit/RTN15c7/failed-resume-new-id-0 + [Fact] + [Trait("spec", "RTN15c7")] + public async Task OnAFailedResume_ShouldRestartTheSerialSequence() + { + var client = await GetClientWithOneUnackedMessage(); + + await Reconnect(client, connectionId: "different", error: new ErrorInfo("resume failed", 80008)); + + SentSerials(client).Should().Equal(0L); + client.State.Connection.MessageSerial.Should().Be(1); + } + + [Fact] + public async Task WhenRestartingTheSequence_ShouldNotLeaveStaleEntriesAwaitingAck() + { + // Requeued messages are re-registered for their ACK as they are sent, so the original + // entries must go: they hold serials from the abandoned sequence that a later ACK + // would also match, running the same callback twice. + var client = await GetClientWithOneUnackedMessage(); + + await Reconnect(client, connectionId: "different"); + + client.State.WaitingForAck.Should().HaveCount(1); + client.State.WaitingForAck.Single().Message.MsgSerial.Should().Be(0); + } + + // UTS: realtime/unit/RTN16f/recover-initializes-msgserial-0 + [Fact] + [Trait("spec", "RTN16f")] + public async Task OnASuccessfulRecover_ShouldKeepTheRecoveredSerial() + { + // RTN16f initialises the counter from the recovery key. A recover deliberately adopts + // another connection's sequence, so a successful one must not restart it, even though + // the connectionId is one we have never seen. + var client = GetClientWithFakeTransport(options => options.Recover = + "{\"connectionKey\":\"uniqueKey\",\"msgSerial\":45,\"channelSerials\":{}}"); + // WaitForState returns as soon as the state is set, which happens before + // ConnectionManager.CreateTransport runs - and that is what applies the recovered + // serial. Drain the queue so the precondition is established rather than raced. + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + client.State.Connection.MessageSerial.Should().Be(45); + + await Reconnect(client, connectionId: "recovered"); + + client.State.Connection.MessageSerial.Should().Be(45); + } + + // UTS: realtime/unit/RTN16f/recover-initializes-msgserial-0 + [Fact] + [Trait("spec", "RTN16f")] + [Trait("spec", "RTN15c7")] + public async Task OnAFailedRecover_ShouldRestartTheSerialSequence() + { + // RTN16f: "If the recover fails, the counter should be reset to 0 per RTN15c7." + var client = GetClientWithFakeTransport(options => options.Recover = + "{\"connectionKey\":\"uniqueKey\",\"msgSerial\":45,\"channelSerials\":{}}"); + // WaitForState returns as soon as the state is set, which happens before + // ConnectionManager.CreateTransport runs - and that is what applies the recovered + // serial. Drain the queue so the precondition is established rather than raced. + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + client.State.Connection.MessageSerial.Should().Be(45); + + await Reconnect(client, connectionId: "different", error: new ErrorInfo("unable to recover", 80008)); + + client.State.Connection.MessageSerial.Should().Be(0); + } + + private async Task GetClientWithOneUnackedMessage() + { + var client = await GetConnectedClient(); + + for (var i = 0; i < 3; i++) + { + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"))); + } + + // Acknowledge the first two, so the sequence is deliberately not zero based and a + // restart is distinguishable from a resume. + client.ExecuteCommand(ProcessMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Ack) { MsgSerial = 0, Count = 2 })); + await client.ProcessCommands(); + + client.State.Connection.MessageSerial.Should().Be(3); + client.State.WaitingForAck.Should().HaveCount(1); + client.State.WaitingForAck.Single().Message.MsgSerial.Should().Be(2); + + LastCreatedTransport.SentMessages.Clear(); + return client; + } + + private static async Task Reconnect( + AblyRealtime client, string connectionId, bool isUpdate = false, ErrorInfo error = null) + { + await client.Workflow.ProcessCommand(SetConnectedStateCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = connectionId, + ConnectionDetails = new ConnectionDetails { ConnectionKey = "connectionKey" }, + Error = error, + }, + isUpdate)); + await client.ProcessCommands(); + } + + private IEnumerable SentSerials(AblyRealtime client) => + LastCreatedTransport.SentMessages + .Select(x => x.Original) + .Where(x => x != null && x.Action == ProtocolMessage.MessageAction.Message) + .Select(x => x.MsgSerial) + .ToList(); + + public ConnectionContinuitySpecs(ITestOutputHelper output) + : base(output) + { + } + } + + [Trait("spec", "RTN24")] + public class ConnectedUpdateSpecs : AblyRealtimeSpecs + { + [Fact] + [Trait("spec", "RTN15g3")] + [Trait("spec", "RTL3d")] + public async Task AfterAnRtn15gClear_ShouldReattachAnAttachedChannel() + { + // Gating the reattach on a changed connectionId cannot work here: RTN15g empties + // Connection.Id before the CONNECTING transition, so there is nothing left to + // compare against by the time CONNECTED arrives, and the channel would stay locally + // ATTACHED on a brand new connection with no server-side attachment. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + // Force the RTN15g path: last activity long enough ago that the state is stale. + client.State.Connection.SetConfirmedAlive(DateTimeOffset.UtcNow.AddMinutes(-30)); + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.ProcessCommands(); + client.ExecuteCommand(SetConnectingStateCommand.Create()); + await client.ProcessCommands(); + + // RTN15g should have discarded the connection state. + client.State.Connection.Key.Should().BeEmpty(); + LastCreatedTransport.SentMessages.Clear(); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "brand-new", + ConnectionDetails = new ConnectionDetails { ConnectionKey = "newKey" }, + }); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Attaching); + LastCreatedTransport.SentMessages + .Select(x => x.Original) + .Where(x => x != null) + .Select(x => x.Action) + .Should().Contain(ProtocolMessage.MessageAction.Attach); + } + + [Fact] + [Trait("spec", "RTL3d")] + public async Task OnAnUpdate_ShouldNotReattachAttachedChannels() + { + // RTL3d applies on entering CONNECTED. An RTN24 update arrives on the connection we + // already hold, so reattaching would be a spurious round trip per channel. + var client = await GetConnectedClient(); + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.SetChannelState(ChannelState.Attached); + await client.ProcessCommands(); + + LastCreatedTransport.SentMessages.Clear(); + var states = new List(); + channel.On(x => states.Add(x.Current)); + + // Same connectionId, delivered as an update - the shape RTC8a reauth produces. + await client.Workflow.ProcessCommand(SetConnectedStateCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + ConnectionDetails = new ConnectionDetails { ConnectionKey = "connectionKey" }, + }, + isUpdate: true)); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Attached); + states.Should().NotContain(ChannelState.Attaching); + LastCreatedTransport.SentMessages + .Select(x => x.Original?.Action) + .Should().NotContain(ProtocolMessage.MessageAction.Attach); + } + + public ConnectedUpdateSpecs(ITestOutputHelper output) + : base(output) + { + } + } + + [Trait("spec", "RTN23a")] + public class IdleTimeoutSpecs : AblyRealtimeSpecs + { + private static readonly TimeSpan PromisedMaxIdleInterval = TimeSpan.FromSeconds(15); + private static readonly TimeSpan RequestTimeout = TimeSpan.FromSeconds(10); + + // 15s promised by the server plus the 10s realtimeRequestTimeout. + private static readonly TimeSpan AllowedIdleTime = PromisedMaxIdleInterval + RequestTimeout; + + private readonly Now _now = new Now(); + + [Fact] + [Trait("spec", "TO3l11")] + public async Task ShouldUseTheConfiguredRealtimeRequestTimeout() + { + // TO3l11 makes realtimeRequestTimeout a client option, and RTN23a derives its + // threshold from it, so a caller changing it has to move the idle timeout with it. + var client = await GetConnectedClient(PromisedMaxIdleInterval, TimeSpan.FromSeconds(30)); + + // Past the default 25s window but inside the 45s this client asked for. + _now.Reset(_now.Value.Add(TimeSpan.FromSeconds(40))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))).Should().BeEmpty(); + + _now.Reset(_now.Value.Add(TimeSpan.FromSeconds(10))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().ContainSingle().Which.Should().BeOfType(); + } + + // UTS: realtime/unit/RTN23a/idle-timeout-reconnect-1 + [Fact] + public async Task WhenIdleForLongerThanAllowed_ShouldDisconnect() + { + var client = await GetConnectedClient(PromisedMaxIdleInterval); + + _now.Reset(_now.Value.Add(AllowedIdleTime.Add(TimeSpan.FromSeconds(1)))); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + var disconnect = commands.Should().ContainSingle() + .Which.Should().BeOfType().Subject; + disconnect.Error.Code.Should().Be(ErrorCodes.Disconnected); + disconnect.Error.StatusCode.Should().Be(HttpStatusCode.RequestTimeout); + + // RTN15a - the idle disconnect asks to reconnect at once rather than waiting out + // the disconnected retry timeout. + disconnect.RetryInstantly.Should().BeTrue(); + } + + [Fact] + public async Task WhenIdleForExactlyTheAllowedTime_ShouldNotDisconnect() + { + var client = await GetConnectedClient(PromisedMaxIdleInterval); + + _now.Reset(_now.Value.Add(AllowedIdleTime)); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + commands.Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "CD2h")] + public async Task WhenMaxIdleIntervalIsZero_ShouldNeverDisconnect() + { + // A zero maxIdleInterval means Ably allows arbitrarily long inactivity. + var client = await GetConnectedClient(TimeSpan.Zero); + + _now.Reset(_now.Value.Add(TimeSpan.FromHours(1))); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + commands.Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "CD2h")] + public async Task WhenMaxIdleIntervalIsAbsent_ShouldNeverDisconnect() + { + // Ably declining to send the field is Ably declining to make the promise. + var client = await GetConnectedClient(null); + + _now.Reset(_now.Value.Add(TimeSpan.FromHours(1))); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + commands.Should().BeEmpty(); + } + + [Fact] + public async Task WhenNotConnected_ShouldNotDisconnect() + { + // ConfirmedAliveAt can still be carrying a timestamp from a previous transport, so + // acting on it outside Connected risks tearing down a healthy new one. + var client = await GetConnectedClient(PromisedMaxIdleInterval); + + await client.Workflow.ProcessCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + _now.Reset(_now.Value.Add(TimeSpan.FromHours(1))); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + commands.Should().BeEmpty(); + } + + // UTS: realtime/unit/RTN23a/any-message-resets-timer-3 + [Fact] + public async Task AnyReceivedMessage_NotOnlyHeartbeat_ShouldResetTheIdleTimer() + { + var client = await GetConnectedClient(PromisedMaxIdleInterval); + + // Most of the way through the allowed window, then a message that is deliberately + // not a Heartbeat - RTN23a counts any received message as a sign of activity. + _now.Reset(_now.Value.Add(TimeSpan.FromSeconds(20))); + client.ExecuteCommand(ProcessMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Attached) { Channel = "test" })); + await client.ProcessCommands(); + + // Another 20s on from that message is still inside the 25s window. + _now.Reset(_now.Value.Add(TimeSpan.FromSeconds(20))); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + commands.Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "RTN23a")] + [Trait("spec", "CD2h")] + public async Task WhenANewTransportOmitsMaxIdleInterval_ShouldStopMeasuring() + { + // RTN23a measures against the interval "sent in the connectionDetails of the most + // recent CONNECTED message received on that transport", so a CONNECTED starting a new + // transport must not inherit the previous threshold. An omitted field is Ably + // declining to promise anything, which CD2h treats as arbitrarily long inactivity. + var client = await GetConnectedClient(PromisedMaxIdleInterval); + + // A real reconnect, so the CONNECTED lands on a new transport rather than becoming + // an RTN24 update on the one we already hold. + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.ProcessCommands(); + client.ExecuteCommand(SetConnectingStateCommand.Create()); + await client.ProcessCommands(); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "2", + ConnectionDetails = new ConnectionDetails { ConnectionKey = "anotherKey" }, + }); + + await client.WaitForState(ConnectionState.Connected); + await client.ProcessCommands(); + + client.State.Connection.MaxIdleInterval.Should().BeNull(); + + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().BeEmpty(); + } + + [Fact] + [Trait("spec", "RTN23a")] + [Trait("spec", "RTN24")] + public async Task WhenAnUpdateOmitsMaxIdleInterval_ShouldKeepMeasuring() + { + // An RTN24 update arrives on the transport we already hold, which is still bound by + // whatever it promised, so an omitted field is not a withdrawal. ably-js keeps the + // previous value here with its if (maxPromisedIdle) guard, nulling the field per + // transport instead. Nulling it here would disarm RTN23a on a live transport. + var client = await GetConnectedClient(PromisedMaxIdleInterval); + var transportBefore = LastCreatedTransport; + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + ConnectionDetails = new ConnectionDetails { ConnectionKey = "connectionKey" }, + }); + + await client.ProcessCommands(); + + // Same transport - nothing reconnected. + LastCreatedTransport.Should().BeSameAs(transportBefore); + client.State.Connection.MaxIdleInterval.Should().Be(PromisedMaxIdleInterval); + + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().ContainSingle().Which.Should().BeOfType(); + } + + [Fact] + [Trait("spec", "RTN23a")] + public async Task WhenTheThresholdCannotBeRepresented_ShouldNotThrow() + { + // The wire value is an unbounded integer of milliseconds, so the sum can overflow. A + // throw would be logged and dropped by the command loop, and every later tick would + // throw too - killing idle detection silently for the life of the connection. + var client = await GetConnectedClient(TimeSpan.MaxValue - TimeSpan.FromSeconds(1)); + + _now.Reset(_now.Value.Add(TimeSpan.FromDays(1))); + + var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); + + commands.Should().BeEmpty(); + client.Connection.State.Should().Be(ConnectionState.Connected); + } + + [Theory] + [InlineData("Heartbeats", false)] + [InlineData("heartbeats", false)] + [InlineData("heartbeats", "no")] + [InlineData("heartbeats", 0)] + [InlineData("heartbeats", null)] + + // A differently-cased key with a true value still leaves nothing on the wire asking for + // protocol heartbeats: Merge drops our correctly-cased entry, and only that spelling is + // the param Ably reads. + [InlineData("Heartbeats", true)] + [InlineData("HEARTBEATS", "true")] + [Trait("spec", "RTN23b")] + public async Task WhenTheCallerDoesNotAskForProtocolHeartbeats_ShouldStandDown( + string key, object value) + { + // Two halves, both needed. Merge drops our own heartbeats param on a + // case-insensitive key match, so "Heartbeats" reaches the wire in place of ours. And + // RTN23b guarantees protocol heartbeats only for the literal "true": "if it is false + // or unspecified, the server is permitted to use any transport-level mechanism" - + // which this library cannot observe. + var client = GetClientWithFakeTransport(opts => + { + opts.NowFunc = _now.ValueFn; + opts.RealtimeRequestTimeout = RequestTimeout; + opts.HeartbeatMonitorDelay = (int)TimeSpan.FromMinutes(10).TotalMilliseconds; + opts.TransportParams = new Dictionary { { key, value } }; + }); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + ConnectionDetails = new ConnectionDetails + { + ConnectionKey = "connectionKey", + MaxIdleInterval = PromisedMaxIdleInterval, + }, + }); + + await client.WaitForState(ConnectionState.Connected); + + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().BeEmpty(); + } + + [Theory] + [InlineData("heartbeats", true)] + [InlineData("heartbeats", "true")] + [InlineData("heartbeats", "TRUE")] + [Trait("spec", "RTN23b")] + public async Task WhenTheCallerAsksForProtocolHeartbeats_ShouldStayArmed(string key, object value) + { + // Standing down is the guard's default outcome for any caller value, so the armed + // branch is the one an edit could lose silently. This pins it. + var client = GetClientWithFakeTransport(opts => + { + opts.NowFunc = _now.ValueFn; + opts.RealtimeRequestTimeout = RequestTimeout; + opts.HeartbeatMonitorDelay = (int)TimeSpan.FromMinutes(10).TotalMilliseconds; + opts.TransportParams = new Dictionary { { key, value } }; + }); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + ConnectionDetails = new ConnectionDetails + { + ConnectionKey = "connectionKey", + MaxIdleInterval = PromisedMaxIdleInterval, + }, + }); + + await client.WaitForState(ConnectionState.Connected); + + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().ContainSingle().Which.Should().BeOfType(); + } + + [Fact] + [Trait("spec", "RTN21")] + [Trait("spec", "RTN15b")] + public async Task WhenAConnectedCarriesNoConnectionDetails_ShouldKeepTheConnectionKey() + { + // RTN21 scopes the override to "the attributes within ConnectionDetails", so a + // CONNECTED carrying none overrides nothing and the key has to survive. + var client = await GetConnectedClient(PromisedMaxIdleInterval); + client.State.Connection.Key.Should().Be("connectionKey"); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + }); + + await client.ProcessCommands(); + + client.State.Connection.Key.Should().Be("connectionKey"); + } + + private async Task GetConnectedClient( + TimeSpan? maxIdleInterval, TimeSpan? requestTimeout = null) + { + var client = GetClientWithFakeTransport(opts => + { + opts.NowFunc = _now.ValueFn; + opts.RealtimeRequestTimeout = requestTimeout ?? RequestTimeout; + + // These tests drive the monitor tick explicitly and assert on what that tick + // decides, so the workflow's own once-a-second loop is pushed out of reach; on a + // loaded run it would fire first and the explicit tick would find the disconnect + // already requested. + opts.HeartbeatMonitorDelay = (int)TimeSpan.FromMinutes(10).TotalMilliseconds; + }); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + ConnectionDetails = new ConnectionDetails + { + ConnectionKey = "connectionKey", + MaxIdleInterval = maxIdleInterval, + }, + }); + + await client.WaitForState(ConnectionState.Connected); + return client; + } + + public IdleTimeoutSpecs(ITestOutputHelper output) + : base(output) + { + } + } + public RealtimeWorkflowSpecs(ITestOutputHelper output) : base(output) { From d1f3c15dedabe40701f59e32a85cd3fc48034e4e Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Wed, 2 Sep 2026 15:20:15 +0100 Subject: [PATCH 07/10] Always attempt a resume, and let the server decide continuity RTN14h - implemented. Replaces RTN15g as of specification 6.1.0. The client discarded its connection state once connectionStateTtl had passed and reconnected fresh, throwing away a resume the server would still have honoured. DF1a settles the scope - the ttl is no longer used to decide whether to resume at all - so the gate is general rather than SUSPENDED-only, which is how ably-js reads it too. RTN27c - fixed. DISCONNECTED is a state where "if the library was previously connected, the next connect attempt will be an RTN15b resume attempt". Clearing the key on every failed attempt made that false from the second attempt onwards. RTN8d, RTN9d - fixed twice over. Both list only CLOSED, CLOSING and FAILED, so SUSPENDED must keep the key and id; it cleared them. And all three of the states they do name cleared after SetState, which is what emits the state change - inline, with no SynchronizationContext installed - so the application was told it had reached a terminal state while Connection.Key still read as a resumable key. The clear now happens before the transition, which also stops it depending on a finally. Only a listener reading during the transition could observe this, so it was inherited rather than introduced here. RTN15g1, RTN15g2, RTN15g3 - deleted at 6.1.0. HasConnectionStateTtlPassed and its tests go with them. The reattach RTN15g3 asked for is already unconditional under RTL3d. RTL4j, RTL4j1, RTL4j2 - deleted at 6.1.0; SDKs need not set ATTACH_RESUME. Safe only because RTL4c1 already sends channelSerial on ATTACH and RTL15b2 keeps it across a suspend, so the reattach still carries a continuity signal. The Flag constant stays, per TR3f. The now-dead clearConnectionKey parameter goes from SetDisconnectedStateCommand and SetSuspendedStateCommand. No caller sets it, and leaving it would let the violation back in unnoticed. SetConnectingStateCommand keeps its own, still used by ConnectionClosingState for RTN11b/RTN11d, where a clean connection is the intent. Two clauses the 6.1.0 edit left describing the old model. They are handled differently, and both deliberately: - RTN27d is not followed. It still calls SUSPENDED a state whose "next connect attempt is a clean connection (not a resume attempt)", which RTN14h, RTN8d, RTN9d and DF1a now all contradict; the commit that made those changes does not touch RTN27 at all. ably-js retains the key in SUSPENDED too, citing RTN8d/RTN9d and RTN14h for it. - RTN16g2 is followed. It says createRecoveryKey returns null in SUSPENDED, which sits oddly beside RTN9d now that a key exists there, but it is what the published spec says and CreateRecoveryKey still complies. ably-js returns a key instead, citing an RTN16g3 that "replaces RTN16g2 as of 6.1.0" and appears nowhere in the published spec. Both worth raising upstream rather than guessing. Co-Authored-By: Claude Opus 5 --- .../Realtime/RealtimeChannel.cs | 47 ++--- .../Realtime/Workflows/RealtimeCommands.cs | 24 +-- .../Realtime/Workflows/RealtimeState.cs | 37 ---- .../Realtime/Workflows/RealtimeWorkflow.cs | 71 ++----- .../IO.Ably.Tests.Shared.projitems | 1 - .../Realtime/ChannelSandboxSpecs.cs | 56 ------ .../Realtime/ChannelsSpecs.cs | 14 -- .../Realtime/ConnectionSandBoxSpecs.cs | 66 +++---- .../Realtime/ConnectionStateFreshnessSpecs.cs | 142 -------------- .../Realtime/RealtimeWorkflowSpecs.cs | 179 ++++++++++++++---- 10 files changed, 203 insertions(+), 434 deletions(-) delete mode 100644 src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs diff --git a/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs b/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs index a2b717bab..3aeb69e7d 100644 --- a/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs +++ b/src/IO.Ably.Shared/Realtime/RealtimeChannel.cs @@ -28,12 +28,6 @@ internal class RealtimeChannel : EventEmitter, private readonly PushChannel _pushChannel; private int _retryCount = 0; - /// - /// True when the channel moves to the @ATTACHED@ state, and False - /// when the channel moves to the @DETACHING@ or @FAILED@ states. - /// - internal bool AttachResume { get; set; } - private int _decodeRecoveryInProgress; // We use interlocked exchange because it is a thread safe way to read a variable @@ -160,33 +154,22 @@ internal void ConnectionStateChanged(ConnectionStateChange connectionStateChange } /* - * (RTL3d, RTN19b, RTN15c6, RTN15c7, RTN15g3) On entering CONNECTED, every - * channel that was attached or pending needs its ATTACH - or its DETACH - sent - * again, because the previous transport will never answer. + * On entering CONNECTED, every channel that was attached or pending needs its + * ATTACH - or its DETACH - sent again; the previous transport will never answer. * * The two branches have different owners. RTL3d names ATTACHING, ATTACHED and - * SUSPENDED and asks for an RTL4c attach. DETACHING is not in that list and is - * not a state transition at all - it is RTN19b, "if there are any pending - * channels i.e. in the ATTACHING or DETACHING state, the respective ATTACH or - * DETACH message should be resent". ably-js splits it the same way, with - * checkPendingState handling both pending operations and notifyState the - * reattach. + * SUSPENDED, and asks for an RTL4c attach. DETACHING is RTN19b instead, which + * requires the respective ATTACH or DETACH of any pending channel to be resent. + * ably-js splits it the same way, between checkPendingState and notifyState. * * RTL3d1 requires all of this to be applied before CONNECTED reaches external - * listeners, and is why it lives here rather than in HandleConnectedCommand: - * Connection.NotifyUpdate runs the internal handlers, which is what calls this, - * before handing the emit to NotifyExternalClients. - * - * Unconditional, deliberately. This used to be gated on the connectionId having - * changed, which silently skipped the RTN15g3 reattach: RTN15g empties - * Connection.Id *before* the CONNECTING transition, so by the time CONNECTED - * arrived the id being compared against had already gone and the channel - * concluded nothing had changed. The channel stayed locally ATTACHED on a brand - * new connection with no server-side attachment - permanently silent. + * listeners, which is why it lives here: Connection.NotifyUpdate runs the + * internal handlers, this among them, before the emit. * - * Whether the connection was resumed belongs inside the ATTACH, in channelSerial - * and the ATTACH_RESUME flag, rather than in a decision about whether to send - * one at all. ably-js reattaches unconditionally for the same reason. + * Unconditional, deliberately. Whether the connection was resumed belongs + * inside the ATTACH, in the channelSerial RTL4c1 carries, rather than in a + * decision about whether to send one at all. ably-js reattaches unconditionally + * for the same reason. */ switch (State) { @@ -371,11 +354,6 @@ ProtocolMessage CreateAttachMessage() message.SetModesAsFlags(Options.Modes); } - if (AttachResume) - { - message.SetFlag(ProtocolMessage.Flag.AttachResume); - } - return message; } } @@ -748,11 +726,9 @@ private void HandleStateChange(ChannelState state, ErrorInfo error, ProtocolMess break; case ChannelState.Detaching: AttachedAwaiter.Fail(new ErrorInfo("Channel transitioned to detaching", ErrorCodes.InternalError)); - AttachResume = false; break; case ChannelState.Attached: _retryCount = 0; - AttachResume = true; break; case ChannelState.Detached: /* RTL13a check for unexpected detach */ @@ -792,7 +768,6 @@ an attempt to reattach the channel should be made immediately */ break; case ChannelState.Failed: _retryCount = 0; - AttachResume = false; AttachedAwaiter.Fail(error); DetachedAwaiter.Fail(error); Presence.ChannelDetachedOrFailed(error); diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs index dc144265e..22bd7ba23 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs @@ -204,13 +204,12 @@ protected override string ExplainData() internal class SetDisconnectedStateCommand : RealtimeCommand { - private SetDisconnectedStateCommand(ErrorInfo error, bool retryInstantly, bool skipAttach, Exception exception, bool clearConnectionKey) + private SetDisconnectedStateCommand(ErrorInfo error, bool retryInstantly, bool skipAttach, Exception exception) { Error = error; RetryInstantly = retryInstantly; SkipAttach = skipAttach; Exception = exception; - ClearConnectionKey = clearConnectionKey; } public ErrorInfo Error { get; } @@ -221,45 +220,36 @@ private SetDisconnectedStateCommand(ErrorInfo error, bool retryInstantly, bool s public Exception Exception { get; } - public bool ClearConnectionKey { get; } - protected override string ExplainData() { return $"RetryInstantly: {RetryInstantly}" + "SkipAttach: " + SkipAttach + ((Error != null) ? " Error: " + Error : string.Empty) + - ((Exception != null) ? " Exception: " + Exception.Message : string.Empty) + - " ClearConnectionKey: " + ClearConnectionKey; + ((Exception != null) ? " Exception: " + Exception.Message : string.Empty); } public static SetDisconnectedStateCommand Create( ErrorInfo error, bool retryInstantly = false, bool skipAttach = false, - Exception exception = null, - bool clearConnectionKey = false) - => new SetDisconnectedStateCommand(error, retryInstantly, skipAttach, exception, clearConnectionKey); + Exception exception = null) + => new SetDisconnectedStateCommand(error, retryInstantly, skipAttach, exception); } internal class SetSuspendedStateCommand : RealtimeCommand { - private SetSuspendedStateCommand(ErrorInfo error, bool clearConnectionKey) + private SetSuspendedStateCommand(ErrorInfo error) { Error = error; - ClearConnectionKey = clearConnectionKey; } public ErrorInfo Error { get; } - public bool ClearConnectionKey { get; } - - public static SetSuspendedStateCommand Create(ErrorInfo error, bool clearConnectionKey = false) => new SetSuspendedStateCommand(error, clearConnectionKey); + public static SetSuspendedStateCommand Create(ErrorInfo error) => new SetSuspendedStateCommand(error); protected override string ExplainData() { - var message = (Error != null) ? " Error: " + Error : string.Empty; - message += " ClearConnectionKey:" + ClearConnectionKey; - return message; + return (Error != null) ? " Error: " + Error : string.Empty; } } diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs index 78e881931..85de01981 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs @@ -80,43 +80,6 @@ public ConnectionStateChange UpdateState(ConnectionStateBase state, ILogger logg return new ConnectionStateChange(connectionEvent, oldState, newState, state.RetryIn, ErrorReason); } - public bool HasConnectionStateTtlPassed(Func now) - { - if (ConfirmedAliveAt.HasValue == false) - { - // Nothing has ever been received on this client, so there is no connection - // state to consider stale. - return false; - } - - // RTN15g2 - the window is connectionStateTtl plus maxIdleInterval, measured from - // the last known sign of activity from Ably rather than from when we left the - // Connected state. A device that slept may only have left Connected moments ago - // having last actually heard from Ably hours earlier. - // Clamped at zero rather than coalesced, because nothing between the wire and here - // validates the sign: TimeSpanJsonConverter will hand back a negative TimeSpan for a - // negative number and Update assigns it as-is. A negative value would make the - // subtraction below throw OverflowException, which is precisely the failure this - // method was rewritten to remove - the throw escapes HandleSetStateCommand's - // AblyException-only catch, gets logged and dropped by the command loop, and leaves - // the client wedged in DISCONNECTED with no transport and no retry. The RTN23a - // monitor already treats a non-positive interval as no promise at all. - var maxIdleInterval = MaxIdleInterval > TimeSpan.Zero ? MaxIdleInterval.Value : TimeSpan.Zero; - - if (ConnectionStateTtl >= TimeSpan.MaxValue - maxIdleInterval) - { - // The window cannot be represented, so it can never elapse. - return false; - } - - // Deliberately a subtraction rather than ConfirmedAliveAt + window. Adding to a - // DateTimeOffset throws ArgumentOutOfRangeException once the result runs past - // DateTimeOffset.MaxValue, and that exception escaped into the command loop where - // it was logged and dropped - silently abandoning whichever state transition was - // in progress. Comparing two durations cannot fail that way. - return now() - ConfirmedAliveAt.Value > ConnectionStateTtl + maxIdleInterval; - } - public void Update(ConnectionInfo info, bool isUpdate) { // Guarded differently on purpose. connectionId is a top-level field and always diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index 73ef3c2c8..0daf40084 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -476,9 +476,7 @@ private async Task ProcessCommandInner(RealtimeCommand command) } catch (AblyException e) { - return SetDisconnectedStateCommand.Create( - e.ErrorInfo, - clearConnectionKey: true) + return SetDisconnectedStateCommand.Create(e.ErrorInfo) .TriggeredBy(cmd); } } @@ -506,9 +504,7 @@ async Task AttemptANewConnection() case HandleConnectingDisconnectedCommand cmd: // Suspending is decided in the SetDisconnectedStateCommand handler, for every path. - return SetDisconnectedStateCommand.Create( - cmd.Error ?? ErrorInfo.ReasonDisconnected, - clearConnectionKey: true) + return SetDisconnectedStateCommand.Create(cmd.Error ?? ErrorInfo.ReasonDisconnected) .TriggeredBy(cmd); case HandleConnectingErrorCommand cmd: @@ -522,9 +518,7 @@ async Task AttemptANewConnection() if (error.IsRetryableStatusCode()) { - return SetDisconnectedStateCommand.Create( - error, - clearConnectionKey: true) + return SetDisconnectedStateCommand.Create(error) .TriggeredBy(cmd); } else @@ -764,9 +758,9 @@ private void HandleConnectedCommand(SetConnectedStateCommand cmd) // RTN16f initialises it from the recovery key, which carries no connectionId, so // success is judged by the absence of an error. // - // Broader than testing for an error on the message, which a connection cleared per - // RTN15g would fail: it reconnects fresh, and Ably answers with a new connectionId and - // no error - exactly the case that most needs a new sequence. + // Broader than testing for an error on the message: a resume the server refuses is + // answered with a new connectionId and, often, no error at all - exactly the case that + // most needs a new sequence. // // Must be evaluated before Update below, which overwrites the id being compared, and // before Options.Recover is cleared for RTN16k. @@ -792,7 +786,7 @@ private void HandleConnectedCommand(SetConnectedStateCommand cmd) Client.Options.Recover = null; // RTN16k, explicitly setting null so it won't be used for subsequent connection requests - // RTN15c7, RTN15g3, RTN11d - a connection that is not a continuation of the one we held + // RTN15c7, RTN11d - a connection that is not a continuation of the one we held // restarts the message serial sequence at zero. if (connectionContinues == false) { @@ -882,13 +876,6 @@ private async Task HandleSetStateCommand(RealtimeCommand comman State.Connection.ClearKey(); } - // RTN15g - If a client has been disconnected for longer - // than the connectionStateTtl, it should not attempt to resume. - if (State.Connection.HasConnectionStateTtlPassed(Now)) - { - State.Connection.ClearKeyAndId(); - } - var defaultRealtimeHost = Client.Options.FullRealtimeHost(); // RTN17 - every attempt considers a fallback, including the timer driven @@ -970,16 +957,12 @@ private async Task HandleSetStateCommand(RealtimeCommand comman // about and a throwing transition cannot strand the messages uncalled. // ably-js orders it the same way: enactStateChange then failQueuedMessages. // - // RTN7e - the queued messages are failed with "an error representing the - // reason for the state change", taken off the state object so it is this - // transition's reason even if SetState early-returns. In the finally, after - // the transition, so a publisher's callback sees the state it is being told - // about and a throwing transition cannot strand the messages uncalled. - // ably-js orders it the same way: enactStateChange then failQueuedMessages. - // - // RTN8d and RTN9d share that finally: the connection has entered the state - // by the time SetState rethrows, so a throw must not leave it reporting a - // terminal state while still holding a resumable key and a live transport. + // RTN8d, RTN9d - the key and id go the other way round, before the + // transition, because SetState emits the state change and with no + // SynchronizationContext that emit is inline. Nothing between here and the + // emit reads either field. + State.Connection.ClearKeyAndId(); // RTN8d, RTN9d + try { SetState(failedState); @@ -987,7 +970,6 @@ private async Task HandleSetStateCommand(RealtimeCommand comman finally { ClearAckQueueAndFailMessages(failedState.Error); - State.Connection.ClearKeyAndId(); // RTN8d, RTN9d ConnectionManager.DestroyTransport(); } @@ -1018,17 +1000,10 @@ ErrorInfo TransformIfTokenErrorAndNotRetryable() // diverting would emit SUSPENDED and then immediately CONNECTING. if (cmd.SkipAttach == false && State.ShouldSuspend(Now)) { - return SetSuspendedStateCommand.Create( - cmd.Error ?? ErrorInfo.ReasonSuspended, - clearConnectionKey: true) + return SetSuspendedStateCommand.Create(cmd.Error ?? ErrorInfo.ReasonSuspended) .TriggeredBy(command); } - if (cmd.ClearConnectionKey) - { - State.Connection.ClearKey(); - } - bool? connectivityAnswer = null; var retryInstantly = await CheckInstantRetryFlag(); @@ -1140,8 +1115,8 @@ async Task CheckInstantRetryFlag() var connectedTransport = transport?.State == TransportState.Connected; var closingState = new ConnectionClosingState(ConnectionManager, connectedTransport, Logger); + State.Connection.ClearKeyAndId(); // RTN8d, RTN9d - before the emit SetState(closingState); - State.Connection.ClearKeyAndId(); // RTN8d, RTN9d if (connectedTransport) { @@ -1154,11 +1129,6 @@ async Task CheckInstantRetryFlag() case SetSuspendedStateCommand cmd: - if (cmd.ClearConnectionKey) - { - State.Connection.ClearKey(); - } - var suspendedState = new ConnectionSuspendedState(ConnectionManager, cmd.Error, Logger); // RTN7e and the teardown - see the note on the FAILED case. @@ -1170,12 +1140,6 @@ async Task CheckInstantRetryFlag() { ClearAckQueueAndFailMessages(suspendedState.Error); - // Deliberately NOT RTN8d/RTN9d, which name only CLOSED, CLOSING and - // FAILED. Clearing here is this library's pre-6.1.0 behaviour, kept - // because it is coupled to the connectionStateTtl freshness check that - // also predates 6.1.0. - State.Connection.ClearKeyAndId(); - // Needed here as well as in the DISCONNECTED handler, which diverts to // this case before reaching its own DestroyTransport. A surviving // transport keeps its listener for up to suspendedRetryTimeout. @@ -1191,7 +1155,9 @@ async Task CheckInstantRetryFlag() Exception = cmd.Exception, }; - // RTN7e and the teardown - see the note on the FAILED case. + // RTN7e, RTN8d, RTN9d and the teardown - see the note on the FAILED case. + State.Connection.ClearKeyAndId(); // RTN8d, RTN9d - before the emit + try { SetState(closedState); @@ -1199,7 +1165,6 @@ async Task CheckInstantRetryFlag() finally { ClearAckQueueAndFailMessages(closedState.Error); - State.Connection.ClearKeyAndId(); // RTN8d, RTN9d ConnectionManager.DestroyTransport(); } diff --git a/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems b/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems index ebc14b13b..2eace096f 100644 --- a/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems +++ b/src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems @@ -89,7 +89,6 @@ - diff --git a/src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs index 5aa8c564b..670ad3df0 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs @@ -116,62 +116,6 @@ public async Task WhenAttachingAChannelWithInsufficientPermissions_ShouldSetItTo result.Error.StatusCode.Should().Be(HttpStatusCode.Unauthorized); } - [Theory] - [ProtocolData] - [Trait("spec", "RTL4j")] - public async Task WhenChannelIsAlreadyAttached_AndReAttachIsForcedByChangingChannelOptions_ShouldPassAttachResumeFlagInAttachMessage(Protocol protocol) - { - var sentMessages = new List(); - var client = await GetRealtimeClient(protocol, (options, _) => - { - var optionsTransportFactory = new TestTransportFactory - { - OnMessageSent = sentMessages.Add, - }; - options.TransportFactory = optionsTransportFactory; - }); - - var channel = client.Channels.Get("Test"); - await channel.AttachAsync(); - - var result = await channel.SetOptionsAsync(new ChannelOptions().WithModes(ChannelMode.Publish)); - - result.IsSuccess.Should().BeTrue(); - var attachMessages = sentMessages.Where(x => x.Action == ProtocolMessage.MessageAction.Attach).ToList(); - attachMessages.Should().HaveCount(2); - attachMessages.First().Flags.Should().BeNull(); - attachMessages.Last().HasFlag(ProtocolMessage.Flag.AttachResume).Should().BeTrue(); - } - - [Theory] - [ProtocolData] - [Trait("spec", "RTL4j2")] - public async Task TestAttachResume_And_RewindParam(Protocol protocol) - { - var client = await GetRealtimeClient(protocol); - var client1 = await GetRealtimeClient(protocol, (opts, _) => opts.AutoConnect = false); - var client2 = await GetRealtimeClient(protocol, (opts, _) => opts.AutoConnect = false); - - var channel = client.Channels.Get("Test"); - await channel.PublishAsync("test", "test"); - - var channelWithAttachResume = client1.Channels.Get("Test", new ChannelOptions().WithRewind(1)) as RealtimeChannel; - channelWithAttachResume.AttachResume = true; - var channel1Messages = new List(); - channelWithAttachResume.Subscribe(channel1Messages.Add); - await channelWithAttachResume.WaitForAttachedState(); - var channelWithoutAttachResume = client2.Channels.Get("Test", new ChannelOptions().WithRewind(1)); - - var channel2Messages = new List(); - channelWithoutAttachResume.Subscribe(channel2Messages.Add); - await channelWithoutAttachResume.WaitForAttachedState(); - - await Task.Delay(2000); - - channel2Messages.Should().HaveCount(1); - channel1Messages.Should().BeEmpty(); - } - [Theory] [ProtocolData] [Trait("spec", "RTL4k1")] diff --git a/src/IO.Ably.Tests.Shared/Realtime/ChannelsSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ChannelsSpecs.cs index 722feb98b..bf5fae462 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ChannelsSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ChannelsSpecs.cs @@ -329,20 +329,6 @@ public async Task WithExistingChannel_Get_WithNewChannelOptionsButDifferentModes ex.Message.Should().Contain("SetOptions"); } - [Theory] - [InlineData(ChannelState.Initialized, false)] - [InlineData(ChannelState.Attached, true)] - [InlineData(ChannelState.Detaching, false)] - [InlineData(ChannelState.Failed, false)] - [Trait("spec", "RTL4j1")] - public async Task WhenChannelMovesToState_AttachResumeShouldHaveCorrectValue(ChannelState state, bool expectedAttachResumeValue) - { - var client = GetRealtimeClient(); - var channel = client.Channels.Get("Test") as RealtimeChannel; - channel.SetChannelState(state); - channel.AttachResume.Should().Be(expectedAttachResumeValue); - } - public ChannelsSpecs(ITestOutputHelper output) : base(output) { diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs index cbbf8f4c4..633475e84 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -958,14 +958,19 @@ public async Task WhenDisconnectedMessageContainsTokenError_IfTokenRenewFails_Sh stateChanges[2].Reason.Code.Should().Be(ErrorCodes.ClientAuthProviderRequestFailed); } + // UTS: realtime/proxy/RTN14h/resume-after-ttl-expiry-0 [Theory] [ProtocolData] - [Trait("spec", "RTN15g")] - [Trait("spec", "RTN15g1")] - [Trait("spec", "RTN15g2")] - [Trait("spec", "RTN15g3")] - public async Task WhenDisconnectedPastTTL_ShouldNotResume_ShouldClearConnectionStateAndAttemptNewConnection(Protocol protocol) + [Trait("spec", "RTN14h")] + [Trait("spec", "RTN8d")] + [Trait("spec", "RTN9d")] + [Trait("spec", "RTN15c6")] + public async Task WhenDisconnectedPastTTL_ShouldStillResume_AndReattachChannels(Protocol protocol) { + // RTN14h, which replaces RTN15g as of specification 6.1.0 - the client always attempts + // the resume and lets the server decide whether continuity survives. Against a live + // endpoint the reconnect comes back with the connectionId we were holding, which is only + // possible because the resume param went out (RTN15b1). var client = await GetRealtimeClient(protocol, (options, _) => { options.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(1000); @@ -974,19 +979,11 @@ public async Task WhenDisconnectedPastTTL_ShouldNotResume_ShouldClearConnectionS await client.WaitForState(ConnectionState.Connected); - // No ttl override: winding ConfirmedAliveAt back below is enough to force the stale - // path against the real 120s ttl plus the server's 15s maxIdleInterval. Shortening the - // ttl as well made this test fail, because the RTN14e clamp in - // ConnectionDisconnectedState.StartTimer then clamps the retry backoff down to the - // remaining ttl - about a second - so the reconnect happened far sooner than the - // DisconnectedRetryTimeout this test asserts against. A 1s ttl would also trip - // ShouldSuspend on the retry and land in SUSPENDED instead of CONNECTED. string initialConnectionId = client.Connection.Id; + string initialConnectionKey = client.Connection.Key; - var aliveAt1 = client.Connection.ConfirmedAliveAt; - var aliveAt2 = aliveAt1; - - // RTN15g3 ATTACHED, ATTACHING, or SUSPENDED must be automatically reattached + // RTL3d - channels that were ATTACHED, ATTACHING or SUSPENDED are reattached on + // entering CONNECTED regardless of whether the resume succeeded. var channels = new List { client.Channels.Get("attached".AddRandomSuffix()) as RealtimeChannel, @@ -1002,34 +999,21 @@ public async Task WhenDisconnectedPastTTL_ShouldNotResume_ShouldClearConnectionS channels[1].State.Should().Be(ChannelState.Initialized); // set attaching later channels[2].State.Should().Be(ChannelState.Suspended); - DateTime disconnectedAt = DateTime.MinValue; - DateTime reconnectedAt = DateTime.MinValue; string newConnectionId = string.Empty; await WaitFor(60000, done => { - client.Connection.Once(ConnectionEvent.Disconnected, change2 => + client.Connection.Once(ConnectionEvent.Disconnected, _ => { - disconnectedAt = DateTime.UtcNow; - - // RTN15g2 - the staleness window is connectionStateTtl plus the real - // maxIdleInterval the server sent, which is 15s against a live endpoint. A - // shortened ttl alone is therefore not enough to push us outside it before the - // retry fires, so wind the last known activity back instead. Deliberately done - // here rather than while still Connected: from Connected this would trip the - // RTN23a idle monitor and race the disconnect this test is arranging. - client.State.Connection.SetConfirmedAlive(DateTimeOffset.UtcNow.AddMinutes(-30)); + // RTN8d, RTN9d - DISCONNECTED is not a terminal state, so both survive. + client.Connection.Id.Should().Be(initialConnectionId); + client.Connection.Key.Should().Be(initialConnectionKey); channels[1].Attach(); - client.Connection.Once(ConnectionEvent.Connecting, change3 => + client.Connection.Once(ConnectionEvent.Connected, _ => { - reconnectedAt = DateTime.UtcNow; - client.Connection.Once(ConnectionEvent.Connected, change4 => - { - newConnectionId = client.Connection.Id; - aliveAt2 = client.Connection.ConfirmedAliveAt; - done(); - }); + newConnectionId = client.Connection.Id; + done(); }); }); @@ -1037,14 +1021,10 @@ await WaitFor(60000, done => client.Workflow.QueueCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); }); - var reconnectedInTime = reconnectedAt - disconnectedAt; - - var (lowerBound, _) = ReconnectionStrategyTest.Bounds(1, 5000); - reconnectedInTime.TotalMilliseconds.Should().BeGreaterThan(lowerBound); - + // RTN15c6 - the server still held the connection, so the resume succeeded and the + // connectionId comes back unchanged. initialConnectionId.Should().NotBeNullOrEmpty(); - initialConnectionId.Should().NotBe(newConnectionId); - aliveAt1.Value.Should().BeBefore(aliveAt2.Value); + newConnectionId.Should().Be(initialConnectionId); await channels[0].WaitForAttachedState(); await channels[1].WaitForAttachedState(); diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs deleted file mode 100644 index bb1f6c84e..000000000 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateFreshnessSpecs.cs +++ /dev/null @@ -1,142 +0,0 @@ -using System; -using FluentAssertions; -using IO.Ably.Realtime.Workflow; -using Xunit; -using Xunit.Abstractions; - -namespace IO.Ably.Tests.Realtime -{ - /// - /// Covers the check that decides whether locally held connection state is too old to resume - /// with - RTN15g and, since it must include the maxIdleInterval, RTN15g2. - /// - [Trait("spec", "RTN15g")] - [Trait("spec", "RTN15g2")] - public class ConnectionStateFreshnessSpecs : AblySpecs - { - private static readonly TimeSpan Ttl = TimeSpan.FromSeconds(120); - private static readonly TimeSpan MaxIdleInterval = TimeSpan.FromSeconds(15); - - private readonly Now _now = new Now(); - private readonly RealtimeState _state = new RealtimeState(); - - [Fact] - public void WhenNothingHasEverBeenReceived_ShouldNotBeStale() - { - // No ConfirmedAliveAt means there is no connection state to consider discarding. - Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: null); - - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); - } - - [Fact] - public void WhenWithinTheTtl_ShouldNotBeStale() - { - Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: _now.Value); - - Advance(TimeSpan.FromSeconds(119)); - - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); - } - - [Fact] - public void WhenPastTheTtlButWithinTheMaxIdleInterval_ShouldNotBeStale() - { - // The point of RTN15g2. At 130s we are past the 120s ttl, but the server may have been - // silent for up to maxIdleInterval before we would have noticed, so the real window is - // 135s and the state is still resumable. - Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: _now.Value); - - Advance(TimeSpan.FromSeconds(130)); - - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); - } - - [Fact] - public void WhenPastTheTtlPlusTheMaxIdleInterval_ShouldBeStale() - { - Connection(ttl: Ttl, maxIdleInterval: MaxIdleInterval, aliveAt: _now.Value); - - Advance(TimeSpan.FromSeconds(136)); - - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeTrue(); - } - - [Fact] - public void WithNoMaxIdleInterval_ShouldMeasureAgainstTheTtlAlone() - { - Connection(ttl: Ttl, maxIdleInterval: null, aliveAt: _now.Value); - - Advance(TimeSpan.FromSeconds(121)); - - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeTrue(); - } - - [Theory] - [InlineData(0)] - [InlineData(15)] - [InlineData(-1)] - public void WithAnUnrepresentablyLargeTtl_ShouldNotThrow(int maxIdleIntervalSeconds) - { - // Regression. This used to be computed as ConfirmedAliveAt.Add(ttl), which throws - // ArgumentOutOfRangeException once the result runs past DateTimeOffset.MaxValue. The - // exception escaped into the command loop, where it was logged and dropped - silently - // abandoning whichever state transition was in progress. Reachable today from - // ConnectionSandboxOperatingSystemEventsForNetworkSpecs, which injects a MaxValue ttl - // through a Connected message to assert RTN21 override behaviour. - Connection( - ttl: TimeSpan.MaxValue, - maxIdleInterval: TimeSpan.FromSeconds(maxIdleIntervalSeconds), - aliveAt: _now.Value); - - var ex = Record.Exception(() => _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn)); - - ex.Should().BeNull(); - - // An unreachable window can never have elapsed. - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeFalse(); - } - - [Theory] - [InlineData(-1)] - [InlineData(-120)] - public void WithANegativeMaxIdleInterval_ShouldNotThrow(int maxIdleIntervalSeconds) - { - // Nothing between the wire and here validates the sign - TimeSpanJsonConverter will hand - // back a negative TimeSpan for a negative number - and a negative made the overflow - // guard's own subtraction throw. That exception escapes HandleSetStateCommand and is - // dropped by the command loop, leaving the client wedged in DISCONNECTED with no - // transport: the same failure this method was rewritten to remove. - Connection( - ttl: Ttl, - maxIdleInterval: TimeSpan.FromSeconds(maxIdleIntervalSeconds), - aliveAt: _now.Value); - - Advance(TimeSpan.FromSeconds(130)); - - var ex = Record.Exception(() => _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn)); - ex.Should().BeNull(); - - // Treated as no promise at all, so the window is the ttl alone and 130s is past it. - _state.Connection.HasConnectionStateTtlPassed(_now.ValueFn).Should().BeTrue(); - } - - private void Connection(TimeSpan ttl, TimeSpan? maxIdleInterval, DateTimeOffset? aliveAt) - { - _state.Connection.ConnectionStateTtl = ttl; - _state.Connection.MaxIdleInterval = maxIdleInterval; - - if (aliveAt.HasValue) - { - _state.Connection.SetConfirmedAlive(aliveAt.Value); - } - } - - private void Advance(TimeSpan by) => _now.Reset(_now.Value.Add(by)); - - public ConnectionStateFreshnessSpecs(ITestOutputHelper output) - : base(output) - { - } - } -} diff --git a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs index 1f91288c3..45415d6dc 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs @@ -109,27 +109,95 @@ public ConnectingStateSpecs(ITestOutputHelper output) public class ConnectingCommandSpecs : AblyRealtimeSpecs { + // UTS: realtime/unit/RTN14h/resume-after-ttl-0 [Fact] - public async Task WithInboundErrorMessageWhenItCanUseFallBack_ShouldClearsConnectionKey() + [Trait("spec", "RTN14h")] + [Trait("spec", "RTN15b1")] + public async Task AfterSuspendedAndAFailedAttempt_EveryReconnectionShouldCarryResume() { - // Arrange - var client = GetRealtimeClient(options => + // RTN14h: "Reconnection attempts in this state should continue to attempt to + // resume, regardless of how long it has been since the client was last connected." + var client = await GetConnectedClient(); + var key = client.State.Connection.Key; + var id = client.State.Connection.Id; + key.Should().NotBeNullOrEmpty(); + + client.ExecuteCommand(SetSuspendedStateCommand.Create(ErrorInfo.ReasonSuspended)); + await client.ProcessCommands(); + + // RTN8d, RTN9d - retained, because SUSPENDED is not one of the terminal states. + client.State.Connection.Key.Should().Be(key); + client.State.Connection.Id.Should().Be(id); + + client.ExecuteCommand(SetConnectingStateCommand.Create()); + await client.ProcessCommands(); + LastCreatedTransport.Parameters.GetParams() + .Should().Contain(new KeyValuePair("resume", key)); // RTN15b1 + } + + [Fact] + [Trait("spec", "RTN14h")] + [Trait("spec", "RTN15b1")] + public async Task AfterAFailedAttempt_TheNextReconnectionShouldStillCarryResume() + { + // RTN14h's "continue to attempt to resume" applies from the second attempt onwards, + // not just after a suspend. + // + // Deliberately never passes through SUSPENDED, and asserts as much: once AttemptsInfo + // has recorded a suspend, ShouldSuspend returns true and the DISCONNECTED handler + // diverts straight back to SUSPENDED without reaching the path under test. + var client = await GetConnectedClient(opts => + opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + + var key = client.State.Connection.Key; + key.Should().NotBeNullOrEmpty(); + + client.ExecuteCommand(HandleConnectingErrorCommand.Create( + new ErrorInfo("boom", 50000, System.Net.HttpStatusCode.InternalServerError))); + await client.ProcessCommands(); + + client.Connection.State.Should().NotBe(ConnectionState.Suspended); + client.State.Connection.Key.Should().Be(key); + + // A 500 earns the RTN15h3 instant retry, so the next transport is already built by + // the time the commands settle, and must carry the resume. + LastCreatedTransport.Parameters.GetParams() + .Should().Contain(new KeyValuePair("resume", key)); + } + + [Fact] + [Trait("spec", "RTN14h")] + public async Task WithInboundErrorMessageWhenItCanUseFallBack_ShouldKeepConnectionKey() + { + // RTN14h - a retryable inbound ERROR must not cost the key, because the reconnection + // attempt has to continue to attempt to resume. The client is driven through + // CONNECTED first so there is a real key at stake. + var client = await GetConnectedClient(options => { options.RealtimeRequestTimeout = TimeSpan.FromSeconds(60); options.DisconnectedRetryTimeout = TimeSpan.FromSeconds(60); }); - await client.WaitForState(ConnectionState.Connecting); + var key = client.State.Connection.Key; + key.Should().NotBeNullOrEmpty(); + + // Back to CONNECTING, where a retryable inbound ERROR is a failed attempt rather + // than RTN15j's fatal connection error. + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.ProcessCommands(); + client.ExecuteCommand(SetConnectingStateCommand.Create()); + await client.ProcessCommands(); + + client.State.Connection.Key.Should().Be(key); var messageWithError = new ProtocolMessage(ProtocolMessage.MessageAction.Error) { Error = new ErrorInfo("test", 123, System.Net.HttpStatusCode.InternalServerError), }; - // Act await client.ProcessMessage(messageWithError); - client.State.Connection.Key.Should().BeEmpty(); + client.State.Connection.Key.Should().Be(key); } [Fact] @@ -1054,6 +1122,44 @@ public async Task WhenDetachingWhileDisconnected_ShouldDetachImmediately() client.State.PendingMessages.Should().BeEmpty(); } + [Theory] + [InlineData(ConnectionState.Closing)] + [InlineData(ConnectionState.Closed)] + [InlineData(ConnectionState.Failed)] + [Trait("spec", "RTN8d")] + [Trait("spec", "RTN9d")] + public async Task WhenATerminalStateIsEmitted_TheKeyAndIdShouldAlreadyBeNull( + ConnectionState state) + { + // RTN8d and RTN9d: both are null in CLOSED, CLOSING and FAILED. SetState is what + // emits - inline, when no SynchronizationContext is installed - so the clear has to + // precede it. + // + // Read from inside the listener deliberately: asserting after the commands settle + // passes either way. Same shape as the RTL3d1 ordering requirement one clause over. + var client = await GetConnectedClient(); + client.State.Connection.Key.Should().NotBeEmpty(); + + string keyAtEmit = null; + string idAtEmit = null; + client.Connection.On(state.ToConnectionEvent(), _ => + { + keyAtEmit = client.Connection.Key; + idAtEmit = client.Connection.Id; + }); + + client.ExecuteCommand(state switch + { + ConnectionState.Closing => SetClosingStateCommand.Create(), + ConnectionState.Closed => SetClosedStateCommand.Create(), + _ => SetFailedStateCommand.Create(new ErrorInfo("gone", 1)), + }); + await client.ProcessCommands(); + + keyAtEmit.Should().BeNullOrEmpty(); + idAtEmit.Should().BeNullOrEmpty(); + } + [Theory] [InlineData(ConnectionState.Failed)] [InlineData(ConnectionState.Closed)] @@ -1061,25 +1167,24 @@ public async Task WhenDetachingWhileDisconnected_ShouldDetachImmediately() [Trait("spec", "RTN7e")] [Trait("spec", "RTN8d")] [Trait("spec", "RTN9d")] - public async Task WhenTheTransitionThrows_ShouldStillClearTheKeyAndDestroyTheTransport( + [Trait("spec", "RTN14h")] + public async Task WhenTheTransitionThrows_ShouldStillCompleteTheTeardown( ConnectionState state) { - // RTN8d and RTN9d: connectionId and connectionKey are null in CLOSED, CLOSING and - // FAILED. The connection has entered the state by the time SetState rethrows, so - // leaving the clear outside the finally meant a throwing transition reporting the - // terminal state while still holding a resumable key, and left a live transport - // whose listener kept refreshing the activity timestamp behind an RTN23a monitor - // gated on Connected. RTN7e's failure of the ack queue was skipped the same way, - // stranding those messages with no callback at all. + // The teardown must complete even when SetState rethrows: otherwise a live transport + // survives with its listener attached, behind an RTN23a monitor gated on Connected, + // and RTN7e's failure of the ack queue is skipped. // - // All three sites carry the same finally, so all three are driven here. + // All three states are driven because the key and id differ between them: RTN8d and + // RTN9d name only CLOSED, CLOSING and FAILED, while SUSPENDED retains them for + // RTN14h's next resume. var client = await GetConnectedClient(); client.State.WaitingForAck.Add(new MessageAndCallback(new ProtocolMessage(), null)); - // Connection.NotifyUpdate invokes internal handlers unguarded, which puts the throw - // where the bug needed it: after the connection has entered the state, and before - // the teardown. A plain Exception rather than an AblyException, so the workflow's - // own catch does not convert the outcome into FAILED and hide the state under test. + // Connection.NotifyUpdate invokes internal handlers unguarded, so throwing from one + // lands after the connection has entered the state and before the teardown. A plain + // Exception, not an AblyException, so the workflow's catch does not divert to FAILED + // and hide the state under test. client.Connection.InternalStateChanged += (_, change) => { if (change.Current == state) @@ -1098,9 +1203,18 @@ public async Task WhenTheTransitionThrows_ShouldStillClearTheKeyAndDestroyTheTra client.Connection.State.Should().Be(state); client.State.WaitingForAck.Should().BeEmpty(); - client.State.Connection.Key.Should().BeEmpty(); - client.State.Connection.Id.Should().BeEmpty(); client.ConnectionManager.Transport.Should().BeNull(); + + if (state == ConnectionState.Suspended) + { + client.State.Connection.Key.Should().NotBeEmpty(); + client.State.Connection.Id.Should().NotBeEmpty(); + } + else + { + client.State.Connection.Key.Should().BeEmpty(); + client.State.Connection.Id.Should().BeEmpty(); + } } [Fact] @@ -1203,12 +1317,11 @@ public async Task OnAnUpdate_ShouldKeepTheSerialSequence() } [Fact] - [Trait("spec", "RTN15g3")] + [Trait("spec", "RTN15c7")] public async Task OnAFreshConnectionWithoutAnError_ShouldRestartTheSerialSequence() { - // The RTN15g case, and the one that was broken: connection state was cleared, so - // Ably answers with a new connectionId and no error. Both of the guards this used - // to depend on were false here, which is precisely when a restart is needed. + // A resume the server refuses is answered with a new connectionId and, often, no + // error at all - which is precisely when the sequence must restart. var client = await GetClientWithOneUnackedMessage(); await Reconnect(client, connectionId: "different"); @@ -1342,28 +1455,24 @@ public ConnectionContinuitySpecs(ITestOutputHelper output) public class ConnectedUpdateSpecs : AblyRealtimeSpecs { [Fact] - [Trait("spec", "RTN15g3")] + [Trait("spec", "RTN15c7")] [Trait("spec", "RTL3d")] - public async Task AfterAnRtn15gClear_ShouldReattachAnAttachedChannel() + public async Task AfterAFailedResume_ShouldReattachAnAttachedChannel() { - // Gating the reattach on a changed connectionId cannot work here: RTN15g empties - // Connection.Id before the CONNECTING transition, so there is nothing left to - // compare against by the time CONNECTED arrives, and the channel would stay locally - // ATTACHED on a brand new connection with no server-side attachment. + // RTN15c7 - a resume the server refused, answered with a connectionId we were not + // holding. RTL3d reattaches regardless. var client = await GetConnectedClient(); var channel = (RealtimeChannel)client.Channels.Get("test"); channel.SetChannelState(ChannelState.Attached); await client.ProcessCommands(); - // Force the RTN15g path: last activity long enough ago that the state is stale. - client.State.Connection.SetConfirmedAlive(DateTimeOffset.UtcNow.AddMinutes(-30)); client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); await client.ProcessCommands(); client.ExecuteCommand(SetConnectingStateCommand.Create()); await client.ProcessCommands(); - // RTN15g should have discarded the connection state. - client.State.Connection.Key.Should().BeEmpty(); + // RTN14h - the reconnection attempt still carries the resume. + client.State.Connection.Key.Should().NotBeEmpty(); LastCreatedTransport.SentMessages.Clear(); client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) From 40f5e1e09ba0577a67c068d5387cee8db15fcf7d Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 3 Sep 2026 10:52:25 +0100 Subject: [PATCH 08/10] Return a recovery key while suspended RTN16g3 - implemented. Replaces RTN16g2, which listed SUSPENDED among the states where createRecoveryKey returns null. RTN8d and RTN9d now keep the connectionKey through SUSPENDED because RTN14h always attempts a resume, so the connection is still recoverable there and the key has to be available to hand to another client. Withholding it left the SDK holding a usable recovery key it would not surface, in the one prolonged-outage state where handing recovery over is most useful. RTN16i - fixed alongside it. GetChannelSerials filtered on ChannelState.Attached, and RTL3c puts every channel into SUSPENDED when the connection suspends, so the key would have gone out with no channelSerials at all - connection continuity without message continuity, and nothing to tell the caller. Gated on the serial instead, as ably-js does; RTL15b2 already keeps it through SUSPENDED. RTN16g3 is not yet in the published specification - ably/specification#511 adds it and tombstones RTN16g2. Implemented ahead of that merging because it is the direct consequence of RTN14h in the previous commit, and shipping the two apart would mean two behaviour changes for callers instead of one. ably-js already behaves this way, from 2.27.0. Co-Authored-By: Claude Opus 5 --- src/IO.Ably.Shared/Realtime/Connection.cs | 7 +++- .../Realtime/RealtimeChannels.cs | 5 ++- .../ConnectionRecoverySpecs.cs | 39 ++++++++++++++++++- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Connection.cs b/src/IO.Ably.Shared/Realtime/Connection.cs index 5eae16db9..2db827ad3 100644 --- a/src/IO.Ably.Shared/Realtime/Connection.cs +++ b/src/IO.Ably.Shared/Realtime/Connection.cs @@ -181,10 +181,13 @@ private void HandleNetworkStateChange(NetworkState state) /// recoveryKey. public string CreateRecoveryKey() { + // RTN16g3, which replaces RTN16g2 as of specification 6.1.0 - null in CLOSED, CLOSING + // and FAILED, and SUSPENDED is deliberately not among them. RTN8d and RTN9d keep the + // key through SUSPENDED because RTN14h always attempts a resume, so the connection is + // still recoverable there and the key has to be available to hand over. if (Key.IsEmpty() || InnerState.State == Realtime.ConnectionState.Closing || InnerState.State == Realtime.ConnectionState.Closed - || InnerState.State == Realtime.ConnectionState.Failed - || InnerState.State == Realtime.ConnectionState.Suspended) + || InnerState.State == Realtime.ConnectionState.Failed) { return string.Empty; } diff --git a/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs b/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs index e3ca4cc85..ccd4d46df 100644 --- a/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs +++ b/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs @@ -276,7 +276,10 @@ internal IDictionary GetChannelSerials() var channelSerials = new Dictionary(); foreach (var realtimeChannel in this) { - if (realtimeChannel.State == ChannelState.Attached) + // Gated on the serial, not on the channel state. RTL15b2 keeps the serial through + // SUSPENDED and RTN16i needs it in the recovery key, so a state test would drop it + // in exactly the state RTN16g3 hands a key out for. ably-js gates on the serial too. + if (realtimeChannel.Properties.ChannelSerial.IsNotEmpty()) { channelSerials[realtimeChannel.Name] = realtimeChannel.Properties.ChannelSerial; } diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionRecoverySpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionRecoverySpecs.cs index 25d400647..bc4d7027a 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionRecoverySpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionRecoverySpecs.cs @@ -34,7 +34,7 @@ public async Task CreateRecoveryKey_ShouldReturnSerializedConnectionKeyAndMsgSer } [Fact] - [Trait("spec", "RTN16g2")] + [Trait("spec", "RTN16g3")] public async Task CreateRecoveryKey_ShouldReturnNullRecoveryKeyForNullConnectionKeyOrWhenStateIsClosed() { var client = GetClientWithFakeTransport(); @@ -49,6 +49,43 @@ public async Task CreateRecoveryKey_ShouldReturnNullRecoveryKeyForNullConnection client.Connection.CreateRecoveryKey().Should().BeNullOrEmpty(); } + [Fact] + [Trait("spec", "RTN16g3")] + [Trait("spec", "RTN14h")] + public async Task CreateRecoveryKey_ShouldReturnAKeyWhileSuspended() + { + // RTN16g3 lists CLOSED, CLOSING and FAILED, and deliberately not SUSPENDED: RTN8d and + // RTN9d keep the key there because RTN14h always attempts a resume, so the connection + // is still recoverable and the key can be handed to another client. + var client = GetClientWithFakeTransport(); + client.FakeProtocolMessageReceived(ConnectedProtocolMessage); + await client.WaitForState(ConnectionState.Connected); + + // An attached channel carrying a channelSerial, because RTN16i makes those the point of + // the key. Without one the assertion below holds even if they are dropped. + var channel = (RealtimeChannel)client.Channels.Get("test"); + channel.Attach(); + await client.ProcessCommands(); + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Attached) + { + Channel = "test", + ChannelSerial = "serial-abc", + }); + await client.ProcessCommands(); + + var connectedKey = client.Connection.CreateRecoveryKey(); + connectedKey.Should().Contain("serial-abc"); + + client.Workflow.QueueCommand(SetSuspendedStateCommand.Create(ErrorInfo.ReasonSuspended)); + await client.WaitForState(ConnectionState.Suspended); + await client.ProcessCommands(); + + // RTL15b2 keeps the serial through SUSPENDED, so the key is unchanged - connectionKey, + // msgSerial and channelSerials all still present. + channel.State.Should().Be(ChannelState.Suspended); + client.Connection.CreateRecoveryKey().Should().Be(connectedKey); + } + [Fact] [Trait("spec", "RTN16m")] [System.Obsolete] From 00ca8bc6c45b6983d9cef70ffe2d906442524857 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 3 Sep 2026 17:54:34 +0100 Subject: [PATCH 09/10] Pin the successful-resume case the UTS covers RTN15b, RTN15c6 - covered. The universal test suite specifies both halves of a successful resume in one case, realtime/unit/RTN15b/successful-resume-0: the reconnect carries the connectionKey in the resume query param, and the server signals success by answering with the same connectionId. WhenTransportCloses_ShouldResumeConnection already pins the first half, but feeds back a CONNECTED with no connectionId, so nothing pinned the second - and neither did the RTN15c6 work in this branch, which is about the message serial sequence rather than connection identity. Tagged with that UTS id, so the case is claimed by a test that implements all of it. --- .../ConnectionFailuresOnceConnectedSpecs.cs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs index c2022d6fc..50ac78e14 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs @@ -215,6 +215,44 @@ public async Task WhenTransportCloses_ShouldResumeConnection() LastCreatedTransport.Should().NotBeSameAs(firstTransport); } + // UTS: realtime/unit/RTN15b/successful-resume-0 + [Fact] + [Trait("spec", "RTN15b")] + [Trait("spec", "RTN15c6")] + public async Task WhenTheTransportDropsAndTheResumeSucceeds_ShouldKeepTheConnectionId() + { + // RTN15b - the reconnect carries the connectionKey in the resume query param. RTN15c6 - + // the server signals a successful resume by answering with the same connectionId, and + // may hand back a refreshed connectionKey with it. + var client = await SetupConnectedClient(); + + var connectionId = client.Connection.Id; + var connectionKey = client.Connection.Key; + connectionId.Should().NotBeNullOrEmpty(); + connectionKey.Should().NotBeNullOrEmpty(); + + // An unexpected transport drop, so the next attempt is a resume rather than a fresh + // connection. + LastCreatedTransport.Listener.OnTransportEvent(LastCreatedTransport.Id, TransportState.Closed); + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + LastCreatedTransport.Parameters.GetParams() + .Should().ContainKey("resume") + .WhoseValue.Should().Be(connectionKey); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = connectionId, + ConnectionDetails = new ConnectionDetails { ConnectionKey = "connectionKey-updated" }, + }); + await client.WaitForState(ConnectionState.Connected); + await client.ProcessCommands(); + + client.Connection.Id.Should().Be(connectionId); + client.Connection.Key.Should().Be("connectionKey-updated"); + } + [Fact] [Trait("spec", "RTN15a")] public async Task AckMessagesAreSentWhenConnectionIsDroppedAndNotResumed() From 4b24922f915fdd60efa11390ac7026b67c816895 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 3 Sep 2026 19:57:16 +0100 Subject: [PATCH 10/10] Make the UTS coverage claims true A UTS tag is a machine-readable claim that a test implements a specific case, so a tag on a test that asserts something else makes the suite report coverage it does not have. The tags added earlier were placed by reading each case's requirement table, which is a summary; the binding part is its Assertions block. Read against those, most of them over-claimed. Strengthened to assert what their case asserts: - RTL5l detach-attached-when-disconnected-1: no DETACH on the wire, not just an empty outbound queue. - RTN7e error-represents-reason-4: the publisher's error agrees with Connection.ErrorReason, which is the point of the clause. - RTN16f recover-initializes-msgserial-0: the serial on a published frame, not only the internal counter. - RTN15c7 failed-resume-new-id-0: new id, updated key, errorReason and still CONNECTED. Now routed through DISCONNECTED, as a refused resume actually arrives - reconnecting from CONNECTED trips UpdateState's same-state early return and the error never reaches Connection.ErrorReason. - RTN15h3 non-token-error-resume-0: followed through to CONNECTED, checking the resume went out and the id survived. The clause is "reconnect with a resume attempt" and only the reconnect half was covered. - RTN14h resume-after-ttl-0: every reconnection attempt carries the resume, not just the most recent one. RTN23a idle-timeout-reconnect-1 asserts the whole cycle - two attempts, an ordered state sequence, a new connectionId - so it moved to a new test that drives it. The existing test still pins what the monitor decides, which is worth keeping but is not that case. RTB1 disconnected-retry-delay-0 asserts the coefficient sequence and its cap across five retries. ReconnectionStrategyTest already did exactly that and predates this branch, so the tag moved there; the single-retry StartTimer test cannot exercise the curve, because FakeConnectionContext has no client and the attempt count is always one. Two tags removed rather than fixed: - realtime/proxy/RTN14h/resume-after-ttl-expiry-0 needs the fault-injecting proxy harness, which this repo does not have. - the second realtime/unit/RTN16f/recover-initializes-msgserial-0 - the case has no failure path, and ids must be unique per uts/docs/writing-test-specs.md. FakeTransportFactory gains CreatedTransports, since LastCreatedTransport cannot answer how many attempts were made or whether each carried a resume. --- .../Infrastructure/AblyRealtimeSpecs.cs | 2 + .../Infrastructure/FakeTransportFactory.cs | 8 ++ .../Realtime/ConnectionSandBoxSpecs.cs | 1 - .../ConnectionFailuresOnceConnectedSpecs.cs | 19 +++ .../DisconnectedStateSpecs.cs | 1 - .../Realtime/RealtimeWorkflowSpecs.cs | 128 +++++++++++++++++- .../Utils/ReconnectionStrategyTest.cs | 3 + 7 files changed, 153 insertions(+), 9 deletions(-) diff --git a/src/IO.Ably.Tests.Shared/Infrastructure/AblyRealtimeSpecs.cs b/src/IO.Ably.Tests.Shared/Infrastructure/AblyRealtimeSpecs.cs index ab3dfaad3..5c8ffd080 100644 --- a/src/IO.Ably.Tests.Shared/Infrastructure/AblyRealtimeSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Infrastructure/AblyRealtimeSpecs.cs @@ -96,6 +96,8 @@ internal AblyRealtime GetRealtimeClient(Action optionsAction, Fun protected FakeTransport LastCreatedTransport => FakeTransportFactory.LastCreatedTransport; + protected List CreatedTransports => FakeTransportFactory.CreatedTransports; + internal AblyRealtime GetClientWithFakeTransport(Action optionsAction = null, Func> handleRequestFunc = null) { var options = new ClientOptions(ValidKey) { TransportFactory = FakeTransportFactory }; diff --git a/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransportFactory.cs b/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransportFactory.cs index 8a0ee9b6f..860e84a0d 100644 --- a/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransportFactory.cs +++ b/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransportFactory.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using IO.Ably.Transport; namespace IO.Ably.Tests.Realtime @@ -7,11 +8,18 @@ public class FakeTransportFactory : ITransportFactory { public FakeTransport LastCreatedTransport { get; set; } + /// + /// Every transport created, oldest first. LastCreatedTransport alone cannot answer questions + /// about a sequence of attempts - how many were made, or whether each carried a resume. + /// + public List CreatedTransports { get; } = new List(); + public Action InitialiseFakeTransport = obj => { }; public ITransport CreateTransport(TransportParams parameters) { LastCreatedTransport = new FakeTransport(parameters); + CreatedTransports.Add(LastCreatedTransport); InitialiseFakeTransport(LastCreatedTransport); return LastCreatedTransport; } diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs index 633475e84..c51051622 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -958,7 +958,6 @@ public async Task WhenDisconnectedMessageContainsTokenError_IfTokenRenewFails_Sh stateChanges[2].Reason.Code.Should().Be(ErrorCodes.ClientAuthProviderRequestFailed); } - // UTS: realtime/proxy/RTN14h/resume-after-ttl-expiry-0 [Theory] [ProtocolData] [Trait("spec", "RTN14h")] diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs index 50ac78e14..c93e32e72 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs @@ -323,6 +323,9 @@ public async Task WithNonTokenDisconnected_ShouldReconnectImmediately() var client = await GetConnectedClient(opts => opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10)); + var originalId = client.Connection.Id; + var connectionKey = client.Connection.Key; + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) { Error = new ErrorInfo("Something else went wrong", 50000), @@ -333,6 +336,22 @@ public async Task WithNonTokenDisconnected_ShouldReconnectImmediately() // Immediately, not in ten minutes. client.State.AttemptsInfo.InstantRetryCount.Should().Be(1); client.Connection.State.Should().Be(ConnectionState.Connecting); + + // RTN15h3 asks for a reconnect *with a resume attempt*, so follow it through: the new + // attempt carries the key, and a CONNECTED bearing the same id keeps the connection. + LastCreatedTransport.Parameters.GetParams() + .Should().ContainKey("resume") + .WhoseValue.Should().Be(connectionKey); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = originalId, + ConnectionDetails = new ConnectionDetails { ConnectionKey = connectionKey }, + }); + await client.WaitForState(ConnectionState.Connected); + await client.ProcessCommands(); + + client.Connection.Id.Should().Be(originalId); } [Fact] diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs index 274381ff1..6de28f028 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionStateTests/DisconnectedStateSpecs.cs @@ -119,7 +119,6 @@ public void StartTimer_ShouldReportTheDelayItActuallyWaits() _state.RetryIn.Should().Be(_timer.LastDelay); } - // UTS: realtime/unit/RTB1/disconnected-retry-delay-0 [Fact] [Trait("spec", "RTN14d")] [Trait("spec", "RTB1")] diff --git a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs index 45415d6dc..b015cf91c 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs @@ -131,8 +131,25 @@ public async Task AfterSuspendedAndAFailedAttempt_EveryReconnectionShouldCarryRe client.ExecuteCommand(SetConnectingStateCommand.Create()); await client.ProcessCommands(); - LastCreatedTransport.Parameters.GetParams() - .Should().Contain(new KeyValuePair("resume", key)); // RTN15b1 + + // A second and third attempt, because the clause is "reconnection attempts" plural - + // one attempt carrying a resume does not show that every later one does. + for (var i = 0; i < 2; i++) + { + client.ExecuteCommand(HandleConnectingErrorCommand.Create( + new ErrorInfo("boom", 50000, System.Net.HttpStatusCode.InternalServerError))); + await client.ProcessCommands(); + client.ExecuteCommand(SetConnectingStateCommand.Create()); + await client.ProcessCommands(); + } + + var reconnects = CreatedTransports.Skip(1).ToList(); + reconnects.Should().HaveCountGreaterOrEqualTo(3); + foreach (var transport in reconnects) + { + transport.Parameters.GetParams() + .Should().Contain(new KeyValuePair("resume", key)); // RTN15b1 + } } [Fact] @@ -777,6 +794,12 @@ public async Task WhenTheConnectionFailsAMessage_ShouldReportTheReasonForTheStat errors.Should().ContainSingle(); errors[0].Message.Should().Be("the actual reason"); + errors[0].Code.Should().Be(12345); + + // The point of the clause: the error handed to the publisher is the reason the + // connection itself is reporting, not a separately minted one. + client.Connection.ErrorReason.Should().NotBeNull(); + client.Connection.ErrorReason.Code.Should().Be(errors[0].Code); } [Fact] @@ -1120,6 +1143,13 @@ public async Task WhenDetachingWhileDisconnected_ShouldDetachImmediately() detach.IsSuccess.Should().BeTrue(); channel.State.Should().Be(ChannelState.Detached); client.State.PendingMessages.Should().BeEmpty(); + + // "No DETACH message was sent (transport is unavailable)" - checked on the wire, + // not only on the outbound queue. + LastCreatedTransport.SentMessages + .Select(x => x.Original) + .Where(x => x != null) + .Should().NotContain(x => x.Action == ProtocolMessage.MessageAction.Detach); } [Theory] @@ -1336,11 +1366,31 @@ public async Task OnAFreshConnectionWithoutAnError_ShouldRestartTheSerialSequenc public async Task OnAFailedResume_ShouldRestartTheSerialSequence() { var client = await GetClientWithOneUnackedMessage(); + var originalId = client.State.Connection.Id; - await Reconnect(client, connectionId: "different", error: new ErrorInfo("resume failed", 80008)); + // Through DISCONNECTED first, as a refused resume actually arrives. Reconnecting + // straight from CONNECTED trips UpdateState's same-state early return, which drops + // the error before it reaches Connection.ErrorReason. + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.WaitForState(ConnectionState.Disconnected); + await client.ProcessCommands(); + + await Reconnect( + client, + connectionId: "different", + error: new ErrorInfo("resume failed", 80008), + connectionKey: "connectionKey-2"); SentSerials(client).Should().Equal(0L); client.State.Connection.MessageSerial.Should().Be(1); + + // The rest of what a refused resume looks like: a new id and key, the reason on the + // connection, and CONNECTED all the same. + client.State.Connection.Id.Should().Be("different").And.NotBe(originalId); + client.State.Connection.Key.Should().Be("connectionKey-2"); + client.Connection.ErrorReason.Should().NotBeNull(); + client.Connection.ErrorReason.Code.Should().Be(80008); + client.Connection.State.Should().Be(ConnectionState.Connected); } [Fact] @@ -1377,9 +1427,14 @@ public async Task OnASuccessfulRecover_ShouldKeepTheRecoveredSerial() await Reconnect(client, connectionId: "recovered"); client.State.Connection.MessageSerial.Should().Be(45); + + // And it is the serial actually put on the wire, which is what RTN16f is for. + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "test"))); + await client.ProcessCommands(); + SentSerials(client).Should().Equal(45L); } - // UTS: realtime/unit/RTN16f/recover-initializes-msgserial-0 [Fact] [Trait("spec", "RTN16f")] [Trait("spec", "RTN15c7")] @@ -1425,13 +1480,13 @@ private async Task GetClientWithOneUnackedMessage() } private static async Task Reconnect( - AblyRealtime client, string connectionId, bool isUpdate = false, ErrorInfo error = null) + AblyRealtime client, string connectionId, bool isUpdate = false, ErrorInfo error = null, string connectionKey = "connectionKey") { await client.Workflow.ProcessCommand(SetConnectedStateCommand.Create( new ProtocolMessage(ProtocolMessage.MessageAction.Connected) { ConnectionId = connectionId, - ConnectionDetails = new ConnectionDetails { ConnectionKey = "connectionKey" }, + ConnectionDetails = new ConnectionDetails { ConnectionKey = connectionKey }, Error = error, }, isUpdate)); @@ -1556,7 +1611,6 @@ public async Task ShouldUseTheConfiguredRealtimeRequestTimeout() .Should().ContainSingle().Which.Should().BeOfType(); } - // UTS: realtime/unit/RTN23a/idle-timeout-reconnect-1 [Fact] public async Task WhenIdleForLongerThanAllowed_ShouldDisconnect() { @@ -1576,6 +1630,52 @@ public async Task WhenIdleForLongerThanAllowed_ShouldDisconnect() disconnect.RetryInstantly.Should().BeTrue(); } + // UTS: realtime/unit/RTN23a/idle-timeout-reconnect-1 + [Fact] + [Trait("spec", "RTN23a")] + [Trait("spec", "RTN15a")] + public async Task WhenTheIdleTimeoutFires_ShouldDisconnectAndReconnect() + { + // The UTS case asserts the whole cycle, not just the decision to disconnect: + // connecting, connected, disconnected, connecting, connected, with a second + // connection attempt and a new connectionId. The test above pins what the monitor + // decides; this one pins what the client does with it. + var client = await GetConnectedClient(PromisedMaxIdleInterval); + + var states = new List(); + client.Connection.On(args => states.Add(args.Current)); + + CreatedTransports.Should().HaveCount(1); + + _now.Reset(_now.Value.Add(AllowedIdleTime.Add(TimeSpan.FromSeconds(1)))); + foreach (var command in await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + { + client.ExecuteCommand(command); + } + + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + // RTN15a's immediate reconnect built a second transport. + CreatedTransports.Should().HaveCount(2); + + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "connection-id-2", + ConnectionDetails = new ConnectionDetails + { + ConnectionKey = "key-2", + MaxIdleInterval = PromisedMaxIdleInterval, + }, + }); + await client.WaitForState(ConnectionState.Connected); + await client.ProcessCommands(); + + states.Should().ContainInOrder( + ConnectionState.Disconnected, ConnectionState.Connecting, ConnectionState.Connected); + client.Connection.Id.Should().Be("connection-id-2"); + } + [Fact] public async Task WhenIdleForExactlyTheAllowedTime_ShouldNotDisconnect() { @@ -1650,6 +1750,20 @@ public async Task AnyReceivedMessage_NotOnlyHeartbeat_ShouldResetTheIdleTimer() var commands = await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value)); commands.Should().BeEmpty(); + + // Still a single attempt at this point, per the case. + CreatedTransports.Should().HaveCount(1); + + // And once the window really does elapse with no activity, the cycle runs. + _now.Reset(_now.Value.Add(AllowedIdleTime.Add(TimeSpan.FromSeconds(1)))); + foreach (var command in await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + { + client.ExecuteCommand(command); + } + + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + CreatedTransports.Should().HaveCount(2); } [Fact] diff --git a/src/IO.Ably.Tests.Shared/Utils/ReconnectionStrategyTest.cs b/src/IO.Ably.Tests.Shared/Utils/ReconnectionStrategyTest.cs index 556446afa..1cee06fa4 100644 --- a/src/IO.Ably.Tests.Shared/Utils/ReconnectionStrategyTest.cs +++ b/src/IO.Ably.Tests.Shared/Utils/ReconnectionStrategyTest.cs @@ -8,8 +8,11 @@ namespace IO.Ably.Tests.Shared.Utils { public class ReconnectionStrategyTest { + // UTS: realtime/unit/RTB1/disconnected-retry-delay-0 [Fact] [Trait("spec", "RTB1")] + [Trait("spec", "RTB1a")] + [Trait("spec", "RTB1b")] public void ShouldCalculateRetryTimeoutsUsingBackOffAndJitter() { var retryAttempts = new[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };