Move resumability decisions from the client to the server (spec 6.1.0)#2273
Open
SimonWoolf wants to merge 6 commits into
Open
Move resumability decisions from the client to the server (spec 6.1.0)#2273SimonWoolf wants to merge 6 commits into
SimonWoolf wants to merge 6 commits into
Conversation
Implements specification version 6.1.0 (spec commit 2a0a0006). The client no longer decides whether continuity can be preserved on reconnection. It always attempts to resume and lets the server make the authoritative decision (surfaced via the `resumed` flag). - RTN8d/RTN9d: retain Connection#id and Connection#key through the SUSPENDED state; only clear them in the terminal states (CLOSED, CLOSING, FAILED). - RTN15g removed / RTN14h: drop checkConnectionStateFreshness(). The client no longer discards connection state based on how long it has been disconnected, so reconnection always attempts a resume. - RTL15b2: retain a channel's channelSerial through SUSPENDED (clear it only on DETACHED/FAILED), so the subsequent ATTACH carries it for the server's continuity decision. - RTL4j removed / TR3f: stop setting the (now deprecated) ATTACH_RESUME flag; remove the _attachResume tracking. The flag constant is retained in the protocol definitions. Also drop the now-unused ConnectionManager#lastActivity mirror (the transport keeps its own copy for the idle timer). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rework or remove the tests that asserted the pre-6.1.0 client behaviour, matching the source changes for spec commit 2a0a0006: - resume.test.js: replace no_resume_once_suspended and no_resume_last_activity (RTN15g) with resume_after_suspended (RTN14h), which checks the client still attempts a resume after being suspended. - connection.test.js: remove stale_connection_resets_msgSerial_and_sendAttempted, which exercised the now-deleted checkConnectionStateFreshness path. - UTS connection_id_key: RTN8c/RTN9c "id/key null in SUSPENDED" becomes RTN8d/RTN9d "id/key retained in SUSPENDED" (now connects first, then suspends). - UTS channel_properties: RTL15b1 "channelSerial cleared on suspended" becomes RTL15b2 "channelSerial retained in suspended"; the detach/failed cases are relabelled RTL15b2. - UTS channel_attach: RTL4j "ATTACH_RESUME flag on reattach" becomes "ATTACH_RESUME flag not set on reattach". - private_api_recorder: drop the now-unused connectionManager.lastActivity entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two more UTS unit tests asserted the pre-6.1.0 behaviour: - connection_failures: RTN15g "no resume after connectionStateTtl expires" becomes RTN14h "reconnection still attempts resume after connectionStateTtl expires" — verifying every post-suspension reconnect carries the resume param with the original connectionKey. - connection_recovery: RTN16g2 "createRecoveryKey returns null in SUSPENDED" becomes "returns a key in SUSPENDED" — since connectionKey is now retained through SUSPENDED (RTN8d/RTN9d) the connection remains recoverable there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughConnection suspension now retains resume identifiers and retries with resume parameters. Channel suspension retains ChangesResume continuity behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionManager
participant Transport
Client->>ConnectionManager: transition to SUSPENDED
ConnectionManager->>Transport: reconnect with resume key
Transport-->>ConnectionManager: connection result
ConnectionManager-->>Client: connection state update
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
temporarily deployed
to
staging/pull/2273/bundle-report
July 22, 2026 18:42
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/uts/realtime/unit/connection/connection_id_key.test.ts`:
- Around line 329-335: Update the RTN8d/RTN9d lifecycle comment to remove
CLOSING from the terminal-state list, stating that connection identifiers are
cleared only in CLOSED and FAILED while retained through CLOSING and SUSPENDED.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 241d9ab9-7e80-42af-bd02-41766b8fe093
📒 Files selected for processing (12)
ably.d.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/transport/connectionmanager.tssrc/common/lib/transport/transport.tstest/common/modules/private_api_recorder.jstest/realtime/connection.test.jstest/realtime/resume.test.jstest/uts/realtime/unit/channels/channel_attach.test.tstest/uts/realtime/unit/channels/channel_properties.test.tstest/uts/realtime/unit/connection/connection_failures.test.tstest/uts/realtime/unit/connection/connection_id_key.test.tstest/uts/realtime/unit/connection/connection_recovery.test.ts
💤 Files with no reviewable changes (2)
- test/common/modules/private_api_recorder.js
- test/realtime/connection.test.js
Comment on lines
+329
to
+335
| * RTN8d, RTN9d - ID and key are retained in the SUSPENDED state | ||
| * | ||
| * As of specification version 6.1.0 (RTN8d/RTN9d, replacing RTN8c/RTN9c) the | ||
| * connection id and key are cleared only in the terminal states (CLOSED, | ||
| * CLOSING, FAILED). They are retained through SUSPENDED, since the client | ||
| * always attempts to resume on reconnecting and lets the server decide | ||
| * whether continuity can be preserved. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove CLOSING from the terminal-state list.
closing is non-terminal; identifiers are cleared only on closed and failed. The current comment misstates the lifecycle contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/uts/realtime/unit/connection/connection_id_key.test.ts` around lines 329
- 335, Update the RTN8d/RTN9d lifecycle comment to remove CLOSING from the
terminal-state list, stating that connection identifiers are cleared only in
CLOSED and FAILED while retained through CLOSING and SUSPENDED.
The spec-side change introduces RTN16g3 (replacing RTN16g2) to say createRecoveryKey() no longer returns null in SUSPENDED. Match the label in the ably-js UTS test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
temporarily deployed
to
staging/pull/2273/bundle-report
July 22, 2026 18:51
Inactive
Three presence tests encoded the pre-6.1.0 assumption that becoming suspended discards the connection state, so that reconnecting makes a fresh connection with a new connectionId and re-syncs presence from scratch. Since 6.1.0 the client retains its connection state through suspended and resumes on reconnect (RTN8d/RTN14h), so a local becomeSuspended followed by connect now resumes with the same connectionId and preserves presence continuity. Add an optional clearConnectionState flag to the becomeSuspended test helper. When set, it calls connectionManager.clearConnection() once suspended, modelling the case where the connection genuinely can no longer be resumed (e.g. the device slept past the server's retention), so the reconnect makes a fresh connection with a new connectionId. - presence_auto_reenter (RTP17i) and presence_auto_reenter_different_connid (RTP17g/g1): use the flag, since both rely on a fresh connection — the former to force own-member re-entry, the latter to get a changed connectionId. Without it the successful resume preserved presence and the tests' premises no longer held (timeout / connId unchanged). - suspended_preserves_presence (RTP5f/RTP11d): left on the genuine resume path (this test is about the members map surviving suspension). The final member count is now 2 (main + continuous) rather than 3: the resume preserves both cleanly and the only change is the leave of 'leaves', consistent with the single leave event the test already asserts. The old fresh-reconnect re-entered main, leaving a residual that showed 3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
temporarily deployed
to
staging/pull/2273/bundle-report
July 23, 2026 18:18
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/common/modules/shared_helper.js`:
- Around line 283-288: Ensure the clearConnectionState path clears the
connection before the suspended state notification is emitted, so reconnect
callbacks cannot use stale recovery credentials. Update the relevant
ConnectionManager state-transition logic, such as enactStateChange or
_becomeSuspended, and preserve the existing conditional behavior and private API
recording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31325e26-c3e1-4243-95b5-7be26641e6f7
📒 Files selected for processing (2)
test/common/modules/shared_helper.jstest/realtime/presence.test.js
Comment on lines
283
to
+288
| self.recordPrivateApi('call.connectionManager.notifyState'); | ||
| realtime.connection.connectionManager.notifyState({ state: 'suspended' }); | ||
| if (clearConnectionState) { | ||
| self.recordPrivateApi('call.connectionManager.clearConnection'); | ||
| realtime.connection.connectionManager.clearConnection(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)test/common/modules/shared_helper\.js$|connectionManager|connect|clearConnection|notifyState'
echo "== file outline =="
ast-grep outline test/common/modules/shared_helper.js --view compact || true
echo "== relevant lines =="
nl -ba test/common/modules/shared_helper.js | sed -n '240,330p'
echo "== search connectionManager definitions/usages =="
rg -n "connectionManager|clearConnection|notifyState|connectionState|suspended|clearConnectionState" test/common/modules/shared_helper.js -C 3Repository: ably/ably-js
Length of output: 1980
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared_helper relevant section =="
awk 'NR>=250 && NR<=315 { printf "%5d\t%s\n", NR, $0 }' test/common/modules/shared_helper.js
echo "== connectionmanager outline/sections =="
wc -l src/common/lib/transport/connectionmanager.ts
grep -n "class ConnectionManager\|clearConnection\|notifyState\|emit\|eventEmitter\|stateChange\|tryTransition" src/common/lib/transport/connectionmanager.ts | head -200
echo "== implementation sections =="
awk 'NR>=1 && NR<=220 { printf "%5d\t%s\n", NR, $0 }' src/common/lib/transport/connectionmanager.tsRepository: ably/ably-js
Length of output: 15400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== clearConnection implementation =="
awk 'NR>=820 && NR<=905 { printf "%5d\t%s\n", NR, $0 }' src/common/lib/transport/connectionmanager.ts
echo "== enactStateChange implementation =="
awk 'NR>=920 && NR<=970 { printf "%5d\t%s\n", NR, $0 }' src/common/lib/transport/connectionmanager.ts
echo "== notifyState implementation =="
awk 'NR>=1136 && NR<=1260 { printf "%5d\t%s\n", NR, $0 }' src/common/lib/transport/connectionmanager.ts
echo "== relevant callback listeners =="
awk 'NR>=1280 && NR<=1365 { printf "%5d\t%s\n", NR, $0 }' src/common/lib/transport/connectionmanager.ts
echo "== call sites around becomeSuspended tests usage =="
rg -n "becomeSuspended|clearConnectionState|notifyState\\(\\{ state: 'suspended'|clearConnection\\(" test src/common -C 2Repository: ably/ably-js
Length of output: 22170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resume tests around becomeSuspended with clearConnectionState =="
awk 'NR>=470 && NR<=580 { printf "%5d\t%s\n", NR, $0 }' test/realtime/resume.test.js
echo "== static behavioral probe for _becomeSuspended ordering =="
node - <<'JS'
const events = [];
const realtime = {
connection: {
once(event, listener) {
events.push({ event, type: 'listener registered', eventListeners: realtime.connection._listeners?.[event] || [] });
realtime.connection._listeners = realtime.connection._listeners || {};
(realtime.connection._listeners[event] = realtime.connection._listeners[event] || []).push(listener);
},
emit(event, ...args) {
events.push({ event, type: 'emit', arg: args[0], listeners: realtime.connection._listeners?.[event]?.length || 0 });
for (const listener of realtime.connection._listeners?.[event] || []) listener(...args);
},
_listeners: {},
},
connectionManager: {
clearConnectionState: false,
disconnectAllTransports() {},
notifyState(ind) {
if (ind.state === 'suspended') {
this.emit('disconnected');
this.emit='suspended;';
this.inSuspendedCallback = true;
}
},
emit(...args){}
},
};
let cbCalledWithIdKey;
function clearConnectionStateWithSameValues() {
realtime.becameSuspended = true;
}
try {
const helperPrototype = { addingHelperFunction: () => ({ disconnectAllTransports() {}, _becomeSuspended(realtime, cb, clearConnectionState) {
realtime.connection.connectionManager.disconnectAllTransports();
const self = this;
self.recordPrivateApi = () => {};
realtime.connection.once('disconnected', function () {
self.recordPrivateApi('call.connectionManager.notifyState');
realtime.connection.connectionManager.notifyState({ state: 'suspended' });
if (clearConnectionState) {
self.recordPrivateApi('call.connectionManager.clearConnection');
realtime.connection.connectionManager.clearConnection();
}
});
if (cb) {
realtime.connection.once('suspended', function () {
cb();
});
}
}}); }
}
JS
echo "== behavioral probe from source text =="
python3 - <<'PY'
from pathlib import Path
source = Path("src/common/lib/transport/connectionmanager.ts").read_text()
notify = source[source.index(" notifyState(indicated: ConnectionState): void {"):source.index(" requestState(request: any): void {")]
print("notifyState calls ConnectionManager.emitter emit:", "this.emit('connectionstate'" in notify)
print("clearConnection only invoked for terminal states in enactStateChange:", "this.clearConnection()" in source[source.index(" enactStateChange"):source.index(" startTransitionTimer")])
print("suspended state non-terminal:", "terminal: false,\n state: 'suspended'," in source)
print("suspended failState is suspended:", source[source.index("suspended"):].split("\n")[9])
PYRepository: ably/ably-js
Length of output: 5128
Clear state before notifying suspended.
For clearConnectionState: true, notifyState({ state: 'suspended' }) enacts suspended, emits connectionstate synchronously, then clearConnection() runs in _becomeSuspended, so pending reconnect callbacks can still use the old recovery credentials. Move the clearing into ConnectionManager.enactStateChange() with non-terminal handling before emitting connectionstate, or clear in becomeSuspended before notifying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/common/modules/shared_helper.js` around lines 283 - 288, Ensure the
clearConnectionState path clears the connection before the suspended state
notification is emitted, so reconnect callbacks cannot use stale recovery
credentials. Update the relevant ConnectionManager state-transition logic, such
as enactStateChange or _becomeSuspended, and preserve the existing conditional
behavior and private API recording.
The ably-js translation of the proxy connection_resume UTS still asserted the pre-6.1.0 RTN15g behaviour: that the reconnection made after the connectionStateTtl expired carried no resume param. Under RTN14h the client always attempts to resume, so that reconnection now carries the resume param (the server, having discarded the state, then responds with a new connectionId). Rework the test (RTN15g/g2 -> RTN14h) to assert the final ws_connect carries resume=<connectionKey>. Verified against the real uts-proxy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements specification version 6.1.0 (spec commit
2a0a0006, ably/specification#488).The client no longer decides whether continuity can be preserved on reconnection. It always attempts to resume and lets the server make the authoritative decision (surfaced via the
resumedflag on the subsequentattached/connection state change).Behaviour changes
Connection#idandConnection#keyare now cleared only in the terminal states (CLOSED,CLOSING,FAILED); they are retained throughSUSPENDED.checkConnectionStateFreshness(). The client no longer discards connection state based on how long it has been disconnected, so reconnection (including out ofSUSPENDED) always attempts a resume.channelSerialis retained throughSUSPENDED(cleared only onDETACHED/FAILED), so the subsequentATTACHcarries it (RTL4c1) for the server's continuity decision.ATTACH_RESUMEflag; the_attachResumetracking is removed. The flag constant is retained in the protocol definitions.Also updates the
SUSPENDED/DISCONNECTEDdocstrings inably.d.ts, and drops the now-unusedConnectionManager#lastActivitymirror (the transport keeps its own copy for the idle timer).Tests
Reworked or removed the tests that asserted the pre-6.1.0 behaviour, and added positive coverage for the new behaviour (id/key and channelSerial retained through
SUSPENDED, reconnects still carrying the resume param after the TTL,ATTACH_RESUMEnot set,createRecoveryKeyreturning a key inSUSPENDED).Follow-up in the spec repo
A companion PR is needed in ably/specification to bring the shared UTS
.mdtest-spec sources in line with 6.1.0 — spec PR #488 updatedfeatures.md/api-docstrings.mdbut not the UTS sources. That PR should also fix RTN16g2, which still sayscreateRecoveryKey()returnsNullinSUSPENDED; sinceconnectionKeyis now retained there, this implementation returns a valid recovery key inSUSPENDEDinstead.🤖 Generated with Claude Code
Summary by CodeRabbit