From f6c7c11d513bfcf25dbfc8769a2f883208313b9e Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:43:22 +0100 Subject: [PATCH 01/13] 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 | 181 ++++++++++++++++-- .../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, 556 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..f9130ddfa 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,99 @@ 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(); + } + + // Read live, unlike the two per-transport snapshots above: this term is the caller's own + // slack rather than anything Ably promised on this transport, its setter validates the + // range, and it is only ever additive. + 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 +633,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 +751,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 e824cf6c908ee5a64c213358b07aae566325d593 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:44:16 +0100 Subject: [PATCH 02/13] 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. RTN24, RTN19a - an RTN24 update no longer redrives the queues at all. RTN19a is scoped to "when a transport is disconnected for any reason", and an update - a reauth, typically - disconnects nothing: the transport that will ACK the in-flight messages is the one they went out on. Resending put a duplicate of each on the wire for Ably to discard by msgSerial. Invisible to callers, and the old code did the same, but this commit reworks exactly this path and ably-js does not redrain on a CONNECTED received while already connected. Co-Authored-By: Claude Opus 5 --- .../Realtime/Workflows/RealtimeWorkflow.cs | 137 +++++++++++++----- 1 file changed, 102 insertions(+), 35 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index f9130ddfa..31fbb0c2e 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -407,7 +407,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( @@ -746,10 +759,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 @@ -768,25 +800,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, cmd.IsUpdate); // RTN19a } private void HandlePingTimer(PingTimerCommand cmd) @@ -1124,25 +1148,53 @@ private void UpdateStateAndNotifyConnection(ConnectionStateBase newState) } } - private void SendPendingMessagesOnConnected(bool failedResumeOrRecover) + private void SendPendingMessagesOnConnected(bool connectionContinues, bool isUpdate) { - // RTN19a1 - if (failedResumeOrRecover) + // RTN19 is scoped to "when a transport is disconnected for any reason", which puts an + // RTN24 update outside it: nothing was disconnected, and the transport that will ACK the + // in-flight messages is the one they went out on. Resending would put a duplicate of each + // on the wire for Ably to discard by msgSerial, and requeueing would renumber messages + // the server is still expecting under their original serials. ably-js does not re-drain + // on a CONNECTED received while already connected either. + // + // The RTL6c2 queue below is still flushed. It should be empty while connected, and + // skipping it would strand anything that did land there. + if (isUpdate == false) { - foreach (var messageAndCallback in State.WaitingForAck) + if (connectionContinues) { - State.PendingMessages.Add(new MessageAndCallback( - messageAndCallback.Message, - messageAndCallback.Callback, - messageAndCallback.Logger)); + // 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)) + { + ConnectionManager.SendToTransport(message); + } } - } - else - { - // RTN19a2 - successful resume, msgSerial doesn't change - foreach (var message in State.WaitingForAck.Select(x => x.Message)) + else { - 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(); } } @@ -1163,15 +1215,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 026ac2002dade652cc2dff1ff9bd664c60ba8e3e Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:44:39 +0100 Subject: [PATCH 03/13] 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 | 162 +++++++++++++----- .../Realtime/RealtimeChannels.cs | 18 +- .../Realtime/ChannelSpecs.cs | 65 ++++++- 4 files changed, 205 insertions(+), 53 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..bb364fa5b 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)); - } 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 d74b5bf1ecc4472dbc50f8b6ee0837a45ec840e4 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:45:01 +0100 Subject: [PATCH 04/13] 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 | 126 ++++++++++++++++++ 6 files changed, 255 insertions(+), 37 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index 31fbb0c2e..4b2e95fbb 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -496,20 +496,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; @@ -522,14 +514,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) @@ -968,11 +952,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) @@ -981,6 +984,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..d50443467 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,131 @@ 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(); + } + + [Fact] + [Trait("spec", "RTN14d")] + [Trait("spec", "RTN14e")] + public void StartTimer_WhenTheBackoffWouldOvershootTheStateTtl_ShouldWakeAtTheDeadline() + { + // RTN14e requires SUSPENDED once connectionStateTtl has elapsed, and that is decided when + // an attempt fails. Sleeping the whole RTB1 backoff past the deadline would postpone + // SUSPENDED by however long we slept - unbounded, since disconnectedRetryTimeout is a + // client option - so the wait is cut back to what is left of the ttl. + var now = TestHelpers.Now(); + _context.RetryTimeout = TimeSpan.FromSeconds(30); + + using (var client = NewClientWithFirstAttemptAt(now.AddSeconds(-119), TimeSpan.FromSeconds(120))) + { + _context.Connection = new Connection(client, () => now); + var state = GetState(); + + state.StartTimer(); + + // One second of the ttl is left, and the backoff would have waited at least 24. + _timer.LastDelay.Should().Be(TimeSpan.FromSeconds(1)); + state.RetryIn.Should().Be(TimeSpan.FromSeconds(1)); + } + } + + [Fact] + [Trait("spec", "RTN14e")] + public void StartTimer_WhenTheClockHasSteppedBack_ShouldNotWaitBeyondTheTtl() + { + // A backwards clock step leaves the first attempt in the future, making the elapsed time + // negative. Unclamped that lengthens the remaining ttl rather than shortening it, so the + // deadline is missed by however far the clock moved. + var now = TestHelpers.Now(); + _context.RetryTimeout = TimeSpan.FromSeconds(30); + + using (var client = NewClientWithFirstAttemptAt(now.AddMinutes(5), TimeSpan.FromSeconds(1))) + { + _context.Connection = new Connection(client, () => now); + var state = GetState(); + + state.StartTimer(); + + // Elapsed treated as zero, so the whole ttl remains - not the ttl plus five minutes, + // which would exceed the backoff and leave nothing clamped at all. + _timer.LastDelay.Should().Be(TimeSpan.FromSeconds(1)); + } + } + + [Fact] + [Trait("spec", "RTN14d")] + public void StartTimer_WhenTheDeadlineIsBeyondTheBackoff_ShouldWaitTheBackoffNotTheDeadline() + { + // The clamp may only ever shorten the wait. The deadline is deliberately just past the + // backoff here - 1.2s against a jittered 0.8-1.0s - so waiting until the deadline would + // overshoot RTB1 rather than being indistinguishable from it. + var now = TestHelpers.Now(); + _context.RetryTimeout = TimeSpan.FromSeconds(1); + + using (var client = NewClientWithFirstAttemptAt(now.AddSeconds(-10), TimeSpan.FromMilliseconds(11200))) + { + _context.Connection = new Connection(client, () => now); + var state = GetState(); + + state.StartTimer(); + + // 1.2s of ttl remains, so an unconditional clamp would wait that. RTB1's jitter caps + // the legitimate answer at the nominal second. + _timer.LastDelay.Should().BeLessOrEqualTo(TimeSpan.FromSeconds(1)); + _timer.LastDelay.Should().BeGreaterOrEqualTo(TimeSpan.FromMilliseconds(800)); + } + } + + /// + /// A client whose RealtimeState carries an attempt history and a connectionStateTtl, which is + /// what ClampToStateTtl reads. The shared FakeConnectionContext has no client at all, so every + /// other test in this class takes that method's null-state early return. + /// + private AblyRealtime NewClientWithFirstAttemptAt(DateTimeOffset firstAttempt, TimeSpan connectionStateTtl) + { + var client = new AblyRealtime(new ClientOptions(ValidKey) { AutoConnect = false }); + client.State.Connection.ConnectionStateTtl = connectionStateTtl; + client.State.AttemptsInfo.Attempts.Add(new ConnectionAttempt(firstAttempt)); + return client; + } + private ConnectionDisconnectedState GetState(ErrorInfo error = null) { return new ConnectionDisconnectedState(_context, error, _timer, Logger); From ed660d9383d2a74fc777dd419a2cfe692588d1fb Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:45:24 +0100 Subject: [PATCH 05/13] 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. This changes what it takes to observe a client sitting in DISCONNECTED, so the RTN12d sandbox case is reworked with it. That test forced DISCONNECTED from CONNECTED and expected the client to stay put - which is precisely the case this grants an immediate reconnect to, and against a healthy sandbox the reconnect succeeds, so close() was racing a client that had already gone back to CONNECTED. It now fails every reconnect attempt, which is what a client genuinely stuck in DISCONNECTED looks like and the only situation where RTN12d has a retry to abort at all. RTN14e - 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, so FirstAttempt stayed null and the suspend clock started late. RTN17i - that same drop is deliberately held back from host selection. RTN17f admits an exception through RSC15l1, "host unresolvable or unreachable", which describes an attempt that never landed; a transport falling out of CONNECTED says the opposite, and RTN17i requires the primary be preferred "even if a previous connection attempt to that endpoint has failed". Counting it would send the first resume after a transient blip to another datacenter while the primary was most likely healthy - and pay a distant datacenter's latency to do it. The reconnect still goes out: if the datacenter really has gone, that attempt fails at connect time, which is eligible, so the fallbacks are one attempt away rather than skipped. A server-sent DISCONNECTED carrying a 500-504 status is untouched: it reaches the ErrorInfo overload with no exception, so RTN17f1 still makes it fallback-worthy. ably-js draws the same line, setting forceFallbackHost only for a statusCode above 500 and rebuilding each attempt's candidate list primary-first. One consequence to note: the instant retry budget is domain count, so the first unit is now spent on the primary and the last fallback is reached on an RTB1 timer rather than instantly. Every domain is still reachable, just not all of them within the instant path. 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 | 87 +++++++++- .../Transport/AttemptFailedState.cs | 13 +- .../Transport/ConnectionAttemptsInfo.cs | 36 +++- .../Connection/ConnectionConnectedState.cs | 3 + .../Realtime/ConnectionSandBoxSpecs.cs | 48 +++++- .../ConnectionFailuresOnceConnectedSpecs.cs | 69 ++++++++ .../ConnectionFallbackSpecs.cs | 158 ++++++++++++++++++ 8 files changed, 409 insertions(+), 20 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 4b2e95fbb..e8acb8851 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -883,13 +883,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); @@ -1013,7 +1037,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() @@ -1023,9 +1061,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; @@ -1123,7 +1194,7 @@ public void SetState(ConnectionStateBase newState, bool skipTimer = false) return; } - State.AttemptsInfo.UpdateAttemptState(newState, Logger); + State.AttemptsInfo.UpdateAttemptState(newState, State.Connection.State, Logger); State.Connection.CurrentStateObject.AbortTimer(); } diff --git a/src/IO.Ably.Shared/Transport/AttemptFailedState.cs b/src/IO.Ably.Shared/Transport/AttemptFailedState.cs index d16beaae5..113ede7aa 100644 --- a/src/IO.Ably.Shared/Transport/AttemptFailedState.cs +++ b/src/IO.Ably.Shared/Transport/AttemptFailedState.cs @@ -5,16 +5,19 @@ namespace IO.Ably.Transport { internal sealed class AttemptFailedState { + private readonly bool _droppedAnEstablishedConnection; + public AttemptFailedState(ConnectionState state, ErrorInfo error) { State = state; Error = error; } - public AttemptFailedState(ConnectionState state, Exception ex) + public AttemptFailedState(ConnectionState state, Exception ex, bool droppedAnEstablishedConnection = false) { State = state; Exception = ex; + _droppedAnEstablishedConnection = droppedAnEstablishedConnection; } public ErrorInfo Error { get; } @@ -36,7 +39,13 @@ private bool IsDisconnectedOrSuspendedState() private bool IsRecoverableException() { - return Exception != null; + // RTN17f admits an exception through RSC15l1, "host unresolvable or unreachable", which + // describes an attempt that never landed. A transport dropping out of CONNECTED says the + // opposite - the host answered a moment ago - so it is not on its own grounds to move off + // the primary, and RTN17i requires the primary be preferred regardless. The reconnect + // still goes out; if the datacenter really has gone, that attempt fails at connect time + // and is eligible, so the fallbacks are one attempt away rather than skipped. + return Exception != null && _droppedAnEstablishedConnection == false; } private bool IsRecoverableError() diff --git a/src/IO.Ably.Shared/Transport/ConnectionAttemptsInfo.cs b/src/IO.Ably.Shared/Transport/ConnectionAttemptsInfo.cs index 032796196..8cc293845 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() @@ -40,7 +53,7 @@ public int DisconnectedCount() => Attempts.SelectMany(x => x.FailedStates) public int SuspendedCount() => Attempts.SelectMany(x => x.FailedStates) .Count(x => x.State == ConnectionState.Suspended); - public void UpdateAttemptState(ConnectionStateBase newState, ILogger logger) + public void UpdateAttemptState(ConnectionStateBase newState, ConnectionState previousState, ILogger logger) { switch (newState.State) { @@ -59,7 +72,10 @@ public void UpdateAttemptState(ConnectionStateBase newState, ILogger logger) logger.Debug($"Recording failed attempt for state {newState.State}."); if (newState.Exception != null) { - RecordAttemptFailure(newState.State, newState.Exception); + RecordAttemptFailure( + newState.State, + newState.Exception, + droppedAnEstablishedConnection: previousState == ConnectionState.Connected); } else { @@ -80,12 +96,20 @@ private void RecordAttemptFailure(ConnectionState state, ErrorInfo error) } } - private void RecordAttemptFailure(ConnectionState state, Exception ex) + private void RecordAttemptFailure(ConnectionState state, Exception ex, bool droppedAnEstablishedConnection) { - 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 + // altogether would leave FirstAttempt null and delay the RTN14e clock. + // + // Recorded for that clock but held back from RTN17 host selection, which is what + // droppedAnEstablishedConnection carries - see AttemptFailedState. + var attempt = Attempts.LastOrDefault() ?? new ConnectionAttempt(_now()); + attempt.FailedStates.Add(new AttemptFailedState(state, ex, droppedAnEstablishedConnection)); + 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/ConnectionSandBoxSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs index cbbf8f4c4..7f2b141a3 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -270,6 +270,12 @@ async Task AssertsClosesAndDoesNotReconnect(AblyRealtime realtime, ConnectionSta { await realtime.WaitForState(state); + // Drained before the watch is armed: the retry that put us in this state may still + // have a CONNECTING queued behind it, and that would read as a reconnect close() + // failed to abort rather than one it never had the chance to. + await realtime.ProcessCommands(); + realtime.Connection.State.Should().Be(state); + var reconnectAwaiter = new TaskCompletionAwaiter(5000); realtime.Connection.On(args => { @@ -284,19 +290,59 @@ async Task AssertsClosesAndDoesNotReconnect(AblyRealtime realtime, ConnectionSta await realtime.WaitForState(ConnectionState.Closed); realtime.Connection.State.Should().Be(ConnectionState.Closed); + // This covers the path rather than the abort. SetState aborts the outgoing state's + // timer on every transition, so RTN12d's "aborts the retry process" cannot be broken + // without also breaking the transition to CLOSED, which WaitForState above catches + // first. The abort is pinned where it can actually be observed failing, on the state + // objects, by DisconnectedStateSpecs and SuspendedStateSpecs. var didReconnect = await reconnectAwaiter.Task; didReconnect.Should().BeFalse($"should not attempt a reconnect for state {state}"); } - // setup a new client and put into a DISCONNECTED state + // Set up a client that is genuinely stuck in DISCONNECTED. Forcing the state is no longer + // enough by itself: RTN15h3 grants a non-token DISCONNECTED arriving while CONNECTED an + // immediate reconnect, and against a healthy sandbox that reconnect succeeds - so the + // client is CONNECTED again before close() can be called, and the assertions race it. + // Failing every reconnect attempt is what a client stuck in DISCONNECTED actually looks + // like, and it is the only situation in which RTN12d's "aborts the retry process" has a + // retry to abort. + var failConnects = false; + var throwingTransports = new TestTransportFactory(t => t.ThrowOnConnect = failConnects); + var client = await GetRealtimeClient(protocol, (opts, _) => { + // Kept short deliberately. Stability comes from draining the queue below, not from + // a long timeout - and the retry has to be due inside the window the assertion + // watches, or a close() that failed to abort it would go unnoticed. opts.DisconnectedRetryTimeout = TimeSpan.FromSeconds(2); + opts.TransportFactory = throwingTransports; }); await client.WaitForState(ConnectionState.Connected); + + failConnects = true; client.Workflow.QueueCommand(SetDisconnectedStateCommand.Create(new ErrorInfo("force disconnect"))); + // The immediate retries are bounded by the number of domains to traverse, and until that + // budget is spent the client is legitimately still cycling through CONNECTING - which is + // not the state RTN12d is about. Wait for it to run out before closing. + var domainCount = 1 + client.State.Connection.FallbackHosts.Count; + var budgetDeadline = DateTimeOffset.UtcNow.AddSeconds(30); + while (client.State.AttemptsInfo.InstantRetryCount < domainCount + && DateTimeOffset.UtcNow < budgetDeadline) + { + await Task.Delay(50); + } + + client.State.AttemptsInfo.InstantRetryCount.Should().Be( + domainCount, + "the client cannot sit still in DISCONNECTED until the immediate retries are spent"); + + // The budget is recorded when the retry is granted, not when its attempt finishes, so + // the last CONNECTING is still in flight here. Let it fail before closing. + await client.WaitForState(ConnectionState.Disconnected); + await client.ProcessCommands(); + await AssertsClosesAndDoesNotReconnect(client, ConnectionState.Disconnected); // reinitialize the client and put into a SUSPENDED state 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..ca95984cc 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs @@ -8,6 +8,7 @@ using IO.Ably.Realtime; using IO.Ably.Realtime.Workflow; using IO.Ably.Tests.Infrastructure; +using IO.Ably.Transport; using IO.Ably.Types; using Xunit; using Xunit.Abstractions; @@ -110,6 +111,163 @@ 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", "RTN17i")] + public async Task AfterAnExceptionDropsAConnectedTransport_ShouldTryThePrimaryBeforeAnyFallback() + { + // RTN17i - "every connection attempt is first attempted to the primary domain ... even if + // a previous connection attempt to that endpoint has failed". A transport dropping out of + // CONNECTED is not grounds to move off the primary: it answered a moment ago. Sending the + // first reconnect to another datacenter would abandon a healthy primary on a blip, and + // pay the latency of a distant one to do it. + var client = await GetConnectedClient(opts => + opts.DisconnectedRetryTimeout = TimeSpan.FromMilliseconds(10)); + + client.State.Connection.FallbackHosts.Should().NotBeEmpty("otherwise there is nothing to prefer the primary over"); + + var hostsTried = new List(); + FakeTransportFactory.InitialiseFakeTransport = t => hostsTried.Add(t.Parameters.Host); + + // An ordinary socket error. This is the path that carries an Exception rather than an + // ErrorInfo, and so the one that used to count as a fallback-worthy failure. + LastCreatedTransport.Listener.OnTransportEvent( + LastCreatedTransport.Id, + TransportState.Closed, + new Exception("socket closed")); + + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + hostsTried.Should().Equal(Defaults.RealtimeHost); + + // The fallbacks are deferred by one attempt rather than lost. That reconnect to the + // primary now fails at connect time, which is the RSC15l1 "host unreachable" case RTN17f + // does admit, so the attempt after it moves off the primary. + LastCreatedTransport.Listener.OnTransportEvent( + LastCreatedTransport.Id, + TransportState.Closed, + new Exception("connect failed")); + + await client.ProcessCommands(); + + // Asserted on the attempt straight after the primary failed, not on the last one: with + // instant retries and a 10ms timeout several attempts land here, so Last() would read as + // a fallback even if the primary had been tried twice over first. + hostsTried.Should().HaveCountGreaterThan(1); + hostsTried[1].Should().BeOneOf(client.State.Connection.FallbackHosts); + } + + [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 169f8574e731349beae7d1c3d9e92a5161ba2deb Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Fri, 28 Aug 2026 10:45:59 +0100 Subject: [PATCH 06/13] 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 | 1276 ++++++++++++++++- 9 files changed, 1565 insertions(+), 32 deletions(-) diff --git a/src/IO.Ably.Shared/AblyAuth.cs b/src/IO.Ably.Shared/AblyAuth.cs index c5f13e580..404b4fa28 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 e8acb8851..45fc3f853 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -363,6 +363,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)); @@ -952,14 +963,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() { @@ -1111,7 +1144,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) { @@ -1129,27 +1162,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; } @@ -1180,6 +1235,8 @@ public void SetState(ConnectionStateBase newState, bool skipTimer = false) Logger.Debug(message); } + var notified = false; + try { if (newState.IsUpdate == false) @@ -1207,13 +1264,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..8b84f3c19 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,1226 @@ 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", "RTN15c6")] + public async Task OnAResumeCarryingAnError_ShouldStillKeepTheSerialSequence() + { + // RTN15c6 - continuity is judged on the connectionId alone. A resume the server + // honoured can still carry a non-fatal error, which must be surfaced without + // restarting the sequence. The old code keyed continuity on the error's presence and + // so got precisely this case wrong, renumbering a connection that had continued. + var client = await GetClientWithOneUnackedMessage(); + + // Through DISCONNECTED first, as a real reconnect 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: "1", + error: new ErrorInfo("resumed, with something to report", 80018)); + + // Same id, so the counter and the in-flight serial both stand. + client.State.Connection.MessageSerial.Should().Be(3); + SentSerials(client).Should().Equal(2L); + + // And the error still reaches the application, per RTN25. + client.Connection.ErrorReason.Should().NotBeNull(); + client.Connection.ErrorReason.Code.Should().Be(80018); + client.Connection.State.Should().Be(ConnectionState.Connected); + } + + [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", "RTN24")] + [Trait("spec", "RTN19a")] + public async Task OnAnUpdate_ShouldNotResendTheMessagesStillInFlight() + { + // RTN19a is scoped to a transport that has been disconnected. An RTN24 update - a + // reauth, typically - disconnects nothing, so the transport still holding these + // messages is the one that will ACK them. Resending would put a duplicate of each on + // the wire for Ably to discard by msgSerial. + var client = await GetClientWithOneUnackedMessage(); + + await Reconnect(client, connectionId: "1", isUpdate: true); + + SentSerials(client).Should().BeEmpty(); + + // Left in flight under its original serial, which is the one the ACK will carry. + client.State.WaitingForAck.Should().HaveCount(1); + client.State.WaitingForAck.Single().Message.MsgSerial.Should().Be(2); + } + + [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); + } + + [Fact] + [Trait("spec", "RTN19a1")] + public async Task OnAFailedResume_ShouldRenumberTheInFlightMessageAheadOfTheQueuedOne() + { + // The in-flight message was published before the one queued while disconnected, so on + // a fresh sequence it must take the lower serial. RTN19a1 puts the renumbered + // messages on the RTL6c2 queue, which already holds the later one - appending would + // reverse publish order on the wire, and every other test here leaves that queue + // empty, so nothing pins it. + var client = await GetClientWithOneUnackedMessage(); + + // A publish while disconnected, on its own channel so the two are told apart on the + // wire rather than only by serial - a reversal would still read as 0 then 1. + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.WaitForState(ConnectionState.Disconnected); + client.ExecuteCommand(SendMessageCommand.Create( + new ProtocolMessage(ProtocolMessage.MessageAction.Message, "queued-while-disconnected"))); + await client.ProcessCommands(); + + client.State.WaitingForAck.Should().HaveCount(1, "the in-flight message is still awaiting its ACK"); + client.State.PendingMessages.Should().HaveCount(1, "and the new publish is queued behind it"); + + await Reconnect(client, connectionId: "different"); + + SentFrames(client).Should().Equal( + ("test", 0L), + ("queued-while-disconnected", 1L)); + } + + // 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<(string Channel, long Serial)> SentFrames(AblyRealtime client) => + LastCreatedTransport.SentMessages + .Select(x => x.Original) + .Where(x => x != null && x.Action == ProtocolMessage.MessageAction.Message) + .Select(x => (x.Channel, x.MsgSerial)) + .ToList(); + + 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 0b62bf13682901cd6a512a03ff4cc846fab1b3bc Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Wed, 2 Sep 2026 15:20:15 +0100 Subject: [PATCH 07/13] 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 plumbing goes entirely - from SetDisconnectedStateCommand and SetSuspendedStateCommand, whose violation it was, and from SetConnectingStateCommand and HandleConnectingErrorCommand, where it no longer does anything. Leaving it would let the violation back in unnoticed, and leave a retired mechanism looking load-bearing. ConnectionClosingState was its last caller, for RTN11b/RTN11d's clean connection, and needs it no longer: entering CLOSING now clears key and id for RTN8d/RTN9d before the emit, and the single reader processes that before it can reach ClosingState.Connect(), so the following CONNECTED finds no id to match and restarts the serial sequence under RTN15c7 regardless. HandleConnectingErrorCommand's copy was never read by any handler. RTN27d - the 6.1.0 edit left it describing the old model. It called SUSPENDED a state whose "next connect attempt is a clean connection (not a resume attempt)", which RTN14h, RTN8d, RTN9d and DF1a between them contradict; the commit that made those changes did not touch RTN27 at all. ably/specification#511 has since amended the clause in place and dropped that sentence, so this commit follows RTN27d rather than deviating from it. Amending in place is the right shape for RTN27, which declares its states "mutually exclusive and exhaustive" - tombstoning the clause would have left SUSPENDED undescribed. ably-js retains the key in SUSPENDED too, citing RTN8d/RTN9d and RTN14h for it. Co-Authored-By: Claude Opus 5 --- .../Realtime/RealtimeChannel.cs | 47 ++--- .../Realtime/Workflows/RealtimeCommands.cs | 44 ++--- .../Realtime/Workflows/RealtimeState.cs | 37 ---- .../Realtime/Workflows/RealtimeWorkflow.cs | 76 ++------ .../Connection/ConnectionClosingState.cs | 6 +- .../IO.Ably.Tests.Shared.projitems | 1 - .../Realtime/ChannelSandboxSpecs.cs | 56 ------ .../Realtime/ChannelSpecs.cs | 143 ++++++++++++++ .../Realtime/ChannelsSpecs.cs | 14 -- .../Realtime/ConnectionSandBoxSpecs.cs | 66 +++---- .../Realtime/ConnectionStateFreshnessSpecs.cs | 142 -------------- .../Realtime/RealtimeWorkflowSpecs.cs | 179 ++++++++++++++---- 12 files changed, 358 insertions(+), 453 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 bb364fa5b..ccaa656a2 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..175694aca 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs @@ -154,15 +154,12 @@ private CloseConnectionCommand() internal class SetConnectingStateCommand : RealtimeCommand { - private SetConnectingStateCommand(bool clearConnectionKey, bool retryAuth, bool? connectivityConfirmed) + private SetConnectingStateCommand(bool retryAuth, bool? connectivityConfirmed) { - ClearConnectionKey = clearConnectionKey; RetryAuth = retryAuth; ConnectivityConfirmed = connectivityConfirmed; } - public bool ClearConnectionKey { get; } - public bool RetryAuth { get; } /// @@ -172,8 +169,8 @@ private SetConnectingStateCommand(bool clearConnectionKey, bool retryAuth, bool? /// public bool? ConnectivityConfirmed { get; } - public static SetConnectingStateCommand Create(bool clearConnectionKey = false, bool retryAuth = false, bool? connectivityConfirmed = null) => - new SetConnectingStateCommand(clearConnectionKey, retryAuth, connectivityConfirmed); + public static SetConnectingStateCommand Create(bool retryAuth = false, bool? connectivityConfirmed = null) => + new SetConnectingStateCommand(retryAuth, connectivityConfirmed); protected override string ExplainData() { @@ -204,13 +201,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 +217,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; } } @@ -452,21 +439,18 @@ internal class HandleConnectingErrorCommand : RealtimeCommand public AblyException Exception { get; } - public bool ClearConnectionKey { get; } - - private HandleConnectingErrorCommand(ErrorInfo error, AblyException ex, bool clearConnectionKey) + private HandleConnectingErrorCommand(ErrorInfo error, AblyException ex) { Error = error; Exception = ex; - ClearConnectionKey = clearConnectionKey; } - public static HandleConnectingErrorCommand Create(ErrorInfo error = null, AblyException ex = null, bool clearConnectionKey = false) => - new HandleConnectingErrorCommand(error, ex, clearConnectionKey); + public static HandleConnectingErrorCommand Create(ErrorInfo error = null, AblyException ex = null) => + new HandleConnectingErrorCommand(error, ex); protected override string ExplainData() { - return $"Error: {Error}. Exception: {Exception?.Message}. ClearConnectionKey: {ClearConnectionKey}"; + return $"Error: {Error}. Exception: {Exception?.Message}"; } } 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 45fc3f853..33f7105e0 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -479,9 +479,7 @@ private async Task ProcessCommandInner(RealtimeCommand command) } catch (AblyException e) { - return SetDisconnectedStateCommand.Create( - e.ErrorInfo, - clearConnectionKey: true) + return SetDisconnectedStateCommand.Create(e.ErrorInfo) .TriggeredBy(cmd); } } @@ -509,9 +507,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: @@ -525,9 +521,7 @@ async Task AttemptANewConnection() if (error.IsRetryableStatusCode()) { - return SetDisconnectedStateCommand.Create( - error, - clearConnectionKey: true) + return SetDisconnectedStateCommand.Create(error) .TriggeredBy(cmd); } else @@ -767,9 +761,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. @@ -795,7 +789,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) { @@ -880,18 +874,6 @@ private async Task HandleSetStateCommand(RealtimeCommand comman try { - if (cmd.ClearConnectionKey) - { - 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 @@ -973,16 +955,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); @@ -990,7 +968,6 @@ private async Task HandleSetStateCommand(RealtimeCommand comman finally { ClearAckQueueAndFailMessages(failedState.Error); - State.Connection.ClearKeyAndId(); // RTN8d, RTN9d ConnectionManager.DestroyTransport(); } @@ -1021,17 +998,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(); @@ -1143,8 +1113,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) { @@ -1157,11 +1127,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. @@ -1173,12 +1138,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. @@ -1194,7 +1153,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); @@ -1202,7 +1163,6 @@ async Task CheckInstantRetryFlag() finally { ClearAckQueueAndFailMessages(closedState.Error); - State.Connection.ClearKeyAndId(); // RTN8d, RTN9d ConnectionManager.DestroyTransport(); } diff --git a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs index f6b63c906..5a00c4458 100644 --- a/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs +++ b/src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs @@ -74,8 +74,12 @@ private void OnTimeOut() public override RealtimeCommand Connect() { + // No key to clear for RTN11b's clean connection: entering CLOSING already ran + // ClearKeyAndId for RTN8d and RTN9d, and the single reader processed that before it can + // reach this. The following CONNECTED therefore finds no id to match and restarts the + // serial sequence under RTN15c7. _timer.Abort(); - return SetConnectingStateCommand.Create(clearConnectionKey: true).TriggeredBy("ClosingState.Connect()"); + return SetConnectingStateCommand.Create().TriggeredBy("ClosingState.Connect()"); } } } 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/ChannelSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs index 17ecbc7c7..b13a6c670 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ChannelSpecs.cs @@ -1683,6 +1683,149 @@ public HistorySpecs(ITestOutputHelper output) } } + /// + /// channelSerial is the only continuity signal a reattach carries now that RTN15g and RTL4j + /// are deleted at 6.1.0, so its three invariants are pinned directly: RTL15b sets it, RTL15b2 + /// says which states clear it, and RTL4c1 puts it on the outgoing ATTACH. + /// + [Trait("spec", "RTL15b")] + public class ChannelSerialSpecs : ChannelSpecs + { + [Theory] + [InlineData(ChannelState.Suspended, true)] + [InlineData(ChannelState.Detached, false)] + [InlineData(ChannelState.Failed, false)] + [Trait("spec", "RTL15b2")] + public async Task OnAStateChange_ShouldClearTheSerialOnlyWhereRTL15b2SaysTo(ChannelState state, bool shouldRetain) + { + // 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)." Keeping it through SUSPENDED is what lets the reattach after + // an RTL4f attach timeout, or after the connection suspends and RTL3c suspends every + // channel, still ask to continue where it left off. + var (client, channel) = await GetClientAndChannel(); + await AttachWithChannelSerial(client, channel, "serial-1"); + + SetState(channel, state, state == ChannelState.Failed ? new ErrorInfo("failed") : null); + await client.ProcessCommands(); + + if (shouldRetain) + { + channel.Properties.ChannelSerial.Should().Be("serial-1"); + } + else + { + channel.Properties.ChannelSerial.Should().BeNull(); + } + } + + [Fact] + [Trait("spec", "RTL4c1")] + public async Task OnAReattach_TheAttachShouldCarryTheChannelSerial() + { + // RTL4c1 - "The ATTACH ProtocolMessage channelSerial field must be set to the RTL15b + // channelSerial." Reattaching out of SUSPENDED is the case that matters: the serial + // survived per RTL15b2, and putting it on the wire is the whole of what replaced + // ATTACH_RESUME. + var (client, channel) = await GetClientAndChannel(); + await AttachWithChannelSerial(client, channel, "serial-1"); + + SetState(channel, ChannelState.Suspended); + await client.ProcessCommands(); + LastCreatedTransport.SentMessages.Clear(); + + channel.Attach(); + await client.ProcessCommands(); + + var attach = SentProtocolMessages() + .Should().ContainSingle(x => x.Action == ProtocolMessage.MessageAction.Attach) + .Subject; + attach.ChannelSerial.Should().Be("serial-1"); + } + + [Fact] + [Trait("spec", "RTL4c1")] + public async Task WithNoSerialYet_TheAttachShouldNotInventOne() + { + // RTL4c1's second sentence - "If the RTL15b channelSerial is not set, the field may be + // set to null or omitted." A first attach has nothing to continue from. + var (client, channel) = await GetClientAndChannel(); + + channel.Attach(); + await client.ProcessCommands(); + + SentProtocolMessages() + .Should().ContainSingle(x => x.Action == ProtocolMessage.MessageAction.Attach) + .Subject.ChannelSerial.Should().BeNull(); + } + + [Fact] + [Trait("spec", "RTL4j")] + public async Task AcrossTheLifecycle_NoOutgoingMessageShouldSetAttachResume() + { + // RTL4j and its sub-clauses are deleted as of 6.1.0: "SDKs need not set ATTACH_RESUME + // any more". The Flag constant stays per TR3f, so nothing but a sweep of what actually + // goes out will notice it being set again - and the reattaches below are exactly the + // ones the deleted clauses used to demand it on. + var (client, channel) = await GetClientAndChannel(); + await AttachWithChannelSerial(client, channel, "serial-1"); + + // A reattach out of SUSPENDED, where the serial survives. + SetState(channel, ChannelState.Suspended); + await client.ProcessCommands(); + channel.Attach(); + await client.ProcessMessage(new ProtocolMessage(ProtocolMessage.MessageAction.Attached) + { + Channel = channel.Name, + ChannelSerial = "serial-2", + }); + await client.ProcessCommands(); + + // And one out of DETACHED, where it does not. + SetState(channel, ChannelState.Detached); + await client.ProcessCommands(); + channel.Attach(); + await client.ProcessCommands(); + + channel.Publish("name", "data"); + await client.ProcessCommands(); + + var sent = SentProtocolMessages().ToList(); + sent.Should().Contain( + x => x.Action == ProtocolMessage.MessageAction.Attach, + "the sweep has to have seen the ATTACHes for its result to mean anything"); + sent.Where(x => x.Flags.HasValue + && ((ProtocolMessage.Flag)x.Flags.Value).HasFlag(ProtocolMessage.Flag.AttachResume)) + .Should().BeEmpty(); + } + + private IEnumerable SentProtocolMessages() => + LastCreatedTransport.SentMessages.Select(x => x.Original).Where(x => x != null); + + /// + /// Attaches the channel and hands it an ATTACHED carrying a channelSerial, which is what + /// RTL15b reads. Asserting the serial landed makes RTL15b itself part of every case below. + /// + private async Task AttachWithChannelSerial(AblyRealtime client, IRealtimeChannel channel, string serial) + { + channel.Attach(); + await client.ProcessMessage(new ProtocolMessage(ProtocolMessage.MessageAction.Attached) + { + Channel = channel.Name, + ChannelSerial = serial, + }); + await client.ProcessCommands(); + + channel.State.Should().Be(ChannelState.Attached); + channel.Properties.ChannelSerial.Should().Be(serial, "RTL15b sets it from the ATTACHED"); + } + + public ChannelSerialSpecs(ITestOutputHelper output) + : base(output) + { + } + } + protected void SetState( IRealtimeChannel channel, ChannelState state, 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 7f2b141a3..125c10a05 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -1004,14 +1004,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); @@ -1020,19 +1025,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, @@ -1048,34 +1045,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(); }); }); @@ -1083,14 +1067,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 8b84f3c19..efd825298 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] @@ -1255,12 +1369,11 @@ public async Task OnAnUpdate_ShouldNotResendTheMessagesStillInFlight() } [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"); @@ -1430,28 +1543,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 34af4e1ac2219a49080c231e48162e2e294a6f8d Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 3 Sep 2026 10:52:25 +0100 Subject: [PATCH 08/13] 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 comes from ably/specification#511, which tombstones RTN16g2 and drops SUSPENDED from the states where createRecoveryKey returns null. Written here before that merged, 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 has behaved this way since 2.27.0. Co-Authored-By: Claude Opus 5 --- src/IO.Ably.Shared/Realtime/Connection.cs | 15 +++++-- .../Realtime/RealtimeChannels.cs | 5 ++- .../ConnectionRecoverySpecs.cs | 39 ++++++++++++++++++- 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Connection.cs b/src/IO.Ably.Shared/Realtime/Connection.cs index 5eae16db9..1d01fb5ed 100644 --- a/src/IO.Ably.Shared/Realtime/Connection.cs +++ b/src/IO.Ably.Shared/Realtime/Connection.cs @@ -178,13 +178,22 @@ private void HandleNetworkStateChange(NetworkState state) /// /// Connection#CreateRecoveryKey is an attribute composed of the connectionKey, messageSerial and channelSerials (RTN16g, RTN16g1, RTN16h). /// - /// recoveryKey. + /// + /// The recovery key, or where RTN16g3 calls for null. This SDK + /// returns empty strings rather than nulls for absent string values throughout, and callers + /// pass the result straight back as ClientOptions.Recover, which treats the two alike - so + /// returning null instead would break every consumer testing the result with IsNotEmpty for + /// no behavioural gain. ably-js returns null here. + /// 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 ea7d56607939d50da45e9d23972c90a29fc3c336 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 3 Sep 2026 17:54:34 +0100 Subject: [PATCH 09/13] 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 a93fe57254486689749fc25835400138fb954b8e Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 3 Sep 2026 19:57:16 +0100 Subject: [PATCH 10/13] 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 125c10a05..743a1727f 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -1004,7 +1004,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 d50443467..ab62343d1 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 efd825298..60f685eec 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] @@ -1388,11 +1418,31 @@ public async Task OnAFreshConnectionWithoutAnError_ShouldRestartTheSerialSequenc public async Task OnAFailedResume_ShouldRestartTheSerialSequence() { var client = await GetClientWithOneUnackedMessage(); + var originalId = client.State.Connection.Id; + + // 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)); + 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] @@ -1458,9 +1508,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")] @@ -1506,13 +1561,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)); @@ -1644,7 +1699,6 @@ public async Task ShouldUseTheConfiguredRealtimeRequestTimeout() .Should().ContainSingle().Which.Should().BeOfType(); } - // UTS: realtime/unit/RTN23a/idle-timeout-reconnect-1 [Fact] public async Task WhenIdleForLongerThanAllowed_ShouldDisconnect() { @@ -1664,6 +1718,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() { @@ -1738,6 +1838,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 }; From f3828dde73019623fbcb03e0245665678628e671 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Wed, 9 Sep 2026 11:14:57 +0100 Subject: [PATCH 11/13] Close four holes around the idle monitor and the request timeout TO3l11 - the lower bound was zero, which let a positive sub-millisecond value through. CountdownTimer hands the delay to System.Threading.Timer as (int)TotalMilliseconds, so anything under a millisecond truncates to a zero delay timer - the same hot loop a literal zero produces, and just as quiet. The bound is now one millisecond. RTN23b - the heartbeats guard was answered once and cached for the client's lifetime, on the belief that TransportParams is fixed at construction. It is not: ClientOptions.TransportParams is a mutable dictionary the client keeps a reference to, and TransportParams.Create reads it afresh for every transport. A cached answer can therefore describe a param the current transport never sent, arming the monitor against heartbeats nobody asked for or standing it down while they are being sent. Recomputed per tick; the cost is a scan of a dictionary that is normally empty. RTN23a - HeartbeatMonitorDelay is the granularity of idle detection but was an unvalidated public int, and the monitor driving it is a fire-and-forget loop with nothing observing the task. So a value it cannot wait on took detection out for the life of the client: zero is a hot loop queueing a command per scheduler tick, and minus one is Timeout.Infinite, which Task.Delay accepts as genuine infinity - the monitor ticks once and is then silent for good, nothing thrown and nothing logged. Below minus one it throws inside the loop, faulting the task just as quietly. The setter now rejects anything under a millisecond, matching the sibling knob above, and the loop body is wrapped so a monitor that stops for any reason we did not foresee says so instead of being inferred later from a connection that never notices it is dead. Rejecting rather than clamping because none of the three values can be what a caller meant, so silently substituting one would hide the mistake rather than surface it. RTN14h - the sandbox test named for a past-ttl reconnect was not reaching one. A live endpoint reconnects inside the ttl, so an implementation that restored the old RTN15g gate would still have passed. It now holds the attempts in CONNECTING until the ttl is spent and the client suspends - SUSPENDED being the state RTN14h names - then lets them complete, and asserts the key and id survive that. The connectionId is deliberately no longer asserted either way: whether the server still honours the resume after the ttl is its decision, and asserting it made the test depend on server retention rather than on client behaviour. Also states, on both sides, why RTN11d's connection level reset covers CLOSED and FAILED but not CLOSING: RTN11b asks only that channels be reinitialised from CLOSING, and the operations table maps that column to RTN11b rather than RTN11d. The previous wording said RTN11b "routes connect() through RTN11d", which reads as all of it. --- src/IO.Ably.Shared/ClientOptions.cs | 52 ++++++++++--- .../Realtime/RealtimeChannels.cs | 5 +- .../Realtime/Workflows/RealtimeWorkflow.cs | 59 +++++++++----- .../Realtime/ConnectionSandBoxSpecs.cs | 78 +++++++++++-------- .../ConnectionFailuresOnceConnectedSpecs.cs | 48 ++++++++++++ .../Realtime/RealtimeWorkflowSpecs.cs | 43 ++++++++++ 6 files changed, 222 insertions(+), 63 deletions(-) diff --git a/src/IO.Ably.Shared/ClientOptions.cs b/src/IO.Ably.Shared/ClientOptions.cs index 5c9e83591..c77f7fd90 100644 --- a/src/IO.Ably.Shared/ClientOptions.cs +++ b/src/IO.Ably.Shared/ClientOptions.cs @@ -296,17 +296,18 @@ public bool UseBinaryProtocol public TimeSpan ChannelRetryTimeout { get; set; } = Defaults.ChannelRetryTimeout; private TimeSpan _realtimeRequestTimeout = Defaults.RealtimeRequestTimeout; + private int _heartbeatMonitorDelay = 1000; /// /// 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. + /// Default: 10s. Must be at least one millisecond, 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 + /// when set below one millisecond, or to an /// interval too large for the underlying timers to fire. public TimeSpan RealtimeRequestTimeout { @@ -314,23 +315,25 @@ public TimeSpan 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 lower bound is one millisecond, not zero. CountdownTimer hands the delay to + // System.Threading.Timer as (int)TotalMilliseconds, so anything under a millisecond + // truncates to a zero delay timer - the same hot loop a literal zero produces, and + // just as quiet. A negative value stops the timer firing at all, and + // Timeout.InfiniteTimeSpan is -1ms, 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 outright. // // 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) + if (value < TimeSpan.FromMilliseconds(1) || value.TotalMilliseconds > int.MaxValue) { throw new ArgumentOutOfRangeException( nameof(RealtimeRequestTimeout), value, - "RealtimeRequestTimeout must be a positive interval. The default is 10s; " + + "RealtimeRequestTimeout must be at least one millisecond. 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."); @@ -435,7 +438,32 @@ public TimeSpan RealtimeRequestTimeout /// connection has been lost. /// Defaults: 1000. /// - public int HeartbeatMonitorDelay { get; set; } = 1000; + public int HeartbeatMonitorDelay + { + get => _heartbeatMonitorDelay; + + set + { + // This is the granularity of RTN23a idle detection, and the monitor driving it is a + // fire-and-forget loop, so a value it cannot wait on takes detection out for the life + // of the client. Zero is a hot loop, queueing a command per scheduler tick. Minus one + // is Timeout.Infinite, which Task.Delay accepts as genuine infinity, so the monitor + // ticks once and is then silent for good - nothing thrown, nothing logged. Below + // minus one it throws inside the loop instead. None of the three can be what a caller + // meant, so this rejects rather than clamping and quietly overriding them. + if (value < 1) + { + throw new ArgumentOutOfRangeException( + nameof(HeartbeatMonitorDelay), + value, + "HeartbeatMonitorDelay must be at least one millisecond. The default is 1000. " + + "It is how often RTN23a idle detection is evaluated, so a large value delays " + + "noticing a dead connection, and turning it off is not supported."); + } + + _heartbeatMonitorDelay = value; + } + } /// /// If enabled, every REST request to Ably includes a `request_id` query string parameter. diff --git a/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs b/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs index ccd4d46df..8eb82a1f0 100644 --- a/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs +++ b/src/IO.Ably.Shared/Realtime/RealtimeChannels.cs @@ -241,8 +241,9 @@ private void HandleInitialiseFailedChannelsCommand(RealtimeChannel channel) case ConnectionState.Closed: case ConnectionState.Failed: /* (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. + * errorReason unset (RTL24). CLOSING is included because RTN11b asks for the + * channel half there - "reinitialize channels per RTN11d" - and for that half + * only. The connection half stays CLOSED/FAILED, per RTN11d's own trigger. * * 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. diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index 33f7105e0..e9fd55bbe 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -35,9 +35,6 @@ internal sealed class RealtimeWorkflow : IQueueCommand, IDisposable private volatile bool _processingCommand; private bool _heartbeatMonitorDisconnectRequested; - // Null until first asked. See ProtocolHeartbeatsNotRequestedByCaller. - private bool? _protocolHeartbeatsNotRequested; - private bool _warnedIdleCheckInactive; private bool _disposedValue; @@ -111,10 +108,32 @@ public void Start() _ = Task.Run( async () => { - while (true) + var monitorToken = _heartbeatMonitorCancellationTokenSource.Token; + + try + { + while (true) + { + QueueCommand(HeartbeatMonitorCommand.Create(Now()).TriggeredBy("AblyRealtime.HeartbeatMonitor()")); + await Task.Delay(Client.Options.HeartbeatMonitorDelay, monitorToken); + } + } + catch (OperationCanceledException) + { + // Disposal. Not worth a line. + } + catch (Exception ex) { - QueueCommand(HeartbeatMonitorCommand.Create(Now()).TriggeredBy("AblyRealtime.HeartbeatMonitor()")); - await Task.Delay(Client.Options.HeartbeatMonitorDelay, _heartbeatMonitorCancellationTokenSource.Token); + // Nothing observes this task, so without the catch anything thrown here + // faults it silently and RTN23a detection is gone for the life of the client + // with no trace of why. HeartbeatMonitorDelay is validated on the way in, so + // reaching this means something unforeseen - which is precisely the case that + // needs to be visible rather than inferred from a connection that never + // notices it is dead. + Logger.Error( + "The RTN23a heartbeat monitor has stopped. Idle connection detection is " + + "off for the rest of this client's life.", + ex); } }, _heartbeatMonitorCancellationTokenSource.Token); @@ -367,6 +386,17 @@ private async Task ProcessCommandInner(RealtimeCommand command) // 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. + // + // Deliberately not CLOSING. RTN11d's trigger is CLOSED or FAILED, and RTN11b + // asks only that channels be reinitialised from CLOSING - the operations table + // maps that column to RTN11b, not RTN11d. So the channel command below covers + // CLOSING and this connection level reset does not. + // + // ably-js reaches the same place for CLOSING by a different route rather than + // by the same split: its reset lives in clearConnection, called on entering a + // terminal state, and closing is not one - so no msgSerial reset either. It is + // no guide to the rest of RTN11d though, since it clears errorReason only on + // reaching CONNECTED and never returns channels to INITIALIZED at all. if (State.Connection.State == ConnectionState.Closed || State.Connection.State == ConnectionState.Failed) { @@ -644,18 +674,11 @@ ErrorInfo GetErrorInfoFromTransportException(Exception ex, ErrorInfo @default) /// 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() - { + // Recomputed per tick rather than cached. ClientOptions.TransportParams is a mutable + // dictionary the client keeps a reference to, and TransportParams.Create reads it afresh + // for every transport - so a cached answer can describe a param the current transport + // never sent, arming the monitor against heartbeats nobody asked for or standing it down + // while they are being sent. The cost is a scan of a dictionary that is normally empty. var transportParams = Client.Options.TransportParams; if (transportParams == null) { diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs index 743a1727f..488a1fa42 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSandBoxSpecs.cs @@ -1009,26 +1009,36 @@ public async Task WhenDisconnectedMessageContainsTokenError_IfTokenRenewFails_Sh [Trait("spec", "RTN14h")] [Trait("spec", "RTN8d")] [Trait("spec", "RTN9d")] - [Trait("spec", "RTN15c6")] + [Trait("spec", "RTL3d")] 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). + // RTN14h, which replaces RTN15g as of specification 6.1.0 - the client keeps its + // connection state however long it has been disconnected, and still attempts a resume. + // + // The ttl has to actually elapse for this to be about RTN14h rather than about a brief + // reconnect, and a live endpoint reconnects too quickly for that on its own. So the + // attempts are held in CONNECTING until the ttl has passed and the client suspends - + // SUSPENDED being the state RTN14h names - and only then allowed to complete. + var holdInConnecting = false; + var transportFactory = new TestTransportFactory( + transport => transport.KeepInConnectingState = holdInConnecting); + var client = await GetRealtimeClient(protocol, (options, _) => { + options.TransportFactory = transportFactory; options.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(1000); - options.DisconnectedRetryTimeout = TimeSpan.FromMilliseconds(5000); + options.DisconnectedRetryTimeout = TimeSpan.FromMilliseconds(500); + options.SuspendedRetryTimeout = TimeSpan.FromMilliseconds(500); }); await client.WaitForState(ConnectionState.Connected); - string initialConnectionId = client.Connection.Id; - string initialConnectionKey = client.Connection.Key; + var initialConnectionId = client.Connection.Id; + var initialConnectionKey = client.Connection.Key; + initialConnectionKey.Should().NotBeNullOrEmpty(); // RTL3d - channels that were ATTACHED, ATTACHING or SUSPENDED are reattached on - // entering CONNECTED regardless of whether the resume succeeded. + // entering CONNECTED. var channels = new List { client.Channels.Get("attached".AddRandomSuffix()) as RealtimeChannel, @@ -1044,33 +1054,39 @@ public async Task WhenDisconnectedPastTTL_ShouldStillResume_AndReattachChannels( channels[1].State.Should().Be(ChannelState.Initialized); // set attaching later channels[2].State.Should().Be(ChannelState.Suspended); - string newConnectionId = string.Empty; + // A ttl short enough to elapse while the attempts below are being held open. + client.State.Connection.ConnectionStateTtl = TimeSpan.FromMilliseconds(2000); - await WaitFor(60000, done => - { - client.Connection.Once(ConnectionEvent.Disconnected, _ => - { - // RTN8d, RTN9d - DISCONNECTED is not a terminal state, so both survive. - client.Connection.Id.Should().Be(initialConnectionId); - client.Connection.Key.Should().Be(initialConnectionKey); + holdInConnecting = true; + client.GetTestTransport().Close(); // close event is suppressed by default + client.Workflow.QueueCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); - channels[1].Attach(); - client.Connection.Once(ConnectionEvent.Connected, _ => - { - newConnectionId = client.Connection.Id; - done(); - }); - }); + await client.WaitForState(ConnectionState.Disconnected); - client.GetTestTransport().Close(); // close event is suppressed by default - client.Workflow.QueueCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); - }); + // RTN8d, RTN9d - DISCONNECTED is not a terminal state, so both survive. + client.Connection.Id.Should().Be(initialConnectionId); + client.Connection.Key.Should().Be(initialConnectionKey); + + // Attached during the outage, so it is pending when the connection comes back - RTL3d + // reattaches ATTACHING, ATTACHED and SUSPENDED channels and deliberately leaves + // INITIALIZED ones alone. + channels[1].Attach(); + + // The held attempts time out until the ttl is spent, which is what takes us here. + await client.WaitForState(ConnectionState.Suspended, TimeSpan.FromSeconds(30)); + + // The point of RTN14h: past the ttl, in the state RTN15g used to clear state in, the + // key is still there to resume with. + client.Connection.Key.Should().Be(initialConnectionKey); + client.Connection.Id.Should().Be(initialConnectionId); + + holdInConnecting = false; - // RTN15c6 - the server still held the connection, so the resume succeeded and the - // connectionId comes back unchanged. - initialConnectionId.Should().NotBeNullOrEmpty(); - newConnectionId.Should().Be(initialConnectionId); + await client.WaitForState(ConnectionState.Connected, TimeSpan.FromSeconds(30)); + // Whether the server still honoured the resume is its decision, not ours, so the + // connectionId is deliberately not asserted either way. What matters is that the + // client got back and RTL3d reattached everything. await channels[0].WaitForAttachedState(); await channels[1].WaitForAttachedState(); await channels[2].WaitForAttachedState(); diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs index c93e32e72..9f70a1fc6 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFailuresOnceConnectedSpecs.cs @@ -444,6 +444,35 @@ public async Task WhenTheAuthCallbackFails_ShouldSetTheCauseNotOnlyTheInnerExcep error.ErrorInfo.Cause.Message.Should().Be("the underlying cause"); } + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(-2)] + [InlineData(int.MinValue)] + [Trait("spec", "RTN23a")] + public void WithANonPositiveHeartbeatMonitorDelay_ShouldReject(int milliseconds) + { + // The RTN23a monitor is a fire-and-forget loop, so a delay it cannot wait on disables + // idle detection for the life of the client. -1 is the quietest of the three: it is + // Timeout.Infinite, which Task.Delay accepts as genuine infinity, so the monitor ticks + // once and then never again with nothing thrown and nothing logged. + var options = new ClientOptions(ValidKey); + + Assert.Throws( + () => options.HeartbeatMonitorDelay = milliseconds); + } + + [Fact] + [Trait("spec", "RTN23a")] + public void WithTheSmallestUsableHeartbeatMonitorDelay_ShouldAccept() + { + var options = new ClientOptions(ValidKey); + + options.HeartbeatMonitorDelay = 1; + + options.HeartbeatMonitorDelay.Should().Be(1); + } + [Theory] [InlineData(0)] [InlineData(-1)] @@ -456,6 +485,25 @@ public void WithANonPositiveRealtimeRequestTimeout_ShouldReject(int seconds) () => options.RealtimeRequestTimeout = TimeSpan.FromSeconds(seconds)); } + [Fact] + [Trait("spec", "TO3l11")] + public void WithASubMillisecondRealtimeRequestTimeout_ShouldReject() + { + // Positive but below a millisecond is the same hazard as zero, and quieter: + // CountdownTimer hands the delay to System.Threading.Timer as (int)TotalMilliseconds, + // so it truncates to a zero delay timer and turns RTN14c into a hot loop. + var options = new ClientOptions(ValidKey); + + Assert.Throws( + () => options.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(0.5)); + Assert.Throws( + () => options.RealtimeRequestTimeout = TimeSpan.FromTicks(1)); + + // Exactly one millisecond is the smallest the timers can carry, so it is allowed. + options.RealtimeRequestTimeout = TimeSpan.FromMilliseconds(1); + options.RealtimeRequestTimeout.Should().Be(TimeSpan.FromMilliseconds(1)); + } + [Fact] [Trait("spec", "TO3l11")] public void WithARealtimeRequestTimeoutTooLargeForATimer_ShouldReject() diff --git a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs index 60f685eec..1cc8a866c 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs @@ -1980,6 +1980,49 @@ public async Task WhenTheCallerDoesNotAskForProtocolHeartbeats_ShouldStandDown( .Should().BeEmpty(); } + [Fact] + [Trait("spec", "RTN23b")] + public async Task WhenTheCallerChangesTransportParams_ShouldFollowTheChange() + { + // ClientOptions.TransportParams is a mutable dictionary the client keeps a reference + // to, and TransportParams.Create reads it again for every transport - so the guard + // has to track it rather than answer once. A cached answer arms the monitor against + // heartbeats the current transport never asked for, or stands it down while they + // are being sent. + var client = GetClientWithFakeTransport(opts => + { + opts.NowFunc = _now.ValueFn; + opts.RealtimeRequestTimeout = RequestTimeout; + opts.HeartbeatMonitorDelay = (int)TimeSpan.FromMinutes(10).TotalMilliseconds; + opts.TransportParams = new Dictionary { { "heartbeats", "false" } }; + }); + + 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("stood down while the caller's heartbeats=false stands"); + + // Driven stood-down first on purpose. The other direction cannot detect a stale + // answer: a disconnect latches _heartbeatMonitorDisconnectRequested, so the second + // tick returns nothing whether the guard was consulted again or not. + client.Options.TransportParams["heartbeats"] = "true"; + + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().ContainSingle("the current transport does ask for protocol heartbeats"); + } + [Theory] [InlineData("heartbeats", true)] [InlineData("heartbeats", "true")] From 4a5f218ee2dab27e7040d91a42054f1750e8320c Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Wed, 9 Sep 2026 11:43:47 +0100 Subject: [PATCH 12/13] Decide RTN23b from the transport's own params The guard asked ClientOptions.TransportParams whether protocol heartbeats had been requested. That is the wrong source: the params are rebuilt for every transport, and the dictionary is public and mutable, so a caller changing it retuned the monitor for a transport already on the wire. Armed against one that went out with heartbeats=false, the monitor measures against pings ClientWebSocket cannot see and disconnects a healthy connection; stood down against one that did ask, it never detects a dead one. ConnectionManager.CreateTransport now records the answer from the params the transport is actually built with, read after the merge, and the monitor consults that. Being per transport is the whole point, so it is stored per transport on the connection state. This deletes ComputeProtocolHeartbeatsNotRequested, whose job was to predict what DictionaryExtensions.Merge would do to the caller's entry - including the case-insensitive key match that lets "Heartbeats" displace ours while Ably reads neither. Asking the merged result answers all of that directly, so the reimplementation and its case analysis go, and the warning moves to transport creation where it fires once per transport rather than once per client. --- .../Realtime/Workflows/RealtimeState.cs | 10 +++ .../Realtime/Workflows/RealtimeWorkflow.cs | 72 ++----------------- .../Transport/ConnectionManager.cs | 18 +++++ .../Realtime/RealtimeWorkflowSpecs.cs | 58 +++++++++------ 4 files changed, 70 insertions(+), 88 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs index 85de01981..68884f0fd 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs @@ -48,6 +48,16 @@ public ConnectionData(List fallbackHosts) /// public TimeSpan? MaxIdleInterval { get; internal set; } + /// + /// Whether the current transport actually asked Ably for protocol heartbeats, read off + /// the query params it was built with. RTN23b guarantees them only for heartbeats=true, + /// and a caller's own TransportParams entry can displace ours - so without them Ably may + /// satisfy maxIdleInterval with websocket pings, which ClientWebSocket cannot observe + /// and RTN23a therefore cannot measure. Per transport, because the params are rebuilt + /// for each one. + /// + public bool ProtocolHeartbeatsRequested { get; internal set; } + /// /// Information relating to the transition to the current state, /// as an Ably ErrorInfo object. This contains an error code and diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index e9fd55bbe..ef1246d3e 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -298,8 +298,10 @@ private IEnumerable HandleHeartbeatMonitorCommand(HeartbeatMoni } // RTN23b - without protocol heartbeats Ably may satisfy maxIdleInterval with websocket - // ping frames, which this library cannot observe, leaving nothing to measure. - if (ProtocolHeartbeatsNotRequestedByCaller()) + // ping frames, which this library cannot observe, leaving nothing to measure. Read off + // the params this transport was built with, not off ClientOptions, which the caller can + // change after the fact; ConnectionManager.CreateTransport records it and warns. + if (connection.ProtocolHeartbeatsRequested == false) { return Enumerable.Empty(); } @@ -665,72 +667,6 @@ 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() - { - // Recomputed per tick rather than cached. ClientOptions.TransportParams is a mutable - // dictionary the client keeps a reference to, and TransportParams.Create reads it afresh - // for every transport - so a cached answer can describe a param the current transport - // never sent, arming the monitor against heartbeats nobody asked for or standing it down - // while they are being sent. The cost is a scan of a dictionary that is normally empty. - 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()) diff --git a/src/IO.Ably.Shared/Transport/ConnectionManager.cs b/src/IO.Ably.Shared/Transport/ConnectionManager.cs index ed699c7f8..929328183 100644 --- a/src/IO.Ably.Shared/Transport/ConnectionManager.cs +++ b/src/IO.Ably.Shared/Transport/ConnectionManager.cs @@ -77,6 +77,24 @@ public async Task CreateTransport(string host) } } + // RTN23b - taken from the params this transport is actually built with, so the + // RTN23a monitor measures against what went on the wire rather than against + // ClientOptions, which the caller can mutate at any time. Read after the merge, so + // a caller entry that displaced ours is already reflected however it was spelled. + var wireParams = transportParams.GetParams(); + var heartbeatsRequested = wireParams.TryGetValue("heartbeats", out var heartbeatsValue) + && heartbeatsValue.EqualsTo("true"); + Connection.InnerState.ProtocolHeartbeatsRequested = heartbeatsRequested; + + if (heartbeatsRequested == false) + { + Logger.Warning( + "This connection did not ask Ably for protocol heartbeats, so Ably may keep " + + "it alive with websocket pings, which this library cannot see. Idle " + + "connection detection is off and a silently dropped connection will not be " + + "detected. Set transportParams heartbeats to 'true' to enable it."); + } + var transport = GetTransportFactory().CreateTransport(transportParams); transport.Listener = this; Transport = transport; diff --git a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs index 1cc8a866c..63c02ec27 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/RealtimeWorkflowSpecs.cs @@ -1982,13 +1982,12 @@ public async Task WhenTheCallerDoesNotAskForProtocolHeartbeats_ShouldStandDown( [Fact] [Trait("spec", "RTN23b")] - public async Task WhenTheCallerChangesTransportParams_ShouldFollowTheChange() + public async Task WhenTheCallerChangesTransportParams_ShouldFollowTheNextTransport() { - // ClientOptions.TransportParams is a mutable dictionary the client keeps a reference - // to, and TransportParams.Create reads it again for every transport - so the guard - // has to track it rather than answer once. A cached answer arms the monitor against - // heartbeats the current transport never asked for, or stands it down while they - // are being sent. + // The decision is per transport, not per client and not per tick. A caller can + // mutate ClientOptions.TransportParams at any time, but the transport already open + // went out with whatever it went out with - so a change must not retune the monitor + // for the current transport, only for the next one built from it. var client = GetClientWithFakeTransport(opts => { opts.NowFunc = _now.ValueFn; @@ -1997,32 +1996,51 @@ public async Task WhenTheCallerChangesTransportParams_ShouldFollowTheChange() opts.TransportParams = new Dictionary { { "heartbeats", "false" } }; }); - client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Connected) - { - ConnectionId = "1", - ConnectionDetails = new ConnectionDetails - { - ConnectionKey = "connectionKey", - MaxIdleInterval = PromisedMaxIdleInterval, - }, - }); - + client.FakeProtocolMessageReceived(ConnectedWithMaxIdleInterval()); await client.WaitForState(ConnectionState.Connected); + LastCreatedTransport.Parameters.GetParams() + .Should().Contain(new KeyValuePair("heartbeats", "false")); + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) - .Should().BeEmpty("stood down while the caller's heartbeats=false stands"); + .Should().BeEmpty("this transport did not ask for protocol heartbeats"); - // Driven stood-down first on purpose. The other direction cannot detect a stale - // answer: a disconnect latches _heartbeatMonitorDisconnectRequested, so the second - // tick returns nothing whether the guard was consulted again or not. client.Options.TransportParams["heartbeats"] = "true"; + // Still stood down: the change cannot reach a transport that has already been built. + _now.Reset(_now.Value.Add(AllowedIdleTime).Add(TimeSpan.FromSeconds(1))); + (await client.Workflow.ProcessCommand(HeartbeatMonitorCommand.Create(_now.Value))) + .Should().BeEmpty("the open transport still went out with heartbeats=false"); + + // A new transport, which does carry the change. + client.ExecuteCommand(SetDisconnectedStateCommand.Create(ErrorInfo.ReasonDisconnected)); + await client.WaitForState(ConnectionState.Disconnected); + client.ExecuteCommand(SetConnectingStateCommand.Create()); + await client.ProcessCommands(); + + LastCreatedTransport.Parameters.GetParams() + .Should().Contain(new KeyValuePair("heartbeats", "true")); + + client.FakeProtocolMessageReceived(ConnectedWithMaxIdleInterval()); + 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("the current transport does ask for protocol heartbeats"); } + private static ProtocolMessage ConnectedWithMaxIdleInterval() => + new ProtocolMessage(ProtocolMessage.MessageAction.Connected) + { + ConnectionId = "1", + ConnectionDetails = new ConnectionDetails + { + ConnectionKey = "connectionKey", + MaxIdleInterval = PromisedMaxIdleInterval, + }, + }; + [Theory] [InlineData("heartbeats", true)] [InlineData("heartbeats", "true")] From ffb5cf1bf634bccc0952bb0ae47fb0d4680a0977 Mon Sep 17 00:00:00 2001 From: Andy Ford Date: Thu, 10 Sep 2026 13:17:05 +0100 Subject: [PATCH 13/13] Scope the RTN17j connectivity check to the fallback decision RTN17j - fixed, in the two places the check was doing a job that is not its own. The clause covers "the use of an alternative host", nothing wider. It gated the immediate retry itself, which is the divergence the earlier "Consult the fallback hosts on every attempt" commit declared and left in place. That cost the attempt twice over: a reconnect which was going to stay on the primary waited out a probe nothing asked for, and a probe failing while the realtime endpoint was fine cancelled the reconnect outright - deferring it to the RTB1 timer, up to disconnectedRetryTimeout of downtime where RTN15h3 says reconnect now. The grant and the host decision are now separate: RTN15a and RTN15h3 decide whether to retry at all, and the check runs only if the host chosen for that retry turns out to be a fallback. ably-js draws the line in the same place - checkConnectivity sits inside its fallback handler, and the primary attempt takes no precheck. And the token renewal path took no check at all. HandleConnectingTokenError builds a transport directly rather than queueing a CONNECTING - the connection is already in that state, and the renewed token has to be picked up by the next transport rather than by a re-transition - so it reached GetHost without the CONNECTING handler's gate and could open a transport against another datacenter on the strength of an earlier failure. Both paths now share one gated decision, ChooseHostForNextAttempt. One check per cycle now falls out of scoping the check correctly rather than needing an answer passed between commands, so SetConnectingStateCommand.ConnectivityConfirmed and the test that pinned its lifetime go with it. The instant retry is also queued now rather than returned. A returned command is processed inside the same batch one level deeper, and with the probe no longer vetoing the retry an endpoint that fails synchronously - a transport whose connect throws - recursed DISCONNECTED -> CONNECTING -> DISCONNECTED within that batch until the command loop's nesting guard tripped at six levels. That throw is logged and swallowed by the outer catch, so the batch was abandoned and the connection left in CONNECTING with no transport and no timer, never reaching RTB1 or the RTN14e deadline. Queueing restarts the level count, so the traversal is bounded by the instant retry budget, which is what is meant to bound it. Found by the live WhenInternetConnectionIsLost_WithoutOSNotification test and now also pinned by a unit test, for which FakeTransport gains ThrowOnConnect. Co-Authored-By: Claude Opus 5 --- .../Realtime/Workflows/RealtimeCommands.cs | 16 +- .../Realtime/Workflows/RealtimeWorkflow.cs | 138 ++++++++------- .../Infrastructure/FakeTransport.cs | 12 ++ .../ConnectionFallbackSpecs.cs | 167 +++++++++++++++--- 4 files changed, 229 insertions(+), 104 deletions(-) diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs index 175694aca..3f4455f4c 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs @@ -154,27 +154,19 @@ private CloseConnectionCommand() internal class SetConnectingStateCommand : RealtimeCommand { - private SetConnectingStateCommand(bool retryAuth, bool? connectivityConfirmed) + private SetConnectingStateCommand(bool retryAuth) { RetryAuth = retryAuth; - ConnectivityConfirmed = connectivityConfirmed; } public bool RetryAuth { get; } - /// - /// 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 retryAuth = false, bool? connectivityConfirmed = null) => - new SetConnectingStateCommand(retryAuth, connectivityConfirmed); + public static SetConnectingStateCommand Create(bool retryAuth = false) => + new SetConnectingStateCommand(retryAuth); protected override string ExplainData() { - return ConnectivityConfirmed.HasValue ? $"ConnectivityConfirmed: {ConnectivityConfirmed}" : string.Empty; + return RetryAuth ? "RetryAuth: true" : string.Empty; } } diff --git a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs index ef1246d3e..cc4118ac3 100644 --- a/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs +++ b/src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs @@ -530,7 +530,13 @@ private async Task ProcessCommandInner(RealtimeCommand command) async Task AttemptANewConnection() { - var host = AttemptsHelpers.GetHost(State, Client.Options.FullRealtimeHost()); + // Through the same gated decision as the CONNECTING handler. This path builds + // a transport directly rather than queueing a CONNECTING - the connection is + // already in that state and the renewed token has to be picked up by the next + // transport, not by a re-transition - so before, it reached GetHost with no + // RTN17j check at all and could open a transport against a fallback on the + // strength of an earlier failure. + var host = await ChooseHostForNextAttempt(); SetNewHostInState(host); await ConnectionManager.CreateTransport(host); @@ -667,6 +673,39 @@ ErrorInfo GetErrorInfoFromTransportException(Exception ex, ErrorInfo @default) return EmptyCommand.Instance; } + /// + /// The host for the next transport, with the connectivity check RTN17j requires before an + /// alternative host is used. + /// + /// + /// 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. Which host is a candidate + /// at all is RTN17i's business and GetHost's job - it returns to the primary whenever the + /// last host was a fallback. + /// + /// RTN17j scopes the check to "the use of an alternative host", so a candidate that is the + /// primary is taken without one: there is nothing to verify, and the probe would only add + /// latency to the reconnect. When the candidate is a fallback and the internet is + /// unreachable, the problem is not this host, so we stay on the primary rather than working + /// through fallbacks that cannot answer either. + /// + /// Shared with the token renewal path, which reaches CreateTransport without queueing a + /// CONNECTING and so cannot inherit the handler's own gate. + /// + private async Task ChooseHostForNextAttempt() + { + var defaultRealtimeHost = Client.Options.FullRealtimeHost(); + var candidateHost = AttemptsHelpers.GetHost(State, defaultRealtimeHost); + + if (candidateHost == defaultRealtimeHost) + { + return defaultRealtimeHost; + } + + return await Client.RestClient.CanConnectToAbly() ? candidateHost : defaultRealtimeHost; + } + private void SetNewHostInState(string newHost) { if (IsFallbackHost()) @@ -833,41 +872,7 @@ private async Task HandleSetStateCommand(RealtimeCommand comman try { - var defaultRealtimeHost = Client.Options.FullRealtimeHost(); - - // 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; - - // 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 = candidateHost; - } - + var connectingHost = await ChooseHostForNextAttempt(); SetNewHostInState(connectingHost); var connectingState = new ConnectionConnectingState(ConnectionManager, Logger); @@ -961,8 +966,7 @@ ErrorInfo TransformIfTokenErrorAndNotRetryable() .TriggeredBy(command); } - bool? connectivityAnswer = null; - var retryInstantly = await CheckInstantRetryFlag(); + var retryInstantly = CheckInstantRetryFlag(); var disconnectedState = new ConnectionDisconnectedState(ConnectionManager, cmd.Error, Logger) { @@ -1001,22 +1005,25 @@ ErrorInfo TransformIfTokenErrorAndNotRetryable() { 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. + // Queued, not returned. A returned command is processed inside the same + // batch, one level deeper - and now that the probe no longer vetoes the + // retry, an endpoint that fails synchronously (a transport whose connect + // throws) recurses DISCONNECTED -> CONNECTING -> DISCONNECTED within that + // batch until the loop's nesting guard trips. The guard throws, the outer + // catch logs and swallows it, and the batch is abandoned: no transport, no + // timer, and a connection left sitting in CONNECTING for good. Queueing + // restarts the level count, so the traversal is bounded by the instant + // retry budget above, which is what is meant to bound it. // - // 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); + // One check per cycle also falls out of scoping the RTN17j check + // correctly rather than passing an answer along: this handler no longer + // asks, and the CONNECTING behind it asks only if it settles on a + // fallback. + QueueCommand(SetConnectingStateCommand.Create().TriggeredBy(command)); + break; } - async Task CheckInstantRetryFlag() + bool CheckInstantRetryFlag() { if (cmd.RetryInstantly) { @@ -1046,22 +1053,19 @@ async Task CheckInstantRetryFlag() // 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) - { - // Remembered so the CONNECTING behind this command does not repeat it. - connectivityAnswer = await Client.RestClient.CanConnectToAbly(); - return connectivityAnswer.Value; - } - - return false; + // Not gated on the connectivity check. RTN17j scopes that check to "the + // use of an alternative host", and whether this attempt reaches one is + // decided later, by ChooseHostForNextAttempt, which takes the check where + // the clause actually asks for it. Gating here cost the attempt twice + // over: every reconnect that was going to stay on the primary waited out + // a probe RTN17j never asked for, and a probe that failed while the + // realtime endpoint was fine cancelled the reconnect outright, deferring + // it to the RTB1 timer - up to disconnectedRetryTimeout of downtime where + // RTN15h3 says reconnect now. + return cmd.Exception != null + || (cmd.Error != null && cmd.Error.IsRetryableStatusCode()) + || (State.Connection.State == ConnectionState.Connected + && cmd.Error?.IsTokenError != true); } break; diff --git a/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransport.cs b/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransport.cs index 0a98c0533..15e64e62a 100644 --- a/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransport.cs +++ b/src/IO.Ably.Tests.Shared/Infrastructure/FakeTransport.cs @@ -45,10 +45,22 @@ public FakeTransport(TransportParams parameters) public bool OnConnectChangeStateToConnected { get; set; } = true; + /// + /// Makes Connect throw, as a transport does when the endpoint cannot be reached at all. + /// Mirrors TestTransportWrapper.ThrowOnConnect, for tests that need the failure to happen + /// inside the command that created the transport rather than as a later event. + /// + public bool ThrowOnConnect { get; set; } + public void Connect() { DefaultLogger.Debug($"Connecting using: {Parameters.GetUri()}"); + if (ThrowOnConnect) + { + throw new Exception("Test transport failing on connect"); + } + ConnectCalled = true; if (OnConnectChangeStateToConnected) { diff --git a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs index ca95984cc..575effd55 100644 --- a/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs +++ b/src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs @@ -196,10 +196,11 @@ public async Task AfterAnExceptionDropsAConnectedTransport_ShouldTryThePrimaryBe [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. + // RTN17j asks for a connectivity check before an alternative host is used, and only + // there. One decision in the cycle needs the answer - whether this attempt may move off + // the primary - so one check is taken. Asking again to decide whether to retry at all + // would hold the workflow's single reader thread for a second MaxHttpOpenTimeout on + // every failing attempt, and would let the probe veto a retry RTN15h3 requires. var response = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(Defaults.InternetCheckOkMessage), @@ -231,41 +232,157 @@ public async Task WhenAnImmediateRetryIsGranted_ShouldCheckConnectivityOnceForTh [Fact] [Trait("spec", "RTN17j")] - public async Task WhenAConnectingCommandIsAbandoned_ShouldNotLeaveAnAnswerBehindForALaterAttempt() + [Trait("spec", "RTN15h3")] + public async Task WhenTheCandidateIsThePrimary_ShouldRetryImmediatelyWithoutCheckingConnectivity() { - // 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) + // RTN17j scopes the check to "the use of an alternative host". An attempt that is going + // to stay on the primary has nothing to verify, so it should not pay for a probe - and + // must not be cancelled by one, or a probe failing while the realtime endpoint is fine + // defers the RTN15h3 reconnect to the RTB1 timer. + var handler = new FakeHttpMessageHandler(_ => new HttpResponseMessage(HttpStatusCode.OK) { - Content = new StringContent(Defaults.InternetCheckOkMessage), - }; + Content = new StringContent("no"), + }); - var handler = new FakeHttpMessageHandler(response); - var client = GetClientWithFakeTransportAndMessageHandler(messageHandler: handler); + var client = GetClientWithFakeTransportAndMessageHandler( + opts => opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10), + handler); client.Options.SkipInternetCheck = false; await client.ConnectClient(); await client.ProcessCommands(); + handler.Requests.Clear(); - // 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 })); + // A non-token DISCONNECTED carrying no retryable status: RTN15h3 earns it an immediate + // reconnect, and with nothing fallback-worthy on record the candidate is the primary. + // Deliberately not a socket drop - that path arrives with retryInstantly already set by + // the caller, so it never reaches the decision under test. + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = new ErrorInfo("Something else went wrong", 50000), + }); + + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); + + handler.Requests + .Count(x => x.RequestUri.ToString().EqualsTo(Defaults.InternetCheckUrl)) + .Should().Be(0, "the primary needs no RTN17j check"); - next.Should().ContainSingle().Which.Should().BeOfType(); + // Immediately, not in ten minutes - and the unreachable probe did not veto it. + client.State.AttemptsInfo.InstantRetryCount.Should().Be(1); + LastCreatedTransport.Parameters.Host.Should().Be(Defaults.RealtimeHost); + } + [Fact] + [Trait("spec", "RTN17j")] + [Trait("spec", "RTN15h3")] + public async Task WhenTheCandidateIsAFallbackAndTheInternetIsDown_ShouldStayOnThePrimaryAndStillRetry() + { + // The check governs the host, not whether to reconnect. A failed probe means the fallback + // cannot be trusted to answer either, so the attempt stays on the primary - but it still + // happens, because RTN15h3 asks for it unconditionally. + var handler = new FakeHttpMessageHandler(_ => new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("no"), + }); + + var client = GetClientWithFakeTransportAndMessageHandler( + opts => opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10), + handler); + client.Options.SkipInternetCheck = false; + + await client.ConnectClient(); + await client.ProcessCommands(); handler.Requests.Clear(); - // A fresh attempt that carries no answer of its own must take its own check. - await client.Workflow.ProcessCommand(SetConnectingStateCommand.Create()); + // A 500-504 DISCONNECTED is fallback-worthy under RTN17f1, so the next attempt's + // candidate is a fallback domain. + client.FakeProtocolMessageReceived(new ProtocolMessage(ProtocolMessage.MessageAction.Disconnected) + { + Error = new ErrorInfo { StatusCode = HttpStatusCode.GatewayTimeout }, + }); - var checks = handler.Requests - .Count(x => x.RequestUri.ToString().EqualsTo(Defaults.InternetCheckUrl)); + await client.WaitForState(ConnectionState.Connecting); + await client.ProcessCommands(); - checks.Should().Be(1); + handler.Requests + .Count(x => x.RequestUri.ToString().EqualsTo(Defaults.InternetCheckUrl)) + .Should().Be(1, "a fallback candidate is exactly what RTN17j wants checked"); + + client.State.AttemptsInfo.InstantRetryCount.Should().Be(1, "the probe governs the host, not the retry"); + LastCreatedTransport.Parameters.Host.Should().Be(Defaults.RealtimeHost); + } + + [Fact] + [Trait("spec", "RTN15h3")] + [Trait("spec", "RTN17j")] + public async Task WhenEveryConnectThrows_ShouldSpendTheRetryBudgetAndSettleInDisconnected() + { + // The instant retry is queued, not returned. Returned, it is processed inside the same + // command batch one level deeper, so a transport whose connect throws recurses + // DISCONNECTED -> CONNECTING -> DISCONNECTED within that batch until the command loop's + // nesting guard trips. The guard throws, the outer catch logs and swallows it, and the + // batch is abandoned - leaving the connection in CONNECTING with no transport and no + // timer, never reaching RTB1 or the RTN14e deadline. Only reachable once the retry is no + // longer vetoed by the RTN17j probe, which is why nothing caught it before. + var client = GetClientWithFakeTransport(opts => + { + opts.AutoConnect = false; + opts.DisconnectedRetryTimeout = TimeSpan.FromMinutes(10); + }); + + FakeTransportFactory.InitialiseFakeTransport = t => t.ThrowOnConnect = true; + + client.Connect(); + await client.ProcessCommands(); + + // Bounded by the budget the retry is meant to be bounded by, not by the nesting guard. + var domainCount = 1 + client.State.Connection.FallbackHosts.Count; + client.State.AttemptsInfo.InstantRetryCount.Should().Be(domainCount); + + // And parked on the RTB1 timer rather than stranded mid-attempt. + client.Connection.State.Should().Be(ConnectionState.Disconnected); + } + + [Fact] + [Trait("spec", "RTN17j")] + public async Task WhenRenewingATokenMidAttempt_ShouldNotReachAFallbackWithoutACheck() + { + // The token renewal path builds a transport directly instead of queueing a CONNECTING - + // the connection is already in that state, and the renewed token has to be picked up by + // the next transport rather than by a re-transition. So it never inherited the CONNECTING + // handler's RTN17j gate, and with a fallback-worthy failure on record it would open a + // transport against another datacenter on the strength of that alone, with no check. + var renewed = new TokenDetails("renewed") { Expires = TestHelpers.Now().AddHours(1) }; + + var client = await GetConnectedClient( + opts => opts.UseBinaryProtocol = false, + request => request.Url.Contains("/keys") + ? renewed.ToJson().ToAblyJsonResponse() + : "no".ToAblyResponse()); + + // Set after construction, not through the options action: GetRealtimeClient stamps + // SkipInternetCheck back to true for unit tests once the action has run. + client.Options.SkipInternetCheck = false; + + // Seeded after connecting, because entering CONNECTED clears the attempt collection. + var attempt = new ConnectionAttempt(TestHelpers.Now()); + attempt.FailedStates.Add(new AttemptFailedState( + ConnectionState.Disconnected, + new ErrorInfo { StatusCode = HttpStatusCode.GatewayTimeout })); + client.State.AttemptsInfo.Attempts.Add(attempt); + + AttemptsHelpers.GetHost(client.State, Defaults.RealtimeHost) + .Should().BeOneOf(client.State.Connection.FallbackHosts, "the candidate must really be a fallback, or this proves nothing"); + + await client.Workflow.ProcessCommand(HandleConnectingTokenErrorCommand.Create( + new ErrorInfo { Code = ErrorCodes.TokenError, StatusCode = HttpStatusCode.Unauthorized })); + await client.ProcessCommands(); + + // The internet is unreachable, so the fallback candidate is declined and the renewed + // token goes out against the primary. + LastCreatedTransport.Parameters.Host.Should().Be(Defaults.RealtimeHost); } [Fact]