[AIT-458] Support MAP_CLEAR object operation#2176
[AIT-458] Support MAP_CLEAR object operation#2176VeskeR merged 3 commits intointegration/protocol-v6from
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:
WalkthroughAdds a MAP_CLEAR operation and MapClear payload; tracks Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Server as Server
participant LM as LiveMap
participant State as InternalState
rect rgba(100,150,200,0.5)
note over Client,Server: Submit MAP_CLEAR
Client->>Server: send MAP_CLEAR (op with clearTimeserial)
Server->>LM: applyOperation(MAP_CLEAR, objectMessage)
LM->>State: _applyMapClear(objectMessage)
State->>State: set _clearTimeserial = clearTimeserial
State->>State: tombstone entries with entrySerial <= clearTimeserial
State->>State: cleanup parent references for object entries
LM->>Client: emit LiveMapUpdate (tombstones + clear marker)
end
rect rgba(150,100,200,0.5)
note over Server,LM: Subsequent MAP_SET gating
Server->>LM: applyOperation(MAP_SET, op with opSerial)
LM->>LM: _canApplyMapEntryOperation(entrySerial, opSerial)
alt opSerial <= _clearTimeserial
LM->>LM: skip apply (cleared)
else opSerial > _clearTimeserial
LM->>State: apply map entry update
LM->>Client: emit LiveMapUpdate (entry set)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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/2176/bundle-report
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
1c08be7 to
321ca58
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
321ca58 to
c136f2b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/realtime/liveobjects.test.js`:
- Around line 1509-1573: The file test/realtime/liveobjects.test.js
(specifically the test block "OBJECT_SYNC sequence initializes map with
clearTimeserial") is misformatted and failing the repo Prettier check; run the
repository formatter (e.g. prettier --write test/realtime/liveobjects.test.js or
the project's format script such as npm run format) to reformat the file, verify
npm run format:check passes, stage the updated file(s) and amend or create a new
commit; repeat for the other affected ranges noted in the review if they reside
in the same file or other files.
- Around line 3381-3384: Replace invalid chained relational assertions like
expect(tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg).to.be.true with an
explicit conjunction: expect(tsBeforeMsg <= mapEntry.tombstonedAt &&
mapEntry.tombstonedAt <= tsAfterMsg).to.be.true; do the same for the other four
failing assertions that use chained comparisons (any lines using the pattern a
<= b <= c) so each side is tested with && rather than relying on chained
operators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ca5bb4a-ef19-4922-9c57-bf86a75dce76
📒 Files selected for processing (6)
liveobjects.d.tssrc/plugins/liveobjects/livemap.tssrc/plugins/liveobjects/objectmessage.tssrc/plugins/liveobjects/realtimeobject.tstest/common/modules/liveobjects_helper.jstest/realtime/liveobjects.test.js
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/realtime/liveobjects.test.js (1)
3390-3392:⚠️ Potential issue | 🟠 MajorFix invalid JavaScript range assertion (
a <= b <= c).At Line 3390, this assertion is not a valid range check in JavaScript and can yield false positives.
Proposed fix
- tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg, + mapEntry.tombstonedAt >= tsBeforeMsg && mapEntry.tombstonedAt <= tsAfterMsg,#!/bin/bash # Verify all chained <= comparisons that should be converted to explicit conjunctions. rg -nP '\S+\s*<=\s*\S+\s*<=\s*\S+' test/realtime/liveobjects.test.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/realtime/liveobjects.test.js` around lines 3390 - 3392, The assertion uses an invalid chained comparison (tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg); replace it with an explicit conjunction so the range is checked correctly, e.g. change the boolean expression passed to expect in the test around mapEntry.tombstonedAt to (tsBeforeMsg <= mapEntry.tombstonedAt && mapEntry.tombstonedAt <= tsAfterMsg) while keeping the existing message and .to.be.true expectation; locate the check referencing tsBeforeMsg, tsAfterMsg and mapEntry.tombstonedAt in the test and update accordingly.
🧹 Nitpick comments (1)
src/plugins/liveobjects/objectmessage.ts (1)
234-240: Consider adding a spec reference for consistency.The static analysis flags this as an empty interface, but this pattern is intentionally consistent with
ObjectDelete(lines 190-192). However, other payload interfaces include@specreferences (e.g.,MCR1,MST1,MRM1). Consider adding a spec reference if one exists for MAP_CLEAR payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/liveobjects/objectmessage.ts` around lines 234 - 240, The MapClear interface is intentionally empty but lacks a `@spec` reference like other payloads; update the MapClear declaration to include the appropriate spec tag for the MAP_CLEAR payload (e.g., add an `@spec` annotation similar to MCR1/MST1/MRM1) above the export interface MapClear, and if no spec ID exists, add a short comment referencing ObjectDelete to document the intentional emptiness (reference the MapClear identifier and ObjectDelete for consistency).
🤖 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.d.ts`:
- Around line 1747-1750: The empty interface MapClear (and the similar
ObjectDelete) should be replaced with a type alias to satisfy the
noEmptyInterface lint rule; locate the declarations for MapClear and
ObjectDelete and change "export interface MapClear {}" and "export interface
ObjectDelete {}" to "export type MapClear = {}" and "export type ObjectDelete =
{}", preserving exported names and any surrounding JSDoc comments.
---
Duplicate comments:
In `@test/realtime/liveobjects.test.js`:
- Around line 3390-3392: The assertion uses an invalid chained comparison
(tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg); replace it with an
explicit conjunction so the range is checked correctly, e.g. change the boolean
expression passed to expect in the test around mapEntry.tombstonedAt to
(tsBeforeMsg <= mapEntry.tombstonedAt && mapEntry.tombstonedAt <= tsAfterMsg)
while keeping the existing message and .to.be.true expectation; locate the check
referencing tsBeforeMsg, tsAfterMsg and mapEntry.tombstonedAt in the test and
update accordingly.
---
Nitpick comments:
In `@src/plugins/liveobjects/objectmessage.ts`:
- Around line 234-240: The MapClear interface is intentionally empty but lacks a
`@spec` reference like other payloads; update the MapClear declaration to include
the appropriate spec tag for the MAP_CLEAR payload (e.g., add an `@spec`
annotation similar to MCR1/MST1/MRM1) above the export interface MapClear, and
if no spec ID exists, add a short comment referencing ObjectDelete to document
the intentional emptiness (reference the MapClear identifier and ObjectDelete
for consistency).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30445ec2-c678-4278-9ff4-a870915a4971
📒 Files selected for processing (6)
liveobjects.d.tssrc/plugins/liveobjects/livemap.tssrc/plugins/liveobjects/objectmessage.tssrc/plugins/liveobjects/realtimeobject.tstest/common/modules/liveobjects_helper.jstest/realtime/liveobjects.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/realtimeobject.ts
- test/common/modules/liveobjects_helper.js
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
c136f2b to
2924ce0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/liveobjects/objectmessage.ts`:
- Around line 234-239: Replace the empty interface MapClear with a non-empty
type to satisfy Biome's noEmptyInterface rule: update the declaration for
MapClear (the exported MapClear symbol) to be a type alias using a sealed record
type (i.e., Record<string, never>) so the shape is non-empty but still forbids
any properties; keep the export and comment describing MAP_CLEAR semantics
unchanged.
In `@test/realtime/liveobjects.test.js`:
- Around line 3402-3418: The test currently sends a MAP_CLEAR via
helper.recordPrivateApi('call.channel.sendState') and immediately asserts
counterObj._parentReferences.size, which can race with async message processing;
update the test to await the state-application callback before asserting (e.g.,
use the helper to wait for the corresponding apply step such as the private API
call that handles the incoming state, e.g. wait for 'call.channel.applyState' or
another helper-provided "state applied" event) after channel.sendState([...])
and only then check counterObj._parentReferences.size to ensure the MAP_CLEAR
has been applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a54e150b-8684-49e9-9ef9-b05fe10251a9
📒 Files selected for processing (7)
liveobjects.d.tssrc/plugins/liveobjects/livemap.tssrc/plugins/liveobjects/objectmessage.tssrc/plugins/liveobjects/realtimeobject.tstest/common/modules/liveobjects_helper.jstest/common/modules/private_api_recorder.jstest/realtime/liveobjects.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/liveobjects/realtimeobject.ts
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
2924ce0 to
217e5bf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
217e5bf to
3081e9c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
test/realtime/liveobjects.test.js
Outdated
| { serial: lexicoTimeserial('bbb', 4, 0), siteCode: 'bbb', cleared: false }, // existing site, earlier CGO, not applied | ||
| { serial: lexicoTimeserial('bbb', 5, 0), siteCode: 'bbb', cleared: false }, // existing site, same CGO, not applied | ||
| { serial: lexicoTimeserial('bbb', 6, 0), siteCode: 'bbb', cleared: true }, // existing site, later CGO, applied | ||
| { serial: lexicoTimeserial('aaa', 1, 0), siteCode: 'aaa', cleared: true }, // different site, earlier CGO, applied |
There was a problem hiding this comment.
If "existing site" means "site that already exists in siteTimeserials", then isn't aaa also an existing site (because of the MAP_SET)? Also, the mention of CGO here threw me a bit — I was trying to figure out what relevance CGO had to the siteTimeserials check (it doesn't). For the "existing site" cases, I think "regional order" would be clearer, and then for the "different site" ones I don't think we need to mention CGO?
There was a problem hiding this comment.
If "existing site" means "site that already exists in siteTimeserials", then isn't aaa also an existing site (because of the MAP_SET)?
Oops. It doesn't affect the test though as the set used the earliest possible timestamp. On second thought, we don't actually need a set here. What we want is a map with some initial data to test against, and ideally only a single entry in siteTimeserials to check (via the remove op). We can achieve that by creating a map with initial entries (no siteTimeserials) and then applying a remove op. I also realised I'm not actually creating any map objects in this test - only doing set/remove on them. Will fix.
There was a problem hiding this comment.
Also, the mention of CGO here threw me a bit — I was trying to figure out what relevance CGO had to the siteTimeserials check (it doesn't). For the "existing site" cases, I think "regional order" would be clearer, and then for the "different site" ones I don't think we need to mention CGO?
I don't agree with this, I think mention of CGO is relevant. The point of the test is to confirm that even though the incoming MAP_CLEAR operation has an earlier CGO than some previously applied operations on that object, it should still be applied via the siteTimeserial procedure - because it's coming from a different site (and there is no set clearTimeserial with later CGO). Both concepts are connected here. That's in contrast with map entry operations, which also perform a straight lexicographic timeserial check before applying an op, meaning an earlier CGO from a different site wouldn't pass that check solely by virtue of being from a different site - and just like MAP_CLEAR has its own check against the previously set clearTimeserial.
There was a problem hiding this comment.
We can achieve that by creating a map with initial entries (no siteTimeserials) and then applying a remove op.
Scratch that, map will still have a site timeserial from the create op. Still going to change to map create and use earliest timeserials for entries, just going to add some additional comments
There was a problem hiding this comment.
There was a problem hiding this comment.
Here's my understanding of what we're doing in this test: we're injecting a MAP_CLEAR which always passes the CGO comparison against the visible { foo: bar } entry, and thus which we expect to clear this entry if and only if the siteTimeserials check permits it. Thus, I'm reading the various entries of the ops array as scenarios that test the siteTimeserials check. So, it remains confusing to me why we're mentioning CGO in a test of the siteTimeserials check since in theory CGO is not a concept that is relevant for this check, which is that an operation should be permitted if and only if it's from a site for which there isn't an entry, or is in later regional order than the current entry for the site it's from.
It seems to me that what your assertions are doing is asserting the fact that CGO is irrelevant for the siteTimeserials check (i.e. that higher CGO doesn't matter if it's from a different site), and that all that matters is regional order within a given site. If so, I think that the comments alongside the ops array could be a bit clearer.
If you think that the test needs no changes though then I don't want to block merging over this.
There was a problem hiding this comment.
asserting the fact that CGO is irrelevant for the siteTimeserials check (i.e. that higher CGO doesn't matter if it's from a different site), and that all that matters is regional order within a given site. If so, I think that the comments alongside the ops array could be a bit clearer.
Yes, this is a good way to explain it.
If you think that the test needs no changes though then I don't want to block merging over this.
There are a couple of tests that do a similar siteTimeserials check, and I'd prefer to update them separately in #2181 - for some of them, CGO will actually matter for different sites (like MAP_SET and MAP_REMOVE, which have their own entry timeserial check), so I'll need to make sure the comments make sense for each case.
There was a problem hiding this comment.
Sure — let's keep this one unresolved for future reference, but happy to leave it with you to do separately
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
1a1f8f1 to
35f6d04
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
|
Squashed feedback commits, should be ready for approval and merge |
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
35f6d04 to
5e16856
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2176/bundle-report
See DR [1], realtime implementation [2] and spec [3]. The DR specifies that MAP_CLEAR is currently only emitted by the server for the root object. An additional decision was made that the client should be future-proofed to support MAP_CLEAR on any map object ID, not just root. This implementation follows that decision. Semantics of MAP_CLEAR support: 1. OBJECT_SYNC: The clearTimeserial from the sync state is stored on the LiveMap for use by future operations. Materialised entries from the `ObjectMessage.object.map` arrive pre-tombstoned by the server for entries that predated the clear. Initial entries from the `ObjectMessage.object.createOp` are merged via the existing MAP_SET/MAP_REMOVE semantics, which check against clearTimeserial. 2. MAP_SET: After the usual siteTimeserials check, the operation is additionally discarded if clearTimeserial is set and is lexicographically greater than or equal to the operation's serial, since the set predates (or is concurrent with) the clear. 3. MAP_REMOVE: No changes needed - a remove after a clear is valid regardless of clearTimeserial. 4. MAP_CLEAR: The operation is discarded if the current clearTimeserial is already greater than or equal to the incoming serial (stale clear). Otherwise, clearTimeserial is updated to the operation's serial, and all existing entries whose timeserial is null or less than or equal to the new clearTimeserial are tombstoned. Entries with a strictly later timeserial are preserved. Resolves AIT-458 [1] https://ably.atlassian.net/wiki/x/DABECAE [2] ably/realtime#8074 [3] ably/specification#432
VeskeR
force-pushed
the
AIT-458/map-clear
branch
from
5e16856 to
76f2dfb
Compare
[AIT-458] Support MAP_CLEAR object operation
2 participants
Add support for MAP_CLEAR object operation
See DR [1], realtime implementation [2] and spec [3].
The DR specifies that MAP_CLEAR is currently only emitted by the server
for the root object. An additional decision was made that the client
should be future-proofed to support MAP_CLEAR on any map object ID,
not just root. This implementation follows that decision.
Semantics of MAP_CLEAR support:
OBJECT_SYNC: The clearTimeserial from the sync state is stored on
the LiveMap for use by future operations. Materialised entries
from the
ObjectMessage.object.maparrive pre-tombstoned by theserver for entries that predated the clear. Initial entries from the
ObjectMessage.object.createOpare merged via the existingMAP_SET/MAP_REMOVE semantics, which check against clearTimeserial.
MAP_SET: After the usual siteTimeserials check, the operation is
additionally discarded if clearTimeserial is set and is
lexicographically greater than or equal to the operation's serial,
since the set predates (or is concurrent with) the clear.
MAP_REMOVE: No changes needed - a remove after a clear is valid
regardless of clearTimeserial.
MAP_CLEAR: The operation is discarded if the current clearTimeserial
is already greater than or equal to the incoming serial (stale
clear). Otherwise, clearTimeserial is updated to the operation's
serial, and all existing entries whose timeserial is null or less
than or equal to the new clearTimeserial are tombstoned. Entries
with a strictly later timeserial are preserved.
This PR also consolidates tombstonedAt calculation into LiveObject per RTLO6, See the spec change that adds RTLO6
Resolves AIT-458
[1] https://ably.atlassian.net/wiki/x/DABECAE
[2] https://github.com/ably/realtime/pull/8074
[3] ably/specification#432
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Refactor
Tests