[ECO-5613] fix: resolve presence re-entry on channel re-attach #1171
Conversation
|
Warning Rate limit exceeded@ttypic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughReorders Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Channel as ChannelBase
participant Presence
participant Pending as PendingQueue
participant Conn as ConnectionMgr
rect rgb(255,245,230)
Note over Client,Channel: Previous ordering
Client->>Channel: attach()
Channel->>Presence: onAttached() %% invoked early (was)
Presence->>Pending: enqueue outgoing presence (attaching)
Channel->>Client: emitUpdate(attached)
Pending--x Conn: queued messages not immediately resent
end
rect rgb(230,255,240)
Note over Client,Channel: New ordering (this change)
Client->>Channel: attach()
Channel->>Client: emitUpdate(attached)
Channel->>Presence: onAttached() %% invoked after state/update
Presence->>Conn: send queued presence / re-enter presence
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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/1171/features
github-actions
bot
temporarily deployed
to
staging/pull/1171/javadoc
ttypic
force-pushed
the
ECO-5613/fix-re-entry
branch
from
0433d9f to
17d9d2d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1171/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
2362-2364: Drop the unused helper stub
waitForPresencewas introduced but has no body and no callers. Leaving a blank helper invites confusion later; please either wire it up with real logic or remove it for now to keep the test class tidy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/Presence.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/src/main/java/io/ably/lib/realtime/Presence.java (1)
lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-365)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: check-realtime
- GitHub Check: check (29)
- GitHub Check: check-rest
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: build
github-actions
bot
temporarily deployed
to
staging/pull/1171/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (2)
27-27: Remove unused import and dead code.The
Predicateimport andwaitForPresencemethod appear to be unused scaffolding. The method body is empty and it's never invoked in the test.-import java.util.function.Predicate;- private void waitForPresence(Predicate<PaginatedResult<PresenceMessage>> predicate) throws AblyException { - - } -Also applies to: 2350-2352
2322-2327: Consider adding timeout failure message.The polling loop silently times out if the condition isn't met within 10 seconds. Consider adding an assertion after the loop to provide a clear failure message.
long timeout = 10_000; presentMembers = restClient.channels.get(channelName).presence.get(null); while (presentMembers.items().length != 1 && System.currentTimeMillis() - reconnectTimestamp < timeout) { Thread.sleep(250); presentMembers = restClient.channels.get(channelName).presence.get(null); } + if (presentMembers.items().length != 1) { + fail("Timed out waiting for presence data to sync to server. Expected 1 member, got " + presentMembers.items().length); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/Presence.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/main/java/io/ably/lib/realtime/Presence.java (1)
lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (24)
🔇 Additional comments (1)
lib/src/main/java/io/ably/lib/realtime/Presence.java (1)
758-758: LGTM! Logging additions improve observability.The verbose log statements clearly indicate whether presence messages are queued (attaching state) or sent immediately (attached state), which will aid in debugging presence re-entry issues.
Also applies to: 762-762
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
ECO-5613/fix-re-entry
branch
from
17d9d2d to
11df8bf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1171/features
github-actions
bot
temporarily deployed
to
staging/pull/1171/javadoc
ttypic
force-pushed
the
ECO-5613/fix-re-entry
branch
from
11df8bf to
a8676a2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1171/features
github-actions
bot
temporarily deployed
to
staging/pull/1171/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (4)
2254-2254: Consider using more specific exception types.The method signature changed from
throws AblyExceptiontothrows Exception. While this accommodates bothAblyExceptionandInterruptedException(fromThread.sleep), it's generally better to explicitly declare the specific checked exceptions thrown.Apply this diff for more explicit exception handling:
- public void realtime_presence_suspended_reenter() throws Exception { + public void realtime_presence_suspended_reenter() throws AblyException, InterruptedException {
2319-2326: Add explicit timeout verification in polling loop.The polling loop waits up to 10 seconds for the correct presence state but doesn't explicitly verify that the condition was met within the timeout. If the timeout is exceeded, the test continues and fails later at the assertion, losing the context that it was a timeout issue.
Apply this diff to add explicit timeout verification:
try (AblyRest restClient = new AblyRest(opts)) { long timeout = 10_000; presentMembers = restClient.channels.get(channelName).presence.get(null); while (presentMembers.items().length != 1 && System.currentTimeMillis() - reconnectTimestamp < timeout) { Thread.sleep(250); presentMembers = restClient.channels.get(channelName).presence.get(null); } + assertTrue("Verify presence synced within timeout", + System.currentTimeMillis() - reconnectTimestamp < timeout); }
2340-2340: Consider relaxing timestamp tolerance to prevent flakiness.The assertion validates that the LEAVE message timestamp is within 2 seconds of the reconnect timestamp. This tight tolerance (2000ms) might cause flakiness in slow CI environments or under heavy load.
Consider either:
- Increasing the tolerance to 5000ms (5 seconds) or more
- Removing this assertion if it's testing implementation details rather than required behavior
- assertTrue("Verify LEAVE message follows specs", Math.abs(leaveMessage.timestamp-reconnectTimestamp) < 2000); + assertTrue("Verify LEAVE message follows specs", Math.abs(leaveMessage.timestamp-reconnectTimestamp) < 5000);
2284-2342: Consider explicitly verifying re-entry message was sent.The test validates that
testClientId1is present on the server after suspend/resume (via REST), which indirectly confirms re-entry occurred. However, given the PR objective to fix "presence re-entry messages not being sent after attach," consider explicitly capturing and verifying that a presence ENTER/UPDATE message was actually sent fortestClientId1during the reconnection.This would make the test more directly validate the fix and catch regressions where re-entry succeeds by accident through other mechanisms.
You could add message filtering to MockWebsocketFactory to capture sent presence messages:
// After line 2260 final ArrayList<PresenceMessage> sentPresenceMessages = new ArrayList<>(); mockTransport.allowSend(new MockWebsocketFactory.MessageFilter() { @Override public boolean matches(ProtocolMessage message) { if (message.action == ProtocolMessage.Action.presence && message.presence != null) { synchronized (sentPresenceMessages) { Collections.addAll(sentPresenceMessages, message.presence); } } return true; } }); // After line 2314, before REST verification // Verify re-entry message was sent for testClientId1 assertTrue("Verify re-entry message was sent", sentPresenceMessages.stream().anyMatch(pm -> testClientId1.equals(pm.clientId) && pm.action == Action.enter));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/Presence.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- lib/src/main/java/io/ably/lib/realtime/Presence.java
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-365)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: build
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (19)
🔇 Additional comments (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java (2)
991-991: LGTM! Correctly validates presence re-entry for all members.The updated expectation of 9 presence messages (changed from 6) correctly validates that all presence members re-enter on resume failure, not just the pending and queued ones. This aligns with RTP19a of the Ably spec: when a channel re-attaches after failed resume, all local presence members must perform re-entry.
- First 3 clients (lines 918-923): successfully entered before disconnect
- Next 3 clients (lines 938-940): pending due to blocked acks
- Last 3 clients (lines 948-951): queued while disconnected
- All 9 should re-enter after failed resume
998-999: LGTM! Comprehensive validation of all presence members.The updated validation loop now correctly verifies that all 9 clients are present in the re-entry messages, replacing the previous fixed-index check that only validated clients 3-8. This comprehensive approach ensures the fix properly handles presence re-entry for all members:
- Builds a complete map from all sent presence messages
- Validates every client in the array is present
- No longer assumes only a subset of clients will re-enter
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
2290-2304: ****The test's use of
ably.connection.connectionManager.onMessage(null, msg)is safe. TheonMessageimplementation explicitly handles null transport: line 1203 checksif (transport != null && this.transport != transport), using short-circuit evaluation so null passes through safely to process the message. This is an intentional pattern, not a fragile workaround.
ttypic
force-pushed
the
ECO-5613/fix-re-entry
branch
from
a8676a2 to
e0366e4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1171/features
ttypic
force-pushed
the
ECO-5613/fix-re-entry
branch
from
e0366e4 to
7e2aa55
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1171/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (3)
2319-2326: Avoid passing DebugOptions to AblyRest.Using DebugOptions for REST is unnecessary and may carry transportFactory/debug knobs. Prefer plain ClientOptions for REST.
-try (AblyRest restClient = new AblyRest(opts)) { +try (AblyRest restClient = new AblyRest(createOptions(testVars.keys[0].keyStr))) {
2269-2272: Use a fresh ChannelWaiter per awaited state to avoid stale waits.ChannelWaiter is typically cheap; re-instantiating per wait removes any chance of internal state carry-over.
-ChannelWaiter channelWaiter = new ChannelWaiter(channel); -channelWaiter.waitFor(ChannelState.attached); +new ChannelWaiter(channel).waitFor(ChannelState.attached); ... -ably.connection.connectionManager.requestState(ConnectionState.suspended); -channelWaiter.waitFor(ChannelState.suspended); +ably.connection.connectionManager.requestState(ConnectionState.suspended); +new ChannelWaiter(channel).waitFor(ChannelState.suspended); ... -ably.connection.connectionManager.requestState(ConnectionState.connected); -channelWaiter.waitFor(ChannelState.attached); +ably.connection.connectionManager.requestState(ConnectionState.connected); +new ChannelWaiter(channel).waitFor(ChannelState.attached);Also applies to: 2306-2315
2334-2341: Stabilize LEAVE assertions to reduce flakiness (wait and widen time delta).
- Wait until exactly one LEAVE is observed (with a timeout) before asserting.
- 2s tolerance on timestamp is tight under CI load; bump to 5s (or anchor t0 pre‑connect).
-assertEquals("Verify exactly one LEAVE message was generated", leaveMessages.size(), 1); - -PresenceMessage leaveMessage = leaveMessages.get(0); +// Wait up to 10s for the single expected LEAVE +new Helpers.ConditionalWaiter().wait(() -> leaveMessages.size() == 1, 10_000); +assertEquals("Verify exactly one LEAVE message was generated", 1, leaveMessages.size()); + +PresenceMessage leaveMessage = leaveMessages.get(0); assertEquals("Verify LEAVE message follows specs",leaveMessage.action, Action.leave); assertEquals("Verify LEAVE message follows specs",leaveMessage.clientId, testClientId2); assertEquals("Verify LEAVE message follows specs",leaveMessage.data, presenceData); -assertTrue("Verify LEAVE message follows specs", Math.abs(leaveMessage.timestamp-reconnectTimestamp) < 2000); +assertTrue("Verify LEAVE message follows specs", Math.abs(leaveMessage.timestamp - reconnectTimestamp) < 5000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/Presence.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/main/java/io/ably/lib/realtime/Presence.java (1)
lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: build
- GitHub Check: check-rest
- GitHub Check: check-realtime
🔇 Additional comments (6)
lib/src/main/java/io/ably/lib/realtime/Presence.java (1)
758-758: LGTM! Logging additions improve debuggability.The verbose log statements clearly indicate when presence messages are queued versus sent, which will help diagnose presence re-entry issues. The logging aligns with the PR objective to improve debuggability.
Also applies to: 762-762
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
463-466: Critical fix: Presence re-entry now occurs after state updates.Moving
presence.onAttachedto afteremitUpdateensures that the UPDATE event is emitted before presence re-entry begins. This timing fix addresses the core issue where presence messages were added to the pending queue during attach but not resent after attach completion.
468-471: Consistent timing: Presence re-entry after channel attached state is set.This reordering ensures that when
presence.onAttached()callssendQueuedMessages(), the channel is already in theattachedstate. This guarantees that pending presence messages are sent immediately rather than being queued again, fixing the automatic presence re-entry issue described in ECO-5613.lib/src/test/java/io/ably/lib/test/realtime/RealtimeResumeTest.java (2)
29-29: LGTM! Imports support improved test validation.The additional imports enable the use of Set-based validation for unique client IDs in the presence re-entry test.
Also applies to: 32-33
995-1009: LGTM! Test validation now correctly expects all 9 clients to be re-entered.The updated test logic properly validates that automatic presence re-entry works after resume failure:
- Uses a Set to count unique client IDs (handles potential duplicates)
- Expects 9 unique clients instead of 6 (all clients entered: 0-2, 3-5, 6-8)
- Builds
sentPresenceMapfrom all sent presence messages- Verifies every client in the array is present in the map
This validates the presence re-entry fix in ChannelBase.java works correctly.
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
2254-2333: Solid reattach + re‑entry coverage; scenario reads well.The flow (attach → inject SYNC phantom → suspend → reattach → REST verify) cleanly exercises RTP5f/RTP17/RTP19. Nice use of try‑with‑resources and MockWebsocketFactory.
github-actions
bot
temporarily deployed
to
staging/pull/1171/javadoc
- Ensure presence update messages are correctly sent. - Add logging for presence updates in `Presence`, improving debuggability. - Update presence re-entry tests.
ttypic
force-pushed
the
ECO-5613/fix-re-entry
branch
from
7e2aa55 to
728e3de
Compare
Presence, improving debuggability.Summary by CodeRabbit
Bug Fixes
Improvements
Tests