[AIT-296] fix: clear buffered operations unconditionally on ATTACHED (RTO4d)#1196
Open
ttypic wants to merge 1 commit intoAIT-276/apply-on-ackfrom
Open
[AIT-296] fix: clear buffered operations unconditionally on ATTACHED (RTO4d)#1196ttypic wants to merge 1 commit intoAIT-276/apply-on-ackfrom
ttypic wants to merge 1 commit intoAIT-276/apply-on-ackfrom
Conversation
WalkthroughChanges refactor buffer clearing logic for object operations: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 the current code and only fix it if needed.
Inline comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt`:
- Around line 300-301: The test expects bufferedObjectOperations to be preserved
across server-initiated resyncs, but ObjectsManager.startNewSync currently
clears bufferedObjectOperations; update the implementation by removing the call
that empties the buffer so that the ObjectsManager.bufferedObjectOperations (or
BufferedObjectOperations collection) is not cleared during startNewSync,
ensuring the behavior matches the test (and spec RTO5a2b) and leaving any
buffered ops intact across resyncs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85bd7049-01ca-4888-839a-0787bcc94aa2
📒 Files selected for processing (3)
liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt
Show resolved
Hide resolved
ttypic
force-pushed
the
AIT-296/buffered-ops
branch
from
8eb140f to
d89628c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt (1)
34-41: Make instance cleanup automatic to avoid future test leaks.Good addition of
tearDown(). One maintainability gap: cleanup still depends on remembering to push every createdDefaultRealtimeObjectsintotestInstances.♻️ Small refactor to centralize tracking
class DefaultRealtimeObjectsTest { private val testInstances = mutableListOf<DefaultRealtimeObjects>() + + private fun trackedRealtimeObjects(relaxed: Boolean = false): DefaultRealtimeObjects = + getDefaultRealtimeObjectsWithMockedDeps(relaxed = relaxed).also { testInstances.add(it) } `@After` fun tearDown() { val cleanupError = AblyException.fromErrorInfo(ErrorInfo("test cleanup", 500)) testInstances.forEach { it.dispose(cleanupError) } testInstances.clear() }Then replace direct calls like
getDefaultRealtimeObjectsWithMockedDeps()withtrackedRealtimeObjects().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt` around lines 34 - 41, Tests rely on manually adding each DefaultRealtimeObjects to testInstances for teardown, which is error-prone; create a helper trackedRealtimeObjects() that constructs a DefaultRealtimeObjects (delegating to getDefaultRealtimeObjectsWithMockedDeps() or similar), automatically appends it to the testInstances list, and return it to callers so tests no longer need to push instances themselves; update tests to call trackedRealtimeObjects() instead of getDefaultRealtimeObjectsWithMockedDeps() and keep tearDown() as-is to dispose all tracked DefaultRealtimeObjects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt`:
- Around line 34-41: Tests rely on manually adding each DefaultRealtimeObjects
to testInstances for teardown, which is error-prone; create a helper
trackedRealtimeObjects() that constructs a DefaultRealtimeObjects (delegating to
getDefaultRealtimeObjectsWithMockedDeps() or similar), automatically appends it
to the testInstances list, and return it to callers so tests no longer need to
push instances themselves; update tests to call trackedRealtimeObjects() instead
of getDefaultRealtimeObjectsWithMockedDeps() and keep tearDown() as-is to
dispose all tracked DefaultRealtimeObjects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cf77248-6664-448e-971d-94a0cbbccd10
📒 Files selected for processing (4)
liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.ktliveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt
💤 Files with no reviewable changes (1)
- liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt
- liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns the Kotlin LiveObjects implementation with spec update RTO4d by ensuring buffered object operations are cleared on every channel transition to ATTACHED (independent of hasObjects), and by no longer discarding buffered operations when a new sync starts due to server-driven resyncs.
Changes:
- Clear
bufferedObjectOperationsunconditionally onChannelState.attached(RTO4d) and remove the now-redundant clear in the!hasObjectsbranch. - Stop clearing
bufferedObjectOperationsinObjectsManager.startNewSyncso buffered ops survive server-initiated resyncs. - Expand unit tests to cover attach/sync/resync flows and validate buffer clearing/preservation behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt |
Clears buffered ops unconditionally on ATTACHED and removes duplicate clearing in !hasObjects path. |
liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt |
Removes clearing of buffered object ops from startNewSync, preserving buffered ops across resync restarts. |
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt |
Adds tests for RTO4d attach clearing + resync behavior; introduces teardown for disposing created instances. |
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt |
Updates the startNewSync test intent to reflect new buffering rules and adds buffering setup. |
Comments suppressed due to low confidence (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/ObjectsManagerTest.kt:503
- The test name/docstring says bufferedObjectOperations is NOT cleared by startNewSync, but the test never asserts the buffer size after calling startNewSync("seq-2"). Add an assertion after the second startNewSync to verify BufferedObjectOperations is unchanged (and optionally that the buffered op is still present), otherwise this test won’t catch regressions.
// Start a new sync — should clear both appliedOnAckSerials and bufferedAcks (RTO21b)
// but must NOT clear bufferedObjectOperations (now handled unconditionally by RTO4d in handleStateChange)
objectsManager.startNewSync("seq-2")
// RTO21b — appliedOnAckSerials and bufferedAcks cleared
assertTrue(defaultRealtimeObjects.appliedOnAckSerials.isEmpty(),
"appliedOnAckSerials should be cleared on new sync")
assertEquals(0, objectsManager.BufferedAcks.size,
"bufferedAcks should be cleared on new sync")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt
Show resolved
Hide resolved
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt
Show resolved
Hide resolved
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt
Show resolved
Hide resolved
Collaborator
|
Shared few review comments, once you go through them and address few (optional) and above copilot comments, will approve the PR 👍 |
…(RTO4d) - Ensured `BufferedObjectOperations` is always cleared when channel state transitions to ATTACHED, regardless of the `hasObjects` flag. - Updated tests to verify buffer clearing behavior (RTO4d).
ttypic
force-pushed
the
AIT-296/buffered-ops
branch
from
d89628c to
61a681a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt (2)
34-40: Hide instance registration behind a helper.Now that teardown depends on
testInstances, the manual.also { testInstances.add(it) }bookkeeping is easy to forget in the next test. A small factory/helper would keep construction and cleanup symmetric.Example cleanup helper
private fun newRealtimeObjects(): DefaultRealtimeObjects = DefaultRealtimeObjects("testChannel", getMockObjectsAdapter()) .also(testInstances::add)Also applies to: 199-200, 251-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt` around lines 34 - 40, Create a small factory helper that constructs DefaultRealtimeObjects and registers it into the existing testInstances list to avoid repetitive .also { testInstances.add(it) } calls; e.g. add a private function newRealtimeObjects() that calls DefaultRealtimeObjects("testChannel", getMockObjectsAdapter()) and .also(testInstances::add), then update tests to call newRealtimeObjects() wherever they currently create DefaultRealtimeObjects manually (look for occurrences around the constructions that currently call .also { testInstances.add(it) }) so teardown (tearDown using testInstances) reliably cleans up all instances.
171-195: Assert the ATTACHED call order too.This only proves the buffer is empty eventually. It will still pass if
startNewSync(null)moves beforeclearBufferedObjectOperations(), but the implementation relies on the opposite order to avoid dropping ops in that window. Please pin that ordering here as well.Possible assertion to add
verifyOrder { defaultRealtimeObjects.ObjectsManager.clearBufferedObjectOperations() defaultRealtimeObjects.ObjectsManager.startNewSync(null) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt` around lines 171 - 195, The test currently only checks the buffer is empty eventually; add an explicit ordering assertion to ensure clearBufferedObjectOperations() is invoked before startNewSync(null) on defaultRealtimeObjects.ObjectsManager. After calling defaultRealtimeObjects.handleStateChange(ChannelState.attached, true) and the existing assertWaiter, add a verifyOrder block that references defaultRealtimeObjects.ObjectsManager.clearBufferedObjectOperations() followed by defaultRealtimeObjects.ObjectsManager.startNewSync(null) to pin the required call order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@liveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt`:
- Around line 34-40: Create a small factory helper that constructs
DefaultRealtimeObjects and registers it into the existing testInstances list to
avoid repetitive .also { testInstances.add(it) } calls; e.g. add a private
function newRealtimeObjects() that calls DefaultRealtimeObjects("testChannel",
getMockObjectsAdapter()) and .also(testInstances::add), then update tests to
call newRealtimeObjects() wherever they currently create DefaultRealtimeObjects
manually (look for occurrences around the constructions that currently call
.also { testInstances.add(it) }) so teardown (tearDown using testInstances)
reliably cleans up all instances.
- Around line 171-195: The test currently only checks the buffer is empty
eventually; add an explicit ordering assertion to ensure
clearBufferedObjectOperations() is invoked before startNewSync(null) on
defaultRealtimeObjects.ObjectsManager. After calling
defaultRealtimeObjects.handleStateChange(ChannelState.attached, true) and the
existing assertWaiter, add a verifyOrder block that references
defaultRealtimeObjects.ObjectsManager.clearBufferedObjectOperations() followed
by defaultRealtimeObjects.ObjectsManager.startNewSync(null) to pin the required
call order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2000163a-bb39-4206-a782-8ff1db90aa30
📒 Files selected for processing (3)
liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.ktliveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/objects/DefaultRealtimeObjectsTest.kt
💤 Files with no reviewable changes (1)
- liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectsManager.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- liveobjects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt
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.
BufferedObjectOperationsis always cleared when channel state transitions to ATTACHED, regardless of thehasObjectsflag.Spec: ably/specification#416
Js: ably/ably-js#2150
Summary by CodeRabbit