Skip to content

Add support for protocol v6 - merge integration/protocol-v6 into main#2179

Merged
VeskeR merged 12 commits intomainfrom
integration/protocol-v6
Mar 12, 2026
Merged

Add support for protocol v6 - merge integration/protocol-v6 into main#2179
VeskeR merged 12 commits intomainfrom
integration/protocol-v6

Conversation

@VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Mar 12, 2026

This is an aggregate PR combining all PRs that targeted integration/protocol-v6 branch. This adds support for realtime protocol v6, with partial object sync support, v2 ObjectOperation fields and MAP_CLEAR operation.
All included PRs were reviewed individually, so a full review of all changed files isn't necessary.
Instead, I'd ask reviewers to have a final look at the public type changes to spot any obvious errors.

This PR includes:

Conflicts with the main branch are resolved by rebasing integration/objects-breaking-api on top of latest main using the --rebase-merges option (i.e. git rebase main --rebase-merges). Rebase is chosen over a merge commit to avoid a massive merge commit that would inevitably hinder traversing line history, interfere with git blame, and clutter the git history. As a result of the rebase, final commit hashes won't match those from the PRs mentioned above. However, thanks to --rebase-merges, the merge commits for those PRs will still be preserved in the final history, which should make finding the original context for relevant changes easier in the future.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added MAP_CLEAR operation action for clearing map objects.
    • Introduced strongly-typed data fields (boolean, bytes, number, string, json) in ObjectData.
    • New granular operation payload interfaces for map operations (Create, Set, Remove) and counter operations (Create, Increment).
  • Chores

    • Updated protocol version to v6.
    • Deprecated legacy operation fields; migrate to mapCreate, mapSet, mapRemove, counterCreate, and counterInc for improved clarity.

@VeskeR VeskeR marked this pull request as draft March 12, 2026 16:05
@coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2026

Walkthrough

A protocol version upgrade from v5 to v6 introducing refactored LiveObjects operation payloads and a new object synchronization pool. The changes deprecate legacy map and counter operation fields in favor of structured MapCreate, MapSet, MapRemove, CounterCreate, and CounterInc payloads, plus a new MAP_CLEAR operation. ObjectData now supports typed leaf fields (boolean, bytes, number, string, json) alongside deprecated generic value field. SyncObjectsDataPool is replaced with SyncObjectsPool for managing multi-message object synchronization state.

Changes

Cohort / File(s) Summary
Type Definitions
liveobjects.d.ts
Added MAP_CLEAR operation action and new payload interfaces (MapCreate, MapSet, MapRemove, CounterCreate, CounterInc, ObjectDelete, MapClear). Expanded ObjectOperation with new payload fields and deprecated legacy mapOp/counterOp fields. ObjectData now includes typed leaf fields (boolean, bytes, number, string, json) with deprecated value field.
Protocol Version Bump
src/common/lib/util/defaults.ts, test/realtime/init.test.js, test/rest/http.test.js
Updated protocolVersion from 5 to 6 in Defaults object and corresponding test assertions for API version headers.
Counter Operations
src/plugins/liveobjects/livecounter.ts, src/plugins/liveobjects/livecountervaluetype.ts
Replaced counterOp payloads with new CounterInc (number field) and CounterCreate (count field) structures. Updated _applyCounterInc and _mergeInitialDataFromCreateOperation to use new payload shapes and encoding paths.
Map Operations & Data
src/plugins/liveobjects/livemap.ts, src/plugins/liveobjects/livemapvaluetype.ts
Removed ValueObjectData wrapper type; LiveMapObjectData now uses ObjectData directly for primitives and objectId for references. Replaced map creation flow with MapCreate structure and encodePartialObjectOperationForWire for encoding. Added mapCreateWithObjectId payload support.
Object Lifecycle & Encoding
src/plugins/liveobjects/liveobject.ts, src/plugins/liveobjects/objectmessage.ts
Added _calculateTombstonedAt helper for consistent tombstone timestamp handling. objectmessage.ts extensively refactored: new MapCreate/MapSet/MapRemove/CounterCreate/CounterInc/MapClear interfaces, wire encoding/decoding paths, user-facing operation transformation (toUserFacingObjectOperation), and helpers for primitive-to-ObjectData conversion.
Object Synchronization
src/plugins/liveobjects/syncobjectsdatapool.ts, src/plugins/liveobjects/syncobjectspool.ts, src/plugins/liveobjects/realtimeobject.ts
Removed SyncObjectsDataPool class; introduced new SyncObjectsPool for accumulating and merging object sync state with partial map merging and tombstone handling. RealtimeObject updated to use new pool throughout sync workflow and object reconstruction logic.
Test Helpers & Configuration
scripts/moduleReport.ts, test/common/modules/liveobjects_helper.js, test/common/modules/private_api_recorder.js
moduleReport.ts switched allowed file from syncobjectsdatapool.ts to syncobjectspool.ts. liveobjects_helper.js refactored operation builders to new payload structures (mapCreate, mapSet, mapRemove, counterCreate, counterInc, mapClear) and removed nonce() calls. private_api_recorder.js added new internal API identifiers for toUserFacingMessage and _derivedFrom tracking.
Test Coverage
test/realtime/liveobjects.test.js
Added expectDeepSubset helper for flexible nested object assertions. Expanded test scenarios for new operation payloads, MAP_CLEAR handling, multi-message OBJECT_SYNC sequences, clearTimeserial tracking, and toUserFacingMessage transformations. Updated assertions to reflect new operation field names and wire/user-facing API conversions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A protocol upgrade brings new fields so bright,
MapCreate, MapSet, and MapClear in sight!
The pool now syncs with a fresh, cleaner way,
While deprecated fields bow and fade away.
Typed leaves replace the generic old value,
Version six marches forward—we have evolved! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and accurately summarizes the main objective—adding protocol v6 support to the codebase, including the merge of integration/protocol-v6 into main.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch integration/protocol-v6
📝 Coding Plan
  • Generate coding plan for human review comments

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.41.1)
test/realtime/liveobjects.test.js

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@VeskeR VeskeR changed the title Integration/protocol v6 Add support for protocol v6 - merge integration/protocol-v6 into main Mar 12, 2026
@VeskeR VeskeR mentioned this pull request Mar 12, 2026
VeskeR added 12 commits March 12, 2026 18:59
Bump the Ably protocol version from 5 to 6 and update corresponding test
assertions.

Note: this commit on its own will break tests that depend on protocol v6
features not yet implemented in this commit (e.g. partial object sync,
new ObjectOperation fields, MAP_CLEAR operation support). It is
intentionally separated from those feature commits for reviewability,
and this commit should be skipped during git bisect.

The full set of protocol v6 changes is tracked on the
integration/protocol-v6 branch.
…`value`

Previously, WireObjectData had typed fields (boolean, bytes, number,
string, json) but when decoded into the internal ObjectData, all
primitive typed fields were collapsed into a single `value: Primitive`
property. When encoding back to wire format, the code had to type-check
`value` to reconstruct the original typed field - an unnecessary
round-trip.

This change makes internal ObjectData hold the same typed fields as
WireObjectData, but with decoded values (bytes as Buffer | ArrayBuffer
not base64 string, json as JsonObject | JsonArray not JSON string).
This aligns the internal live map data representation with the wire
protocol and removes unnecessary back-and-forth conversions. The
conversion from typed fields to the combined `value` field now only
happens at the edge of the SDK - when accepting input from users
(e.g. map.set()), when exposing map values to the user (e.g. map.get()),
and when exposing realtime events via subscriptions.

This also sets up easy exposure of typed fields in the public ObjectData
interface in the following commit.
Based on realtime implementation in [1], and DR [2].

Two aspects of this change were not covered by the DR and required
additional decisions:

1. Public ObjectOperation and ObjectData interfaces

   The DR covered the internal wire protocol changes but not the ably-js
   public API surface which has ObjectOperation interface exposed in LO
   subscription callbacks. The public ObjectOperation interface is
   updated to use the protocol v6 field names (mapCreate, mapSet,
   mapRemove, counterCreate, counterInc). The previous fields (mapOp,
   counterOp, map, counter) are preserved as deprecated aliases.

   The public ObjectData interface previously exposed a combined `value`
   field, which was an incorrect internal representation leaking through
   the public API (introduced in 54f8ae2). The LODR-042 [3] DR proposed
   that subscription events should be equivalent to the REST API publish
   endpoint syntax. With protocol v6 aligning the realtime protocol and
   REST API, ObjectData now exposes the same typed fields available on
   the wire: boolean, bytes, number, string, json - with decoded values
   (bytes as Buffer/ArrayBuffer, json as parsed objects). The combined
   `value` field is preserved as a deprecated alias.

   Both new and deprecated fields are populated for backwards
   compatibility; deprecated fields will be removed in a future major
   version.

2. Retaining source *Create for *CreateWithObjectId operations

   Protocol v6 introduces *CreateWithObjectId fields for create
   operations where the client generates the object ID. These contain a
   nonce and an initialValue JSON string, and are the only create fields
   sent to realtime. However, the client also needs the source *Create
   (mapCreate/counterCreate) locally for:
   - Message size calculation: payload size is computed from the encoded
     fields to enforce maxMessageSize before sending.
   - Apply-on-ACK: the decoded fields are applied locally after server
     acknowledgement to update client state.

   To solve this, *CreateWithObjectId carries a _derivedFrom reference
   to the source *Create from which it was derived. This is purely
   local - stripped before wire transmission. The size of a
   *CreateWithObjectId operation is the size of its _derivedFrom
   *Create; to apply it locally, apply the _derivedFrom *Create. This
   follows the approach specified in [4].

   Considered alternatives:
   - Carrying mapCreate/counterCreate as sibling properties on the
     ObjectOperation and stripping them before wire serialisation. This
     works but muddies the semantics: mapCreate is a legitimate wire
     property for server-originated creates, so overloading it for a
     local-only purpose on client-originated operations requires
     special-case stripping logic in the encoding path.
   - Only keeping *CreateWithObjectId and deserializing the initialValue
     JSON back into mapCreate/counterCreate when needed. This adds an
     unnecessary encode-then-decode round-trip for every create
     operation.

Resolves AIT-315

[1] ably/realtime#8025
[2] https://ably.atlassian.net/wiki/x/AQAPEgE
[3] https://ably.atlassian.net/wiki/spaces/LOB/pages/4235722804/LODR-042+LiveObjects+Realtime+Client+API+Improvements#Subscriptions
[4] ably/specification#426 (comment)
Add:
- check client is ably build the final object tree across multiple sync
  messages
- check sync sequence does not break when receiving an unknown object
  type
[AIT-315] Use protocol v6 and update to use new typed `ObjectOperation` fields
JavaScript evaluates chained comparisons (a <= b <= c) left-to-right:
(a <= b) <= c coercing the intermediate boolean to a number. This means
expressions like `tsBeforeMsg <= value <= tsAfterMsg` can produce false
positives. Split into two explicit comparisons joined with `&&`.
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
[AIT-38] Add support for partial sync of objects - protocol v6
[AIT-458] Support MAP_CLEAR object operation
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/plugins/liveobjects/livemap.ts (1)

423-429: ⚠️ Potential issue | 🟠 Major

Resolve mapCreate before enforcing semantics.

Line 423 and Line 731 still validate only the direct mapCreate payload, but _mergeInitialDataFromCreateOperation() now supports both an omitted payload for empty-map creates and the locally retained mapCreateWithObjectId._derivedFrom form. As written, those valid create ops can throw a 500 before merge runs.

Suggested fix
-      if (objectState.createOp.mapCreate?.semantics !== this._semantics) {
+      const createMap = objectState.createOp.mapCreate ?? objectState.createOp.mapCreateWithObjectId?._derivedFrom;
+      if (createMap && createMap.semantics !== this._semantics) {
         throw new this._client.ErrorInfo(
-          `Invalid object state: object state createOp map semantics=${objectState.createOp.mapCreate?.semantics}; LiveMap semantics=${this._semantics}`,
+          `Invalid object state: object state createOp map semantics=${createMap.semantics}; LiveMap semantics=${this._semantics}`,
           92000,
           500,
         );
       }
-    if (this._semantics !== mapCreate?.semantics) {
+    if (mapCreate && this._semantics !== mapCreate.semantics) {
       throw new this._client.ErrorInfo(
-        `Cannot apply MAP_CREATE op on LiveMap objectId=${this.getObjectId()}; map's semantics=${this._semantics}, but op expected ${mapCreate?.semantics}`,
+        `Cannot apply MAP_CREATE op on LiveMap objectId=${this.getObjectId()}; map's semantics=${this._semantics}, but op expected ${mapCreate.semantics}`,
         92000,
         500,
       );
     }

Also applies to: 728-739

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/plugins/liveobjects/livemap.ts` around lines 423 - 429, Validate the
resolved mapCreate payload (including the "omitted empty-map" form and the local
derived form mapCreateWithObjectId._derivedFrom) before throwing on semantics
mismatches: call or reuse _mergeInitialDataFromCreateOperation (or otherwise
resolve objectState.createOp.mapCreate to the canonical payload / check
mapCreateWithObjectId._derivedFrom) to obtain the effective mapCreate, then
compare effective.mapCreate.semantics to this._semantics and throw the
this._client.ErrorInfo only if the resolved payload actually has a contradictory
semantics; update both the check near objectState.createOp (the earlier
validation and the one in the other location using the same pattern) to use the
resolvedMapCreate value instead of the raw objectState.createOp.mapCreate.
test/common/modules/liveobjects_helper.js (1)

164-172: ⚠️ Potential issue | 🟡 Minor

Normalize the counter helper option names.

These helpers now disagree on their input shape: counterIncOp() still reads amount, counterIncRestOp() reads number, and counterCreateRestOp() reads number even though it emits counterCreate.count. Callers using the other obvious spelling silently build malformed fixtures.

Alias-friendly fix
   counterIncOp(opts) {
-    const { objectId, amount } = opts ?? {};
+    const { objectId, amount, number = amount } = opts ?? {};
     const op = {
       operation: {
         action: ACTIONS.COUNTER_INC,
         objectId,
         counterInc: {
-          number: amount,
+          number,
         },
       },
     };

   counterCreateRestOp(opts) {
-    const { objectId, nonce, number } = opts ?? {};
+    const { objectId, nonce, count, number = count } = opts ?? {};
     const opBody = {
       counterCreate: {},
     };

   counterIncRestOp(opts) {
-    const { objectId, number } = opts ?? {};
+    const { objectId, amount, number = amount } = opts ?? {};
     const opBody = {
       objectId,
       counterInc: {
         number,
       },

Also applies to: 394-418

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/common/modules/liveobjects_helper.js` around lines 164 - 172, The
counter helpers use inconsistent option names (amount, number, count) causing
malformed fixtures; update counterIncOp, counterIncRestOp, and
counterCreateRestOp to normalize and emit a single canonical field (use "count")
by reading opts in a fallback order (opts.count ?? opts.amount ?? opts.number)
and assign that resulting value to the emitted payload (e.g., counterInc.number
or counterCreate.count should be driven from the normalized count variable);
apply the same normalization in all three helper functions so callers can pass
any of the common aliases and the emitted objects are consistent.
🤖 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/livemap.ts`:
- Around line 946-971: In LiveMap._applyMapClear(), avoid emitting a 'removed'
diff for keys that were already tombstoned/absent from the public map by only
marking update.update[typedKey] = 'removed' when the entry was previously
visible; i.e., before the assignment (after you compute typedKey and before
update.update[...] = 'removed') check the entry's visibility (for example
require entrySerial != null && entry.data != null/undefined and, if your entry
objects use a tombstone flag, ensure !entry.tombstoned) and only set the
'removed' marker when that condition is true; this change should be applied
around the code in LiveMap._applyMapClear that currently deletes from
this._dataRef.data and unconditionally sets update.update[typedKey].

In `@src/plugins/liveobjects/objectmessage.ts`:
- Around line 67-73: primitiveToObjectData currently treats only Node Buffers as
binary via client.Platform.BufferUtils.isBuffer, causing ArrayBuffer/TypedArray
inputs from browsers (ObjectsApi.Primitive) to be serialized as json; update
primitiveToObjectData to detect ArrayBuffer and ArrayBuffer views (e.g.,
ArrayBuffer.isView(value) or value instanceof ArrayBuffer) in addition to
client.Platform.BufferUtils.isBuffer and return { bytes: value } for those cases
so browser binary payloads are preserved (edit function primitiveToObjectData in
src/plugins/liveobjects/objectmessage.ts).

In `@src/plugins/liveobjects/syncobjectspool.ts`:
- Around line 85-87: The call to _mergeMapSyncState assumes
existingEntry.object!.map exists and will throw if the pooled entry is a
different type (e.g., a counter); update the sync handling to first check that
existingEntry.object && existingEntry.object.map (and that objectState.map is
present) before calling _mergeMapSyncState, and if the types conflict simply
skip/ignore that fragment (optionally log a warning) instead of invoking the
merge; apply the same guard to the other map-merge sites in the same
sync-processing block that merge objectState.map into existingEntry (i.e.,
wherever _mergeMapSyncState is invoked).

In `@test/realtime/liveobjects.test.js`:
- Around line 5668-5716: The test currently only checks that both child
subscribers fired; tighten it by asserting each subscriber is called exactly
once and that each event is scoped to the child path and value: for the two
subscriptions created with map.get('key1').subscribe(...) and
map.get('key2').subscribe(...), replace the simple receivedPaths set with
per-key call counters (e.g., key1Count, key2Count) and inside each subscriber
increment its counter, assert event.message.operation.action === 'map.clear',
assert event.object.path() (or event.object.path().toString()) equals the
specific child path for that subscriber and assert event.value() (or
event.value()) matches the expected cleared value semantics, then after dispatch
assert key1Count === 1 and key2Count === 1 (and that no other subscriber was
invoked). Ensure you reference the existing map, map.get('key1'),
map.get('key2'), event.object.path(), event.value() and the mapClear injection
via objectsHelper.mapClearOp to implement these checks.
- Around line 9433-9543: The loop only asserts variant payloads but not the
public-operation top-level fields; update the test inside the for-loop (around
the calls to objectMessageFromValues and msg.toUserFacingMessage) to also assert
that result.operation.action equals the scenario.operation.action and
result.operation.objectId equals the scenario.operation.objectId (use the
existing local symbols objectMessageFromValues, msg.toUserFacingMessage, and
result.operation to locate where to add the checks); add concise expects for
action and objectId for each scenario so miswired top-level fields fail the
test.
- Around line 47-60: The helper expectDeepSubset is too permissive for
empty-object expectations: when expected is {} it currently only asserts actual
is a non-null object, allowing extra fields to slip through; update
expectDeepSubset so that if expected is a plain object with zero keys
(Object.keys(expected).length === 0) it asserts actual deeply equals {} (e.g.,
expect(actual).to.deep.equal({}, msg)) and returns, otherwise proceed with the
existing recursive property checks; reference the expectDeepSubset function and
the empty-object branch to locate and change the behavior.

---

Outside diff comments:
In `@src/plugins/liveobjects/livemap.ts`:
- Around line 423-429: Validate the resolved mapCreate payload (including the
"omitted empty-map" form and the local derived form
mapCreateWithObjectId._derivedFrom) before throwing on semantics mismatches:
call or reuse _mergeInitialDataFromCreateOperation (or otherwise resolve
objectState.createOp.mapCreate to the canonical payload / check
mapCreateWithObjectId._derivedFrom) to obtain the effective mapCreate, then
compare effective.mapCreate.semantics to this._semantics and throw the
this._client.ErrorInfo only if the resolved payload actually has a contradictory
semantics; update both the check near objectState.createOp (the earlier
validation and the one in the other location using the same pattern) to use the
resolvedMapCreate value instead of the raw objectState.createOp.mapCreate.

In `@test/common/modules/liveobjects_helper.js`:
- Around line 164-172: The counter helpers use inconsistent option names
(amount, number, count) causing malformed fixtures; update counterIncOp,
counterIncRestOp, and counterCreateRestOp to normalize and emit a single
canonical field (use "count") by reading opts in a fallback order (opts.count ??
opts.amount ?? opts.number) and assign that resulting value to the emitted
payload (e.g., counterInc.number or counterCreate.count should be driven from
the normalized count variable); apply the same normalization in all three helper
functions so callers can pass any of the common aliases and the emitted objects
are consistent.
🪄 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: 8b9da26f-2158-464a-ad79-cafbe0e9e3f8

📥 Commits

Reviewing files that changed from the base of the PR and between 53b46ae and 4b3f644.

📒 Files selected for processing (17)
  • liveobjects.d.ts
  • scripts/moduleReport.ts
  • src/common/lib/util/defaults.ts
  • src/plugins/liveobjects/livecounter.ts
  • src/plugins/liveobjects/livecountervaluetype.ts
  • src/plugins/liveobjects/livemap.ts
  • src/plugins/liveobjects/livemapvaluetype.ts
  • src/plugins/liveobjects/liveobject.ts
  • src/plugins/liveobjects/objectmessage.ts
  • src/plugins/liveobjects/realtimeobject.ts
  • src/plugins/liveobjects/syncobjectsdatapool.ts
  • src/plugins/liveobjects/syncobjectspool.ts
  • test/common/modules/liveobjects_helper.js
  • test/common/modules/private_api_recorder.js
  • test/realtime/init.test.js
  • test/realtime/liveobjects.test.js
  • test/rest/http.test.js
💤 Files with no reviewable changes (1)
  • src/plugins/liveobjects/syncobjectsdatapool.ts

Comment on lines +946 to +971
// RTLM24e
for (const [key, entry] of this._dataRef.data.entries()) {
const entrySerial = entry.timeserial;
// RTLM24e1
if (entrySerial == null || this._clearTimeserial > entrySerial) {
this._client.Logger.logAction(
this._client.logger,
this._client.Logger.LOG_MICRO,
'LiveMap._applyMapClear()',
`clearing entry; key="${key}", entry serial=${entrySerial}, clear serial=${this._clearTimeserial}, objectId=${this.getObjectId()}`,
);

// Handle parent reference removal for object references
if (entry.data && 'objectId' in entry.data) {
// Remove parent reference from the object that was being referenced
const referencedObject = this._realtimeObject.getPool().get(entry.data.objectId);
if (referencedObject) {
referencedObject.removeParentReference(this, key);
}
}

// RTLM24e1a - Remove the entry from the internal data map entirely
this._dataRef.data.delete(key);

const typedKey: keyof T & string = key;
update.update[typedKey] = 'removed'; // RTLM24e1b
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Skip already-tombstoned keys when building the MAP_CLEAR diff.

Line 971 marks every deleted entry as 'removed', including entries that were already tombstoned and therefore already absent from the public map. A MAP_CLEAR after a prior MAP_REMOVE will emit a duplicate removal diff/path event for that key even though user-visible state did not change.

Suggested fix
-      if (entrySerial == null || this._clearTimeserial > entrySerial) {
+      if (entrySerial == null || this._clearTimeserial > entrySerial) {
+        const wasVisible = entry.tombstone !== true;
+
         this._client.Logger.logAction(
           this._client.logger,
           this._client.Logger.LOG_MICRO,
           'LiveMap._applyMapClear()',
           `clearing entry; key="${key}", entry serial=${entrySerial}, clear serial=${this._clearTimeserial}, objectId=${this.getObjectId()}`,
@@
-        // RTLM24e1a - Remove the entry from the internal data map entirely
+        // RTLM24e1a - Remove the entry from the internal data map entirely
         this._dataRef.data.delete(key);
 
-        const typedKey: keyof T & string = key;
-        update.update[typedKey] = 'removed'; // RTLM24e1b
+        if (wasVisible) {
+          const typedKey: keyof T & string = key;
+          update.update[typedKey] = 'removed'; // RTLM24e1b
+        }
       } else {
📝 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.

Suggested change
// RTLM24e
for (const [key, entry] of this._dataRef.data.entries()) {
const entrySerial = entry.timeserial;
// RTLM24e1
if (entrySerial == null || this._clearTimeserial > entrySerial) {
this._client.Logger.logAction(
this._client.logger,
this._client.Logger.LOG_MICRO,
'LiveMap._applyMapClear()',
`clearing entry; key="${key}", entry serial=${entrySerial}, clear serial=${this._clearTimeserial}, objectId=${this.getObjectId()}`,
);
// Handle parent reference removal for object references
if (entry.data && 'objectId' in entry.data) {
// Remove parent reference from the object that was being referenced
const referencedObject = this._realtimeObject.getPool().get(entry.data.objectId);
if (referencedObject) {
referencedObject.removeParentReference(this, key);
}
}
// RTLM24e1a - Remove the entry from the internal data map entirely
this._dataRef.data.delete(key);
const typedKey: keyof T & string = key;
update.update[typedKey] = 'removed'; // RTLM24e1b
// RTLM24e
for (const [key, entry] of this._dataRef.data.entries()) {
const entrySerial = entry.timeserial;
// RTLM24e1
if (entrySerial == null || this._clearTimeserial > entrySerial) {
const wasVisible = entry.tombstone !== true;
this._client.Logger.logAction(
this._client.logger,
this._client.Logger.LOG_MICRO,
'LiveMap._applyMapClear()',
`clearing entry; key="${key}", entry serial=${entrySerial}, clear serial=${this._clearTimeserial}, objectId=${this.getObjectId()}`,
);
// Handle parent reference removal for object references
if (entry.data && 'objectId' in entry.data) {
// Remove parent reference from the object that was being referenced
const referencedObject = this._realtimeObject.getPool().get(entry.data.objectId);
if (referencedObject) {
referencedObject.removeParentReference(this, key);
}
}
// RTLM24e1a - Remove the entry from the internal data map entirely
this._dataRef.data.delete(key);
if (wasVisible) {
const typedKey: keyof T & string = key;
update.update[typedKey] = 'removed'; // RTLM24e1b
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/plugins/liveobjects/livemap.ts` around lines 946 - 971, In
LiveMap._applyMapClear(), avoid emitting a 'removed' diff for keys that were
already tombstoned/absent from the public map by only marking
update.update[typedKey] = 'removed' when the entry was previously visible; i.e.,
before the assignment (after you compute typedKey and before update.update[...]
= 'removed') check the entry's visibility (for example require entrySerial !=
null && entry.data != null/undefined and, if your entry objects use a tombstone
flag, ensure !entry.tombstoned) and only set the 'removed' marker when that
condition is true; this change should be applied around the code in
LiveMap._applyMapClear that currently deletes from this._dataRef.data and
unconditionally sets update.update[typedKey].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created https://ably.atlassian.net/browse/AIT-554 for this, need to discuss with wider team

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VeskeR, thanks for tracking this in AIT-554! Happy to wait for the wider team discussion on the intended behaviour. Feel free to re-open or link back here once a decision is reached.

(´• ω •`) 🐇 ~ thinking about serials...

@VeskeR VeskeR merged commit 7f461e6 into main Mar 12, 2026
13 of 14 checks passed
@VeskeR VeskeR deleted the integration/protocol-v6 branch March 12, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants