[AIT-1274] LiveObjects: fail get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1) - #2284
Open
sacOO7 wants to merge 2 commits into
Open
[AIT-1274] LiveObjects: fail get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1)#2284sacOO7 wants to merge 2 commits into
get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1)#2284sacOO7 wants to merge 2 commits into
Conversation
A get() parked waiting for the objects sync state to reach SYNCED never rejected when the channel entered DETACHED/SUSPENDED/FAILED, leaving the returned promise unsettled forever (e.g. a solicited detach, or a connection failure moving a SUSPENDED channel to FAILED after RTO27a has cleared the objects data). publishAndApply already failed deterministically in the identical wait (RTO20e1); get() had no equivalent. Fail parked sync waiters from actOnChannelState via a new internal-only syncWaitFailed event, shared by get() (RTO23c1) and publishAndApply (RTO20e1) through _waitForSyncedOrChannelFailure, which builds the 92008 ErrorInfo with a caller-specific message prefix. notifyState invokes actOnChannelState before assigning channel.errorReason, so the state-change reason is forwarded as an argument (as the presence handler already does) to preserve the error cause.
RTO5a5: an OBJECT_SYNC with no channelSerial is a single-message sync (data applied, sync completes SYNCED). RTO5a6: a present-but-malformed channelSerial (no ':' separator) is handled as if absent per RTO5a5. Both derive from the corresponding new UTS unit spec cases; the existing implementation already conforms.
WalkthroughThe change propagates channel transition reasons to realtime objects. ChangesRealtime object synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RealtimeChannel
participant RealtimeObject
participant SyncWaiter
RealtimeChannel->>RealtimeObject: notifyState(state, reason)
RealtimeObject->>SyncWaiter: emit syncWaitFailed(state, reason)
SyncWaiter-->>RealtimeObject: reject with 92008 and cause
RealtimeObject-->>RealtimeChannel: return rejected operation
Possibly related PRs
Suggested reviewers: 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 |
![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 `@src/plugins/liveobjects/realtimeobject.ts`:
- Around line 621-626: Update the ErrorInfo construction in the object-sync
failure helper to accept caller-specific remediation and include it in the new
ErrorInfo. For the channel.object.get() path, provide remediation that guides
callers without suggesting a retry of publishAndApply(), since its publish
acknowledgment has already completed; preserve existing behavior for other
callers.
🪄 Autofix
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: 115277c0-a82d-49ce-8682-086112ba3776
📒 Files selected for processing (4)
src/common/lib/client/realtimechannel.tssrc/plugins/liveobjects/realtimeobject.tstest/uts/objects/unit/objects_pool.test.tstest/uts/objects/unit/realtime_object.test.ts
Comment on lines
+621
to
+626
| new this._client.ErrorInfo( | ||
| `${failureDescription} due to the channel entering the ${state} state whilst waiting for objects sync to complete`, | ||
| 92008, | ||
| 400, | ||
| reason || undefined, | ||
| ), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a remediation for the public channel.object.get() failure.
channel.object.get() can reject with this new ErrorInfo, but the error has no remediation. Pass caller-specific remediation into the helper. Do not instruct callers to retry publishAndApply(), because its publish ACK has already completed.
Proposed fix
- await this._waitForSyncedOrChannelFailure('the object could not be retrieved'); // RTO23c1
+ await this._waitForSyncedOrChannelFailure(
+ 'the object could not be retrieved',
+ 'Call channel.attach() before retrying channel.object.get().',
+ ); // RTO23c1
- private _waitForSyncedOrChannelFailure(failureDescription: string): Promise<void> {
+ private _waitForSyncedOrChannelFailure(failureDescription: string, remediation?: string): Promise<void> {
return new Promise<void>((resolve, reject) => {
// ...
- new this._client.ErrorInfo(
- `${failureDescription} due to the channel entering the ${state} state whilst waiting for objects sync to complete`,
- 92008,
- 400,
- reason || undefined,
- ),
+ new this._client.ErrorInfo({
+ message: `${failureDescription} due to the channel entering the ${state} state whilst waiting for objects sync to complete`,
+ code: 92008,
+ statusCode: 400,
+ cause: reason || undefined,
+ remediation,
+ }),As per coding guidelines, “Add a concrete remediation to every publicly reachable SDK-originating throw site when it provides actionable value beyond the message.”
🤖 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 `@src/plugins/liveobjects/realtimeobject.ts` around lines 621 - 626, Update the
ErrorInfo construction in the object-sync failure helper to accept
caller-specific remediation and include it in the new ErrorInfo. For the
channel.object.get() path, provide remediation that guides callers without
suggesting a retry of publishAndApply(), since its publish acknowledgment has
already completed; preserve existing behavior for other callers.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
This PR fixes a LiveObjects RealtimeObject.get() deadlock by ensuring the sync-wait rejects deterministically when the underlying channel becomes unusable (DETACHED/SUSPENDED/FAILED), aligning get() behavior with publishAndApply and the updated RTO23c1/RTO20e1 spec requirements.
Changes:
- Emit an internal-only
syncWaitFailedsignal on channel transitions to DETACHED/SUSPENDED/FAILED and race it againstsyncedvia a shared_waitForSyncedOrChannelFailurehelper. - Forward the channel state-change
reasoninto the LiveObjects state handler so the rejectioncausecan be preserved even beforeRealtimeChannel.errorReasonis assigned. - Unskip/add UTS coverage for the new failure behavior and add OBJECT_SYNC
channelSerialhandling tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/uts/objects/unit/realtime_object.test.ts | Adds RTO23c1 tests asserting get() rejects during sync-wait when the channel enters DETACHED/SUSPENDED/FAILED (including cause on FAILED). |
| test/uts/objects/unit/objects_pool.test.ts | Adds RTO5a5/RTO5a6 tests for OBJECT_SYNC behavior when channelSerial is absent or malformed. |
| src/plugins/liveobjects/realtimeobject.ts | Implements internal failure signaling and shared sync-wait helper; refactors get() and publishAndApply to reject on channel failure states with 92008/400 and appropriate cause. |
| src/common/lib/client/realtimechannel.ts | Passes state-change reason into the LiveObjects channel-state handler so failures can propagate accurate causes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+374
to
+381
| // RTO5a5 - no channelSerial: the whole sync is contained in this one message, so the | ||
| // objects are applied and the sync completes (SYNCED) without waiting for a cursor-empty | ||
| // channelSerial (RTO5a4) | ||
| ws.active_connection!.send_to_client( | ||
| buildObjectSyncMessage(msg.channel, null as any, [ | ||
| buildObjectState('counter:new@1000', { aaa: 't:0' }, { counter: { count: 99 } }), | ||
| ]), | ||
| ); |
sacOO7
added a commit
to ably/ably-java
that referenced
this pull request
Aug 9, 2026
…nd add their UTS unit tests - RTO23c1: a get() parked waiting for objects sync now fails when the channel enters DETACHED/SUSPENDED/FAILED — ensureSynced routes through the shared pendingSyncWaiters, each waiter carrying a caller-specific failure description (the object could not be retrieved vs RTO20e1's operation could not be applied locally), built into the 92008/400/cause error at the failure site. - RTO5a6: a malformed OBJECT_SYNC channelSerial (no ':' separator) is normalized to null so it takes the same branch as an absent serial (RTO5a5), with a warning logged. - Add the five UTS unit tests derived from the new spec cases (3x RTO23c1 per channel state, RTO5a5, RTO5a6). - Annotate the implementation sites of the newly specified points (RTO20d4, RTLC14c, RTLM22c). Spec changes: ably/specification#514 Companion ably-js fix: ably/ably-js#2284
sacOO7
changed the title
get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1)get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1)
sacOO7
changed the title
get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1)get()'s sync wait when the channel enters DETACHED/SUSPENDED/FAILED (RTO23c1)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/uts/objects/unit/objects_pool.test.ts:409
- RTO5a6 also requires a warning for a present-but-malformed
channelSerial, but this test only checks that sync completes._parseSyncChannelSerialcurrently silently maps this value toundefined(src/plugins/liveobjects/realtimeobject.ts:434-450), so the new test passes while the warning requirement remains unimplemented. Add the warning in the parser and capture/assert it here.
// RTO5a6 - "malformedserialnocolon" has no ':' separator, so it cannot be parsed per
// RTO5a1; it must be handled as if the channelSerial were absent (RTO5a5): the objects
// are applied and the sync completes (SYNCED)
ws.active_connection!.send_to_client(
buildObjectSyncMessage(msg.channel, 'malformedserialnocolon', [
src/plugins/liveobjects/realtimeobject.ts:625
- This SDK-authored 92008 error is reachable from public
get()and mutation APIs but has no remediation. These paths need distinct actionable advice:get()can be retried after reattaching, while a mutation must not be blindly retried because its publish has already succeeded. Parameterize the helper with a call-specific remediation and constructErrorInfowith the options-object form so the remediation is included.
reject(
new this._client.ErrorInfo(
`${failureDescription} due to the channel entering the ${state} state whilst waiting for objects sync to complete`,
92008,
400,
reason || undefined,
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
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.
Problem
RealtimeObject.get()waits for the objects sync state to reachSYNCEDbefore returning (RTO23c), but that wait listened only for the internalsyncedevent. If the channel left a usable state while aget()was parked, the returned promise never settled:channel.detach()rests the channel inDETACHEDwith the objects data cleared (RTO27a) — the parkedget()hangs until the user happens to re-attach;FAILED— including aSUSPENDEDchannel whose connection then fails terminally (propogateConnectionInterruptionmaps connectionfailed→ channelfailed) — is terminal: the data is cleared, no automatic recovery exists, and the promise can never resolve.This was also internally inconsistent:
publishAndApplyalready rejects deterministically in the identical wait (RTO20e1, 92008), andget()itself rejects when the channel is alreadyFAILEDat entry (ensureAttached, 90001) — but hung when the channel became unusable mid-wait.The spec now covers this as RTO23c1 (companion PR: ably/specification#514): the parked
get()must fail withErrorInfocode92008,statusCode400, andcauseset to the channel'serrorReason, regardless of the state the channel transitioned from.Fix
One mechanism, shared by both waiters:
actOnChannelState— the plugin's single channel-state entry point, invoked on every transition — now emits an internal-onlyObjectsInternalEvent.syncWaitFailedondetached/suspended/failed, before the RTO27a data clearing (drain-then-clear, matching ably-cocoa and ably-java). The event is emitted on_eventEmitterInternalonly, so it is not observable through the publicRealtimeObject#on()API. Because it hooks the state handler rather than a specific event source, every route into the three states is covered — includingSUSPENDED→FAILED._waitForSyncedOrChannelFailure(failureDescription)helper racessyncedagainstsyncWaitFailedand builds the 92008/400/causeErrorInfo;get()andpublishAndApplydiffer only in the message prefix mandated by their respective spec points ('the object could not be retrieved'vs'the operation could not be applied locally').publishAndApplyis refactored onto the helper, replacing its previousinternalStateChangessubscription. Both listeners are removed on either outcome.realtimechannel.ts(4 lines):notifyStateinvokesactOnChannelStatebefore assigningthis.errorReason, so the state-changereasonis now forwarded as an argument — exactly as the adjacent presence handler (_presence.actOnChannelState(state, hasPresence, reason)) already does. Without this the rejection'scausewould be lost.RTO27 data semantics are unchanged:
SUSPENDEDstill retains the objects data (RTO27b); onlyDETACHED/FAILEDclear it (RTO27a).Tests
RTO23c1UTS unit tests (fails-on-channel-{detached,suspended,failed}-0) previously existed as skipped deviations; the skip guards and thetest/uts/deviations.mdentry are removed now that the behaviour is implemented. Thefailedcase asserts thecause(cause.code === 90000), which exercises the reason-forwarding.RTO5a5/RTO5a6UTS unit tests (objects_pool.test.ts) coveringOBJECT_SYNCchannelSerialhandling: absent serial (single-message sync) and malformed serial treated as absent. The existing implementation already conforms — these add the spec-derived coverage.All tests derive from the UTS unit spec cases added in ably/specification#514.
Verification
test/uts/objects/unit: 325 passing, 0 failing, 0 pending (previously 3 pending deviations).test/uts/realtime/unitchannels + presence sweep (for therealtimechannel.tschange): 326 passing, 0 new failures.tsc --noEmitandeslintclean on the touched sources.Summary by CodeRabbit