Move resumability decisions from the client to the server (spec 6.1.0)#2273
Open
SimonWoolf wants to merge 4 commits into
Open
Move resumability decisions from the client to the server (spec 6.1.0)#2273SimonWoolf wants to merge 4 commits into
SimonWoolf wants to merge 4 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 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>
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