[ECO-5728] Fix presence auto reenter causes NACKs with "not currently attached" after reconnect from transient disconnect#2241
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPresence ENTER/UPDATE and LEAVE now queue only when the channel is attaching and ChangesPresence Message Queuing Behavior
Bundle Size Threshold Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
VeskeR
changed the title
github-actions
Bot
temporarily deployed
to
staging/pull/2241/bundle-report
VeskeR
force-pushed
the
ECO-5728/fix-presence-auto-reenter
branch
from
87c4827 to
6675f4f
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2241/bundle-report
![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
🧹 Nitpick comments (1)
test/realtime/presence.test.js (1)
1829-1834: ⚡ Quick winAssert the channel is still
attachedbefore queuing the enter.Right now this only waits for the connection-level
disconnectedevent. If the channel has already leftattachedby then, the enter will take the existing ATTACHING path and the test still passes, so it doesn't prove the new RTP16d2 branch. Achannel.state === 'attached'assertion in this callback would pin the regression to the intended case.🤖 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/realtime/presence.test.js` around lines 1829 - 1834, Add an assertion that the channel is still attached before queuing the presence enter: inside the realtime.connection.once('disconnected', ...) callback (where channel.presence.enterClient(testClientId, 'data') is called), check channel.state === 'attached' and throw or fail the test if not; this ensures the test exercises the path where the channel remains ATTACHED while the connection becomes DISCONNECTED and prevents the enter from taking the ATTACHING path.
🤖 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/common/lib/client/realtimepresence.ts`:
- Around line 135-140: The queued presence branch pushes into pendingPresence
but leaves _myMembers unchanged, so onAttached()’s _ensureMyMembersPresent() can
auto-reenter with stale state before the queued update/leave is flushed; update
the logic so either (A) when queueing a self update/leave in the branches that
push to pendingPresence (the code around pendingPresence, including the similar
branch at the 199-204 area), also mutate _myMembers to reflect the queued op
(apply the intended enter/update/leave locally) or (B) change
_ensureMyMembersPresent()/onAttached() to first inspect pendingPresence for any
queued self ops for the client and suppress or supersede the auto-enter when a
later queued leave/update would contradict it; pick one approach and make sure
both the queueing sites (where pendingPresence is appended) and
_ensureMyMembersPresent()/onAttached() are updated to remain consistent.
---
Nitpick comments:
In `@test/realtime/presence.test.js`:
- Around line 1829-1834: Add an assertion that the channel is still attached
before queuing the presence enter: inside the
realtime.connection.once('disconnected', ...) callback (where
channel.presence.enterClient(testClientId, 'data') is called), check
channel.state === 'attached' and throw or fail the test if not; this ensures the
test exercises the path where the channel remains ATTACHED while the connection
becomes DISCONNECTED and prevents the enter from taking the ATTACHING path.
🪄 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: 81420361-6caa-4b74-9805-0ac0e237494b
📒 Files selected for processing (2)
src/common/lib/client/realtimepresence.tstest/realtime/presence.test.js
| if (channel.client.options.queueMessages) { | ||
| return new Promise((resolve, reject) => { | ||
| this.pendingPresence.push({ | ||
| presence: wirePresMsg, | ||
| callback: (err) => (err ? reject(err) : resolve()), | ||
| }); |
There was a problem hiding this comment.
Queued own update/leave can be undone by auto-reenter.
These branches only append to pendingPresence. _myMembers still reflects the pre-disconnect state, and onAttached() auto-reenters _myMembers before flushing pendingPresence. If an already-present client calls leave() or update() while DISCONNECTED, reconnect will first send a stale enter, then the queued op. That gives other subscribers an incorrect enter→leave sequence, or a stale enter→update. Please reconcile _myMembers with queued own presence ops, or make _ensureMyMembersPresent() consult/supersede pending presence first.
Also applies to: 199-204
🤖 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/common/lib/client/realtimepresence.ts` around lines 135 - 140, The queued
presence branch pushes into pendingPresence but leaves _myMembers unchanged, so
onAttached()’s _ensureMyMembersPresent() can auto-reenter with stale state
before the queued update/leave is flushed; update the logic so either (A) when
queueing a self update/leave in the branches that push to pendingPresence (the
code around pendingPresence, including the similar branch at the 199-204 area),
also mutate _myMembers to reflect the queued op (apply the intended
enter/update/leave locally) or (B) change _ensureMyMembersPresent()/onAttached()
to first inspect pendingPresence for any queued self ops for the client and
suppress or supersede the auto-enter when a later queued leave/update would
contradict it; pick one approach and make sure both the queueing sites (where
pendingPresence is appended) and _ensureMyMembersPresent()/onAttached() are
updated to remain consistent.
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
Looks good, but I think we should add some ConnectionManager handling as well. Right now, if the connection drops during a sendPresence call and we don’t receive an ack, those messages are queued in the ConnectionManager-level queue. I believe we should filter them out and move them to the channel-level queue on reconnect. Otherwise, we may still send those nacks.
This is a much less likely scenario, so we could potentially skip it, but I think it’s worth addressing.
Addresses review feedback on PR #2241. The previous approach queued presence at the channel level whenever the channel was ATTACHED but the connection was not CONNECTED. That left a rare corner case: if the connection dropped while a presence message was in flight, it was unconditionally resent on reconnect via the connection-wide queue, bypassing the re-attach gate. Instead, when the connection becomes CONNECTED, transfer any queued presence off the connection-wide queue onto the relevant channel's pending-presence queue (RTL3d/RTL3d2), so it is only sent once the channel has (re-)attached (RTP5b). This mirrors ably-java's transferQueuedPresenceMessages and also covers the in-flight resend case. Also narrows the queueMessages change: the implicit attach of an initialized or detached channel (RTP8d) happens regardless of queueMessages; only the channel-level queuing of the presence message itself (RTP16b) is gated on it.
github-actions
Bot
temporarily deployed
to
staging/pull/2241/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/common/lib/client/realtimepresence.ts (2)
127-132:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftQueued own
update/leavecan be undone by auto-reenter.This issue was previously flagged and remains unresolved. When queueing presence operations here,
_myMembersis not updated to reflect the queued operation. On reconnect,onAttached()(line 321) calls_ensureMyMembersPresent()(line 337) before flushingpendingPresence(lines 340-362). If a client callsupdate()orleave()while DISCONNECTED, the stale_myMembersstate will cause an incorrect auto-reenter, followed by the queued operation, giving subscribers an incorrect sequence.Please reconcile
_myMemberswith queued own-presence operations, or make_ensureMyMembersPresent()consult/supersede pending presence first.🤖 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/common/lib/client/realtimepresence.ts` around lines 127 - 132, When queueing an own presence operation in pendingPresence (in the branch where channel.client.options.queueMessages is true) the code does not update _myMembers, so on reconnect onAttached() -> _ensureMyMembersPresent() runs against stale state and may auto-reenter; fix by reconciling _myMembers when pushing a pending presence (i.e., when update() or leave() queues wirePresMsg) so _myMembers reflects the intended queued state, or alternatively change _ensureMyMembersPresent() to inspect pendingPresence first and treat queued own-presence messages as authoritative before deciding to auto-reenter; update the logic around pendingPresence, _myMembers, and the pendingPresence flush in onAttached()/flush path to ensure the queued op and _myMembers are consistent and that subscribers see the correct sequence.
184-190:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftQueued
leavecan be undone by auto-reenter.This issue was previously flagged and remains unresolved. When queueing leave operations here,
_myMembersis not updated. On reconnect,onAttached()will auto-reenter the stale_myMembersstate before flushing the queued leave, causing subscribers to see an incorrect enter→leave sequence.Please reconcile
_myMemberswith queued own-presence operations, or make_ensureMyMembersPresent()consult/supersede pending presence first.🤖 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/common/lib/client/realtimepresence.ts` around lines 184 - 190, The queued leave path pushes a pending presence (wirePresMsg) but doesn't update the local _myMembers, so on reconnect onAttached() can auto-reenter stale members before the queued leave is flushed; fix by reconciling pending own-presence when queueing: when channel.client.options.queueMessages is true and you push to pendingPresence in the block that handles wirePresMsg, detect own-client leave/enter (compare wirePresMsg.clientId to this.clientId and wirePresMsg.action === 'leave' or 'enter') and immediately update _myMembers accordingly (remove member on leave, add/update on enter) OR update _ensureMyMembersPresent() to consult pendingPresence for any queued own-presence ops and compute the effective _myMembers before auto-reentering; reference pendingPresence, wirePresMsg, _myMembers, onAttached(), and _ensureMyMembersPresent() when making the change.
🧹 Nitpick comments (2)
src/common/lib/client/realtimepresence.ts (2)
196-203: ⚡ Quick winRemove redundant code assignment.
Line 201 sets
err.code = 90001, but thePartialErrorInfoconstructor (lines 197-200) already set the code to90001as the second parameter. The assignment is redundant.♻️ Proposed fix
const err = new PartialErrorInfo( 'Unable to leave presence channel while in ' + channel.state + ' state', 90001, ); - err.code = 90001; throw err;🤖 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/common/lib/client/realtimepresence.ts` around lines 196 - 203, In the default branch where a PartialErrorInfo is constructed, remove the redundant manual assignment of err.code (the line "err.code = 90001"); the PartialErrorInfo constructor call in that block already sets the code to 90001, so keep the construction and throw err without reassigning err.code (locate this in the default case that creates PartialErrorInfo in realtimepresence.ts).
137-144: ⚡ Quick winRemove redundant code assignment.
Line 142 sets
err.code = 90001, but thePartialErrorInfoconstructor (line 138-141) already set the code to90001as the second parameter. The assignment is redundant.♻️ Proposed fix
const err = new PartialErrorInfo( 'Unable to ' + action + ' presence channel while in ' + channel.state + ' state', 90001, ); - err.code = 90001; throw err;🤖 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/common/lib/client/realtimepresence.ts` around lines 137 - 144, The code creates a PartialErrorInfo with code 90001 and then redundantly reassigns err.code = 90001; remove the redundant assignment so the error code comes solely from the PartialErrorInfo constructor. Edit the default case in the presence handling (where PartialErrorInfo is constructed using 'Unable to ' + action + ' presence channel while in ' + channel.state) and delete the line that sets err.code = 90001.
🤖 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/common/lib/client/realtimepresence.ts`:
- Around line 403-406: requeuePresenceMessages currently pairs the same callback
with each presence and can invoke it multiple times; wrap the incoming callback
in a multicaster before pushing so the batch triggers a single invocation (see
requeuePresenceMessages and this.pendingPresence), mirroring the multicaster
pattern used in onAttached (lines ~346-361), or alternatively change the method
signature to accept per-message callbacks (Array<{ presence:
WirePresenceMessage; callback: ErrCallback }>) and update
RealtimeChannel.requeuePresenceFromConnectionQueue accordingly; implement the
multicaster wrapper at the point where you push into this.pendingPresence so
only one callback invocation occurs for the entire presenceMessages array.
---
Duplicate comments:
In `@src/common/lib/client/realtimepresence.ts`:
- Around line 127-132: When queueing an own presence operation in
pendingPresence (in the branch where channel.client.options.queueMessages is
true) the code does not update _myMembers, so on reconnect onAttached() ->
_ensureMyMembersPresent() runs against stale state and may auto-reenter; fix by
reconciling _myMembers when pushing a pending presence (i.e., when update() or
leave() queues wirePresMsg) so _myMembers reflects the intended queued state, or
alternatively change _ensureMyMembersPresent() to inspect pendingPresence first
and treat queued own-presence messages as authoritative before deciding to
auto-reenter; update the logic around pendingPresence, _myMembers, and the
pendingPresence flush in onAttached()/flush path to ensure the queued op and
_myMembers are consistent and that subscribers see the correct sequence.
- Around line 184-190: The queued leave path pushes a pending presence
(wirePresMsg) but doesn't update the local _myMembers, so on reconnect
onAttached() can auto-reenter stale members before the queued leave is flushed;
fix by reconciling pending own-presence when queueing: when
channel.client.options.queueMessages is true and you push to pendingPresence in
the block that handles wirePresMsg, detect own-client leave/enter (compare
wirePresMsg.clientId to this.clientId and wirePresMsg.action === 'leave' or
'enter') and immediately update _myMembers accordingly (remove member on leave,
add/update on enter) OR update _ensureMyMembersPresent() to consult
pendingPresence for any queued own-presence ops and compute the effective
_myMembers before auto-reentering; reference pendingPresence, wirePresMsg,
_myMembers, onAttached(), and _ensureMyMembersPresent() when making the change.
---
Nitpick comments:
In `@src/common/lib/client/realtimepresence.ts`:
- Around line 196-203: In the default branch where a PartialErrorInfo is
constructed, remove the redundant manual assignment of err.code (the line
"err.code = 90001"); the PartialErrorInfo constructor call in that block already
sets the code to 90001, so keep the construction and throw err without
reassigning err.code (locate this in the default case that creates
PartialErrorInfo in realtimepresence.ts).
- Around line 137-144: The code creates a PartialErrorInfo with code 90001 and
then redundantly reassigns err.code = 90001; remove the redundant assignment so
the error code comes solely from the PartialErrorInfo constructor. Edit the
default case in the presence handling (where PartialErrorInfo is constructed
using 'Unable to ' + action + ' presence channel while in ' + channel.state) and
delete the line that sets err.code = 90001.
🪄 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: 8813e8a6-d4d4-47f5-9a97-664700433976
📒 Files selected for processing (4)
src/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/transport/connectionmanager.tstest/realtime/presence.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/realtime/presence.test.js
| requeuePresenceMessages(presenceMessages: WirePresenceMessage[], callback: ErrCallback): void { | ||
| for (const presence of presenceMessages) { | ||
| this.pendingPresence.push({ presence, callback }); | ||
| } |
There was a problem hiding this comment.
Callback invoked multiple times for batched messages.
The method signature accepts a single callback for an array of presenceMessages, implying one callback invocation for the entire batch. However, the implementation pairs the same callback with each individual message (lines 404-406). If the batch contains multiple messages and individual messages fail during send, the callback will be invoked once per failing message instead of once total.
The caller (RealtimeChannel.requeuePresenceFromConnectionQueue, line 560 in realtimechannel.ts) wraps pendingMessage.callback and expects a single invocation. Multicall will break this contract.
🔧 Suggested fix using Multicaster
Wrap the callback in a multicaster (similar to onAttached, lines 346-361) to ensure one invocation:
requeuePresenceMessages(presenceMessages: WirePresenceMessage[], callback: ErrCallback): void {
+ const multicaster = Multicaster.create(this.logger);
+ multicaster.push(callback);
for (const presence of presenceMessages) {
- this.pendingPresence.push({ presence, callback });
+ this.pendingPresence.push({ presence, callback: (err) => multicaster(err) });
}
}Alternatively, if each message should have independent error handling, change the signature to accept Array<{ presence: WirePresenceMessage; callback: ErrCallback }>.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| requeuePresenceMessages(presenceMessages: WirePresenceMessage[], callback: ErrCallback): void { | |
| for (const presence of presenceMessages) { | |
| this.pendingPresence.push({ presence, callback }); | |
| } | |
| requeuePresenceMessages(presenceMessages: WirePresenceMessage[], callback: ErrCallback): void { | |
| const multicaster = Multicaster.create(this.logger); | |
| multicaster.push(callback); | |
| for (const presence of presenceMessages) { | |
| this.pendingPresence.push({ presence, callback: (err) => multicaster(err) }); | |
| } | |
| } |
🤖 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/common/lib/client/realtimepresence.ts` around lines 403 - 406,
requeuePresenceMessages currently pairs the same callback with each presence and
can invoke it multiple times; wrap the incoming callback in a multicaster before
pushing so the batch triggers a single invocation (see requeuePresenceMessages
and this.pendingPresence), mirroring the multicaster pattern used in onAttached
(lines ~346-361), or alternatively change the method signature to accept
per-message callbacks (Array<{ presence: WirePresenceMessage; callback:
ErrCallback }>) and update RealtimeChannel.requeuePresenceFromConnectionQueue
accordingly; implement the multicaster wrapper at the point where you push into
this.pendingPresence so only one callback invocation occurs for the entire
presenceMessages array.
Addresses review feedback on PR #2241. The previous approach queued presence at the channel level whenever the channel was ATTACHED but the connection was not CONNECTED. That left a rare corner case: if the connection dropped while a presence message was in flight, it was unconditionally resent on reconnect via the connection-wide queue, bypassing the re-attach gate. Instead, when the connection becomes CONNECTED, transfer any queued presence off the connection-wide queue onto the relevant channel's pending-presence queue (RTL3d/RTL3d2), so it is only sent once the channel has (re-)attached (RTP5b). This mirrors ably-java's transferQueuedPresenceMessages and also covers the in-flight resend case. Also narrows the queueMessages change: the implicit attach of an initialized or detached channel (RTP8d) happens regardless of queueMessages; only the channel-level queuing of the presence message itself (RTP16b) is gated on it.
VeskeR
force-pushed
the
ECO-5728/fix-presence-auto-reenter
branch
from
877a1e7 to
e4820f8
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2241/bundle-report
VeskeR
force-pushed
the
ECO-5728/fix-presence-auto-reenter
branch
from
e4820f8 to
d794d79
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2241/bundle-report
VeskeR
force-pushed
the
ECO-5728/fix-presence-auto-reenter
branch
from
d794d79 to
310393b
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2241/bundle-report
A presence operation on a channel that has not yet reached the ATTACHED state is queued at the channel level only when the queueMessages client option is enabled (RTP16b); when it is disabled the operation is rejected instead of queued. The implicit attach of an initialized or detached channel (RTP8d) is independent of queueMessages and always happens.
When the connection becomes CONNECTED the connection-wide message queue is flushed (RTL3d). A presence message must not be flushed while its channel is still re-attaching, or the server rejects it with "not currently attached". Transfer any such queued presence off the connection-wide queue onto the channel's pending-presence queue, so it is sent only once the channel has re-attached (RTL3d2, RTP5b). Presence for channels that are not being re-attached is left on the connection-wide queue. Resolves ECO-5728
VeskeR
force-pushed
the
ECO-5728/fix-presence-auto-reenter
branch
from
310393b to
2931422
Compare
2 participants
A presence operation invoked while the connection was transiently DISCONNECTED
(with the channel still ATTACHED) was placed on the connection-wide queue, which
RTL3d flushes on reconnect before the channel re-attaches. The server then
rejected it with "not currently attached" (code 90001), because presence
requires a real, non-transient channel attachment, unlike regular messages.
Now, when the channel is ATTACHED but the connection is not CONNECTED, presence
is queued at the channel level (RTP16d2) and sent once the channel next reaches
ATTACHED (RTP5b), honoring queueMessages.
Two commits:
queueMessagesfor channel-level presence queueing (RTP16b), apre-existing gap fixed first.
Companion spec PR: ably/specification#490.
Resolves ECO-5728
Summary by CodeRabbit
queueMessages: when disabled, presence actions made before attachment/re-attachment are rejected instead of queued.presenceEnterWithoutAttachdocs to clarify queueing behavior whenqueueMessagesis enabled by default.queueMessages: falserejection and for ensuring queued presence transmits only after post-reconnect attachment, with membership verification.