Skip to content

Commit d6ea2b8

Browse files
AndyTWFclaude
andcommitted
Always attempt a resume, and let the server decide continuity
RTN14h - implemented. Replaces RTN15g as of specification 6.1.0. The client discarded its connection state once connectionStateTtl had passed and reconnected fresh, throwing away a resume the server would still have honoured. DF1a settles the scope - the ttl is no longer used to decide whether to resume at all - so the gate is general rather than SUSPENDED-only, which is how ably-js reads it too. RTN27c - fixed. DISCONNECTED is a state where "if the library was previously connected, the next connect attempt will be an RTN15b resume attempt". Clearing the key on every failed attempt made that false from the second attempt onwards. RTN8d, RTN9d - fixed twice over. Both list only CLOSED, CLOSING and FAILED, so SUSPENDED must keep the key and id; it cleared them. And all three of the states they do name cleared after SetState, which is what emits the state change - inline, with no SynchronizationContext installed - so the application was told it had reached a terminal state while Connection.Key still read as a resumable key. The clear now happens before the transition, which also stops it depending on a finally. Only a listener reading during the transition could observe this, so it was inherited rather than introduced here. RTN15g1, RTN15g2, RTN15g3 - deleted at 6.1.0. HasConnectionStateTtlPassed and its tests go with them. The reattach RTN15g3 asked for is already unconditional under RTL3d. RTL4j, RTL4j1, RTL4j2 - deleted at 6.1.0; SDKs need not set ATTACH_RESUME. Safe only because RTL4c1 already sends channelSerial on ATTACH and RTL15b2 keeps it across a suspend, so the reattach still carries a continuity signal. The Flag constant stays, per TR3f. The now-dead clearConnectionKey parameter goes from SetDisconnectedStateCommand and SetSuspendedStateCommand. No caller sets it, and leaving it would let the violation back in unnoticed. SetConnectingStateCommand keeps its own, still used by ConnectionClosingState for RTN11b/RTN11d, where a clean connection is the intent. Two clauses the 6.1.0 edit left describing the old model. They are handled differently, and both deliberately: - RTN27d is not followed. It still calls SUSPENDED a state whose "next connect attempt is a clean connection (not a resume attempt)", which RTN14h, RTN8d, RTN9d and DF1a now all contradict; the commit that made those changes does not touch RTN27 at all. ably-js retains the key in SUSPENDED too, citing RTN8d/RTN9d and RTN14h for it. - RTN16g2 is followed. It says createRecoveryKey returns null in SUSPENDED, which sits oddly beside RTN9d now that a key exists there, but it is what the published spec says and CreateRecoveryKey still complies. ably-js returns a key instead, citing an RTN16g3 that "replaces RTN16g2 as of 6.1.0" and appears nowhere in the published spec. Both worth raising upstream rather than guessing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 204bd6e commit d6ea2b8

10 files changed

Lines changed: 201 additions & 434 deletions

src/IO.Ably.Shared/Realtime/RealtimeChannel.cs

Lines changed: 11 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,6 @@ internal class RealtimeChannel : EventEmitter<ChannelEvent, ChannelStateChange>,
2828
private readonly PushChannel _pushChannel;
2929
private int _retryCount = 0;
3030

31-
/// <summary>
32-
/// True when the channel moves to the @ATTACHED@ state, and False
33-
/// when the channel moves to the @DETACHING@ or @FAILED@ states.
34-
/// </summary>
35-
internal bool AttachResume { get; set; }
36-
3731
private int _decodeRecoveryInProgress;
3832

3933
// We use interlocked exchange because it is a thread safe way to read a variable
@@ -160,33 +154,22 @@ internal void ConnectionStateChanged(ConnectionStateChange connectionStateChange
160154
}
161155

162156
/*
163-
* (RTL3d, RTN19b, RTN15c6, RTN15c7, RTN15g3) On entering CONNECTED, every
164-
* channel that was attached or pending needs its ATTACH - or its DETACH - sent
165-
* again, because the previous transport will never answer.
157+
* On entering CONNECTED, every channel that was attached or pending needs its
158+
* ATTACH - or its DETACH - sent again; the previous transport will never answer.
166159
*
167160
* The two branches have different owners. RTL3d names ATTACHING, ATTACHED and
168-
* SUSPENDED and asks for an RTL4c attach. DETACHING is not in that list and is
169-
* not a state transition at all - it is RTN19b, "if there are any pending
170-
* channels i.e. in the ATTACHING or DETACHING state, the respective ATTACH or
171-
* DETACH message should be resent". ably-js splits it the same way, with
172-
* checkPendingState handling both pending operations and notifyState the
173-
* reattach.
161+
* SUSPENDED, and asks for an RTL4c attach. DETACHING is RTN19b instead, which
162+
* requires the respective ATTACH or DETACH of any pending channel to be resent.
163+
* ably-js splits it the same way, between checkPendingState and notifyState.
174164
*
175165
* RTL3d1 requires all of this to be applied before CONNECTED reaches external
176-
* listeners, and is why it lives here rather than in HandleConnectedCommand:
177-
* Connection.NotifyUpdate runs the internal handlers, which is what calls this,
178-
* before handing the emit to NotifyExternalClients.
179-
*
180-
* Unconditional, deliberately. This used to be gated on the connectionId having
181-
* changed, which silently skipped the RTN15g3 reattach: RTN15g empties
182-
* Connection.Id *before* the CONNECTING transition, so by the time CONNECTED
183-
* arrived the id being compared against had already gone and the channel
184-
* concluded nothing had changed. The channel stayed locally ATTACHED on a brand
185-
* new connection with no server-side attachment - permanently silent.
166+
* listeners, which is why it lives here: Connection.NotifyUpdate runs the
167+
* internal handlers, this among them, before the emit.
186168
*
187-
* Whether the connection was resumed belongs inside the ATTACH, in channelSerial
188-
* and the ATTACH_RESUME flag, rather than in a decision about whether to send
189-
* one at all. ably-js reattaches unconditionally for the same reason.
169+
* Unconditional, deliberately. Whether the connection was resumed belongs
170+
* inside the ATTACH, in the channelSerial RTL4c1 carries, rather than in a
171+
* decision about whether to send one at all. ably-js reattaches unconditionally
172+
* for the same reason.
190173
*/
191174
switch (State)
192175
{
@@ -371,11 +354,6 @@ ProtocolMessage CreateAttachMessage()
371354
message.SetModesAsFlags(Options.Modes);
372355
}
373356

374-
if (AttachResume)
375-
{
376-
message.SetFlag(ProtocolMessage.Flag.AttachResume);
377-
}
378-
379357
return message;
380358
}
381359
}
@@ -748,11 +726,9 @@ private void HandleStateChange(ChannelState state, ErrorInfo error, ProtocolMess
748726
break;
749727
case ChannelState.Detaching:
750728
AttachedAwaiter.Fail(new ErrorInfo("Channel transitioned to detaching", ErrorCodes.InternalError));
751-
AttachResume = false;
752729
break;
753730
case ChannelState.Attached:
754731
_retryCount = 0;
755-
AttachResume = true;
756732
break;
757733
case ChannelState.Detached:
758734
/* RTL13a check for unexpected detach */
@@ -792,7 +768,6 @@ an attempt to reattach the channel should be made immediately */
792768
break;
793769
case ChannelState.Failed:
794770
_retryCount = 0;
795-
AttachResume = false;
796771
AttachedAwaiter.Fail(error);
797772
DetachedAwaiter.Fail(error);
798773
Presence.ChannelDetachedOrFailed(error);

src/IO.Ably.Shared/Realtime/Workflows/RealtimeCommands.cs

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -204,13 +204,12 @@ protected override string ExplainData()
204204

205205
internal class SetDisconnectedStateCommand : RealtimeCommand
206206
{
207-
private SetDisconnectedStateCommand(ErrorInfo error, bool retryInstantly, bool skipAttach, Exception exception, bool clearConnectionKey)
207+
private SetDisconnectedStateCommand(ErrorInfo error, bool retryInstantly, bool skipAttach, Exception exception)
208208
{
209209
Error = error;
210210
RetryInstantly = retryInstantly;
211211
SkipAttach = skipAttach;
212212
Exception = exception;
213-
ClearConnectionKey = clearConnectionKey;
214213
}
215214

216215
public ErrorInfo Error { get; }
@@ -221,45 +220,36 @@ private SetDisconnectedStateCommand(ErrorInfo error, bool retryInstantly, bool s
221220

222221
public Exception Exception { get; }
223222

224-
public bool ClearConnectionKey { get; }
225-
226223
protected override string ExplainData()
227224
{
228225
return $"RetryInstantly: {RetryInstantly}" +
229226
"SkipAttach: " + SkipAttach +
230227
((Error != null) ? " Error: " + Error : string.Empty) +
231-
((Exception != null) ? " Exception: " + Exception.Message : string.Empty) +
232-
" ClearConnectionKey: " + ClearConnectionKey;
228+
((Exception != null) ? " Exception: " + Exception.Message : string.Empty);
233229
}
234230

235231
public static SetDisconnectedStateCommand Create(
236232
ErrorInfo error,
237233
bool retryInstantly = false,
238234
bool skipAttach = false,
239-
Exception exception = null,
240-
bool clearConnectionKey = false)
241-
=> new SetDisconnectedStateCommand(error, retryInstantly, skipAttach, exception, clearConnectionKey);
235+
Exception exception = null)
236+
=> new SetDisconnectedStateCommand(error, retryInstantly, skipAttach, exception);
242237
}
243238

244239
internal class SetSuspendedStateCommand : RealtimeCommand
245240
{
246-
private SetSuspendedStateCommand(ErrorInfo error, bool clearConnectionKey)
241+
private SetSuspendedStateCommand(ErrorInfo error)
247242
{
248243
Error = error;
249-
ClearConnectionKey = clearConnectionKey;
250244
}
251245

252246
public ErrorInfo Error { get; }
253247

254-
public bool ClearConnectionKey { get; }
255-
256-
public static SetSuspendedStateCommand Create(ErrorInfo error, bool clearConnectionKey = false) => new SetSuspendedStateCommand(error, clearConnectionKey);
248+
public static SetSuspendedStateCommand Create(ErrorInfo error) => new SetSuspendedStateCommand(error);
257249

258250
protected override string ExplainData()
259251
{
260-
var message = (Error != null) ? " Error: " + Error : string.Empty;
261-
message += " ClearConnectionKey:" + ClearConnectionKey;
262-
return message;
252+
return (Error != null) ? " Error: " + Error : string.Empty;
263253
}
264254
}
265255

src/IO.Ably.Shared/Realtime/Workflows/RealtimeState.cs

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -80,43 +80,6 @@ public ConnectionStateChange UpdateState(ConnectionStateBase state, ILogger logg
8080
return new ConnectionStateChange(connectionEvent, oldState, newState, state.RetryIn, ErrorReason);
8181
}
8282

83-
public bool HasConnectionStateTtlPassed(Func<DateTimeOffset> now)
84-
{
85-
if (ConfirmedAliveAt.HasValue == false)
86-
{
87-
// Nothing has ever been received on this client, so there is no connection
88-
// state to consider stale.
89-
return false;
90-
}
91-
92-
// RTN15g2 - the window is connectionStateTtl plus maxIdleInterval, measured from
93-
// the last known sign of activity from Ably rather than from when we left the
94-
// Connected state. A device that slept may only have left Connected moments ago
95-
// having last actually heard from Ably hours earlier.
96-
// Clamped at zero rather than coalesced, because nothing between the wire and here
97-
// validates the sign: TimeSpanJsonConverter will hand back a negative TimeSpan for a
98-
// negative number and Update assigns it as-is. A negative value would make the
99-
// subtraction below throw OverflowException, which is precisely the failure this
100-
// method was rewritten to remove - the throw escapes HandleSetStateCommand's
101-
// AblyException-only catch, gets logged and dropped by the command loop, and leaves
102-
// the client wedged in DISCONNECTED with no transport and no retry. The RTN23a
103-
// monitor already treats a non-positive interval as no promise at all.
104-
var maxIdleInterval = MaxIdleInterval > TimeSpan.Zero ? MaxIdleInterval.Value : TimeSpan.Zero;
105-
106-
if (ConnectionStateTtl >= TimeSpan.MaxValue - maxIdleInterval)
107-
{
108-
// The window cannot be represented, so it can never elapse.
109-
return false;
110-
}
111-
112-
// Deliberately a subtraction rather than ConfirmedAliveAt + window. Adding to a
113-
// DateTimeOffset throws ArgumentOutOfRangeException once the result runs past
114-
// DateTimeOffset.MaxValue, and that exception escaped into the command loop where
115-
// it was logged and dropped - silently abandoning whichever state transition was
116-
// in progress. Comparing two durations cannot fail that way.
117-
return now() - ConfirmedAliveAt.Value > ConnectionStateTtl + maxIdleInterval;
118-
}
119-
12083
public void Update(ConnectionInfo info, bool isUpdate)
12184
{
12285
// Guarded differently on purpose. connectionId is a top-level field and always

0 commit comments

Comments
 (0)