Skip to content

Add UTS tests for LiveObjects path-based API#2219

Open
paddybyers wants to merge 11 commits into
mainfrom
uts-liveobjects
Open

Add UTS tests for LiveObjects path-based API#2219
paddybyers wants to merge 11 commits into
mainfrom
uts-liveobjects

Conversation

@paddybyers

@paddybyers paddybyers commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

  • Translates portable UTS test specs from specification/uts/objects/ into 290 working ably-js TypeScript tests
  • Covers PathObject, Instance, batch operations, LiveCounter/LiveMap CRDT internals, ObjectsPool sync state machine, RealtimeObject lifecycle, value types, and subscriptions
  • 19 test files: 14 unit (270 tests), 4 integration (15 tests), 1 proxy fault injection (5 tests)
  • 1 test pending (LiveMap#clear() not yet implemented in ably-js)

Test plan

  • All 290 tests pass locally: npx mocha --no-config --require tsx/cjs 'test/uts/objects/**/*.test.ts' --timeout 60000
  • CI passes

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • PathObject.instance() now returns an Instance wrapper for primitive values (not just live objects).
  • Bug Fixes
    • LiveObjects pool cleanup no longer deletes the root object.
    • Improved tombstone/removal behavior for map entries, including correct reuse semantics.
    • batch() now rejects non-live-object paths with the expected error.
  • Documentation
    • Updated JSDoc for AnyPathObject.instance to reflect primitive support.

@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0d349c59-6ecb-406a-8c79-555cfe74bcae

📥 Commits

Reviewing files that changed from the base of the PR and between a35b2a7 and 4bd8c27.

📒 Files selected for processing (4)
  • liveobjects.d.ts
  • src/plugins/liveobjects/pathobject.ts
  • test/realtime/liveobjects.test.js
  • test/uts/objects/unit/path_object.test.ts

Walkthrough

New LiveObjects UTS coverage adds protocol-variant integration suites, mock protocol fixtures, CRDT and object-pool tests, PathObject and subscription tests, value-type tests, polling utilities, and deviation documentation. The object pool cleanup also preserves the root object.

Changes

LiveObjects behavior and test coverage

Layer / File(s) Summary
Protocol-parameterized integration flows
test/uts/objects/integration/*, test/uts/objects/integration/proxy/*
Adds JSON and MsgPack integration coverage for lifecycle, synchronization, tombstones, reconnects, buffering, and proxy fault scenarios.
Mock protocol and synced-channel harness
test/uts/objects/helpers/standard_test_pool.ts
Adds standard object fixtures, protocol builders, serial helpers, mock channel setup, ACK control, and update capture utilities.
Object pool and CRDT state behavior
src/plugins/liveobjects/objectspool.ts, test/uts/objects/unit/objects_pool.test.ts, test/uts/objects/unit/live_map.test.ts, test/uts/objects/unit/live_counter.test.ts, test/uts/objects/unit/parent_references.test.ts
Covers synchronization state, buffering, deduplication, map and counter operations, tombstones, garbage collection, and parent-reference rebuilding while preserving the root object.
Object, instance, and path APIs
test/uts/objects/unit/instance.test.ts, test/uts/objects/unit/path_object*.test.ts, test/uts/objects/unit/live_object_subscribe.test.ts, test/uts/objects/unit/object_id.test.ts
Tests object wrappers, path reads and mutations, object identifiers, subscriptions, depth filtering, bubbling, replacement, and event dispatch.
RealtimeObject lifecycle and publish flow
test/uts/objects/unit/realtime_object.test.ts
Covers attach and sync transitions, publish and ACK behavior, failure handling, echo deduplication, channel preconditions, garbage collection, and subscription registration.
LiveMap and LiveCounter value types
test/uts/objects/unit/value_types.test.ts, test/uts/objects/unit/live_map_api.test.ts
Validates value-type creation, wire encoding, nested creation order, primitive and JSON mapping, bytes, API operations, and unsupported clear() behavior.
Polling and UTS deviation documentation
test/uts/helpers.ts, test/uts/realtime/unit/*, test/uts/deviations.md
Adds pollUntil, applies it to asynchronous realtime tests, and records adapted, skipped, and questioned UTS behaviors.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • ably/ably-js#2256: Updates the same LiveObjects integration suites and protocol-variant helper.
  • ably/ably-js#2257: Contains the related root-preservation change in ObjectsPool cleanup.
  • ably/ably-js#2260: Covers primitive wrapping by PathObject.instance().

Suggested reviewers: lawrence-forooghian

Poem

A rabbit tests the maps at night,
With counters hopping left and right.
Tombstones fade, sync signals hum,
Two protocols say, “Here we come!”
The root stays safe beneath the sun. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding UTS tests for the LiveObjects path-based API.
Docstring Coverage ✅ Passed Docstring coverage is 96.30% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch uts-liveobjects

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.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.

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

ast-grep timed out on this file


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.

@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features May 13, 2026 12:48 Inactive
@paddybyers
paddybyers requested a review from ttypic May 13, 2026 13:48
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features May 14, 2026 07:18 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features May 29, 2026 12:10 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features June 6, 2026 11:16 Inactive
@ttypic
ttypic force-pushed the uts-liveobjects branch from 400c6a4 to dfcacca Compare June 30, 2026 17:31
@ttypic
ttypic changed the base branch from uts-integration-proxy to main June 30, 2026 17:32
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features July 2, 2026 10:29 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/bundle-report July 2, 2026 10:29 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/typedoc July 2, 2026 10:29 Inactive
@ttypic
ttypic force-pushed the uts-liveobjects branch from 6d756dd to 91ebdf1 Compare July 2, 2026 10:41
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features July 2, 2026 10:41 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/bundle-report July 2, 2026 10:42 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/typedoc July 2, 2026 10:42 Inactive
paddybyers and others added 4 commits July 2, 2026 11:57
Translate portable UTS test specs from specification/uts/objects/ into
working ably-js TypeScript tests covering PathObject, Instance, batch
operations, LiveCounter/LiveMap CRDT internals, ObjectsPool sync state
machine, RealtimeObject lifecycle, value types, and subscriptions.

19 test files: 14 unit (270 tests), 4 integration (15 tests),
1 proxy fault injection (5 tests). 1 test pending (LiveMap#clear()
not yet implemented).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove client-side port allocation (TOCTOU race) and let the proxy
auto-assign free ports via OS. Read the assigned port from the session
creation response. Update binary to v0.2.0 with new asset naming.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds parentReferences, PublicAPI::ObjectMessage, RTO24/25/26 tests;
updates objectMessage/subscription assertions; removes batch API tests.
308 passing, 4 pending deviations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The webpack build type-checks the whole tsconfig program (which
includes test/**/*), so type errors in the UTS LiveObjects tests
failed `npm run build` and therefore `npm ci`.

 - Add siteCode and objectsGCGracePeriod to the mock CONNECTED
   message's connectionDetails defaults, so tests can set these
   real protocol fields without widening a mismatched type.
 - Pass the LiveObjects plugin module namespace to `plugins`
   instead of the LiveObjects const export, matching the internal
   ModularPlugins type (typeof import('plugins/liveobjects')) and
   the existing JS tests. Runtime behaviour is unchanged.
@ttypic
ttypic force-pushed the uts-liveobjects branch from 91ebdf1 to 4d0fa70 Compare July 2, 2026 10:57
@ttypic
ttypic marked this pull request as ready for review July 2, 2026 10:57

@ttypic ttypic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

sacOO7 added 2 commits July 6, 2026 22:49
…son+msgpack variants

- objects_faults: restructure the RTO20e proxy test to the corrected spec
  sequence (ably/specification#501) — force SYNCING via injected ATTACHED
  with HAS_OBJECTS, mutate while SYNCING, wait for the publish ACK in the
  proxy log, then inject channel ERROR and assert the pending operation
  fails with 92008 / statusCode 400 / cause 90000 (RTO20e1). Removes the
  90001 DEVIATION, which documented the spec error now fixed at source.
- objects_gc: replace the deviation comments with the real spec references —
  undefined reads are RTLM5d2h-compliant (spec says undefined/null), and the
  timer-based GC sweep is covered at the unit tier (RTO10/RTO10b1/RTLM19
  unit tests), so only observable tombstone semantics belong here.
- objects_sync: align the RTO4 subscribe-only test title with the corrected
  spec wording and drop its DEVIATION comment.
- Add describeWithProtocols helper and run objects_lifecycle, objects_sync
  and objects_gc once per protocol variant (json, msgpack) as their specs
  require, without any command-line or environment parameter.

All suites verified against the nonprod sandbox: objects_faults 5/5,
lifecycle/sync/gc 24/24 across both protocol variants.
[AIT-1130] fix(uts): align objects integration tests with corrected specs and run both protocol variants
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features July 8, 2026 12:55 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/bundle-report July 8, 2026 12:55 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/typedoc July 8, 2026 12:55 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
test/uts/objects/integration/objects_lifecycle.test.ts (1)

358-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider tracking or closing the REST client.

The REST client is neither tracked via trackClient() nor explicitly closed. While REST clients don't maintain persistent connections like realtime clients, tracking it would ensure consistent cleanup if trackClient supports REST clients.

🤖 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/uts/objects/integration/objects_lifecycle.test.ts` around lines 358 -
363, The REST client created in the objects lifecycle test is not being cleaned
up consistently. Update the setup around Ably.Rest so the client is either
passed through trackClient() if REST clients are supported there, or explicitly
closed during test cleanup using the existing teardown flow. Use the restClient
symbol in objects_lifecycle.test.ts to locate the creation and ensure it is
included in the same lifecycle management pattern as other clients.
test/uts/objects/integration/objects_sync.test.ts (1)

49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Drop the as any casts on these client options
The same option shape is used in objects_lifecycle.test.ts and objects_gc.test.ts without a cast, so these as any annotations only add noise. Remove them from the matching literals in this file.

🤖 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/uts/objects/integration/objects_sync.test.ts` around lines 49 - 55, The
Ably.Realtime client options in objects_sync.test.ts are already typed
correctly, so the `as any` cast is unnecessary noise. Remove the cast from the
option literal passed to `new Ably.Realtime` in the `client` setup, matching the
same pattern used in `objects_lifecycle.test.ts` and `objects_gc.test.ts` while
keeping the option shape unchanged.
🤖 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.

Nitpick comments:
In `@test/uts/objects/integration/objects_lifecycle.test.ts`:
- Around line 358-363: The REST client created in the objects lifecycle test is
not being cleaned up consistently. Update the setup around Ably.Rest so the
client is either passed through trackClient() if REST clients are supported
there, or explicitly closed during test cleanup using the existing teardown
flow. Use the restClient symbol in objects_lifecycle.test.ts to locate the
creation and ensure it is included in the same lifecycle management pattern as
other clients.

In `@test/uts/objects/integration/objects_sync.test.ts`:
- Around line 49-55: The Ably.Realtime client options in objects_sync.test.ts
are already typed correctly, so the `as any` cast is unnecessary noise. Remove
the cast from the option literal passed to `new Ably.Realtime` in the `client`
setup, matching the same pattern used in `objects_lifecycle.test.ts` and
`objects_gc.test.ts` while keeping the option shape unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 996a5b7a-2a1d-4cf7-909f-1eaa2467be00

📥 Commits

Reviewing files that changed from the base of the PR and between 4d0fa70 and 88a3fa0.

📒 Files selected for processing (5)
  • test/uts/objects/integration/objects_gc.test.ts
  • test/uts/objects/integration/objects_lifecycle.test.ts
  • test/uts/objects/integration/objects_sync.test.ts
  • test/uts/objects/integration/protocol_variants.ts
  • test/uts/objects/integration/proxy/objects_faults.test.ts

sacOO7 added 2 commits July 10, 2026 15:43
…nd close audit gaps

Squash of the five stages of this PR (#2257):

1. Align objects unit tests with the reconciled UTS specs and close
   audit gaps: DETACHED/FAILED channel-lifecycle scenarios (RTO23e re-
   attach, RTO25b client-side detach, RTO20e1 ERROR->FAILED stimulus),
   restored missing/vacuous assertions (statusCode 400 rejections,
   generator-consuming [...root.keys()] precondition checks, RTO3/RTO4b
   zero-value root and update-diff assertions), quiescence controls for
   negative assertions, and UTS tag/Test-ID alignment.

2. Sync unit tests with the corrected spec fixtures and canonical serial
   helpers: SITE_CODE / ackSerial / remoteSerial / belowAckSerial
   replace hand-rolled sort-unsafe "t:N" literals; counter fixtures use
   residual count 0 with the initial value on the createOp (spec issue
   SI-1) so sync materialises count + createOp.count exactly once.

3. fix(liveobjects): never remove root from the pool during sync
   (RTO5c2a): ObjectsPool.deleteExtraObjectIds now excludes
   ROOT_OBJECT_ID, so a completed sync that omits root no longer kills
   root subscriptions. Un-skips the three path_object_subscribe tests
   this bug (and stale deviation notes) kept gated: RTPO19e2 (sync-
   triggered updates dispatch with message omitted), RTO24b2a (map-key
   candidate paths), RTO24b2b (once per dispatch).

4. deviations.md: remove entries fixed by the above (path_object_
   subscribe trio, RTINS16e2); record RTO18d under "Spec Points Under
   Review"; document four previously comment-only deviations (RTLC9/
   RTLC16 missing-field counter ops applied instead of noop; the
   RTLO4a/RTLC7d3/RTLM9b/RTLM15d4 throw-instead-of-false family;
   Instance id as getter; RTLC12e1 null increment defaulting to 1) and
   correct the stale RTLM12 keys()-via-instance() justification.

5. Review fixes: shared pollUntil helper in test/uts/helpers.ts (unit-
   tier equivalent of the spec's poll_until; flushAsync-hop interval,
   throws on timeout, matching the integration pollUntil and ably-java)
   replacing 21 hand-rolled condition polls; this.timeout(6000) on the
   objects suites whose quiescence waits exceed mocha's --no-config 2s
   default; captureNotifyUpdated shared helper with restore(); public
   size() instead of private _dataRef reach-in.

Full UTS unit suite: 1350 passing / 0 failing / 57 pending.
[AIT-1131] fix(uts): align objects unit tests with reconciled specs and close audit gaps
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/features July 10, 2026 10:14 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/bundle-report July 10, 2026 10:15 Inactive
@github-actions
github-actions Bot temporarily deployed to staging/pull/2219/typedoc July 10, 2026 10:15 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
test/uts/objects/helpers/standard_test_pool.ts (1)

387-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract shared setup logic to reduce duplication between setupSyncedChannel and setupSyncedChannelNoAck

These functions share ~35 lines of identical MockWebSocket connection setup, client creation, and channel initialization. Only the onMessageFromClient handler differs (ACK vs no-ACK). Extracting a shared helper eliminates the copy and prevents divergent changes.

♻️ Proposed refactor
+async function setupChannel(
+  channelName: string,
+  onMessageFromClient: (msg: any, mockWs: any) => void,
+): Promise<SyncedChannelResult> {
+  const mockWs = new MockWebSocket({
+    onConnectionAttempt: (conn) => {
+      mockWs.active_connection = conn;
+      conn.respond_with_connected({
+        action: PM_ACTION.CONNECTED,
+        connectionId: 'conn-1',
+        connectionDetails: {
+          connectionKey: 'conn-key-1',
+          connectionStateTtl: 120000,
+          maxIdleInterval: 15000,
+          maxMessageSize: 65536,
+          serverId: 'test-server',
+          clientId: null,
+          siteCode: SITE_CODE,
+          objectsGCGracePeriod: 86400000,
+        },
+      });
+    },
+    onMessageFromClient: (msg: any) => onMessageFromClient(msg, mockWs),
+  });
+  installMockWebSocket(mockWs.constructorFn);
+  const client = new Ably.Realtime({
+    key: 'appId.keyId:keySecret',
+    autoConnect: false,
+    useBinaryProtocol: false,
+    plugins: { LiveObjects: LiveObjectsPlugin },
+  });
+  trackClient(client);
+  client.connect();
+  await new Promise<void>((resolve) => client.connection.once('connected', resolve));
+  const channel = client.channels.get(channelName, { modes: ['OBJECT_SUBSCRIBE', 'OBJECT_PUBLISH'] });
+  const root = await channel.object.get();
+  return { client, channel, root, mockWs };
+}
+
 export async function setupSyncedChannel(channelName: string): Promise<SyncedChannelResult> {
-  const mockWs = new MockWebSocket({
-    onConnectionAttempt: (conn) => {
-      mockWs.active_connection = conn;
-      conn.respond_with_connected({
-        action: PM_ACTION.CONNECTED,
-        connectionId: 'conn-1',
-        connectionDetails: {
-          connectionKey: 'conn-key-1',
-          connectionStateTtl: 120000,
-          maxIdleInterval: 15000,
-          maxMessageSize: 65536,
-          serverId: 'test-server',
-          clientId: null,
-          siteCode: SITE_CODE,
-          objectsGCGracePeriod: 86400000,
-        },
-      });
-    },
-    onMessageFromClient: (msg: any) => {
-      if (msg.action === PM_ACTION.ATTACH) {
-        mockWs.active_connection!.send_to_client({
-          action: PM_ACTION.ATTACHED,
-          channel: msg.channel,
-          channelSerial: 'sync1:',
-          flags: HAS_OBJECTS,
-        });
-        mockWs.active_connection!.send_to_client(buildObjectSyncMessage(msg.channel, 'sync1:', STANDARD_POOL_OBJECTS));
-      } else if (msg.action === PM_ACTION.OBJECT) {
-        const serials = (msg.state || []).map((_: any, i: number) => ackSerial(msg.msgSerial, i));
-        mockWs.active_connection!.send_to_client(buildAckMessage(msg.msgSerial, serials));
-      }
-    },
-  });
-  installMockWebSocket(mockWs.constructorFn);
-
-  const client = new Ably.Realtime({
-    key: 'appId.keyId:keySecret',
-    autoConnect: false,
-    useBinaryProtocol: false,
-    plugins: { LiveObjects: LiveObjectsPlugin },
-  });
-  trackClient(client);
-  client.connect();
-  await new Promise<void>((resolve) => client.connection.once('connected', resolve));
-
-  const channel = client.channels.get(channelName, { modes: ['OBJECT_SUBSCRIBE', 'OBJECT_PUBLISH'] });
-  const root = await channel.object.get();
-
-  return { client, channel, root, mockWs };
+  return setupChannel(channelName, (msg, mockWs) => {
+    if (msg.action === PM_ACTION.ATTACH) {
+      mockWs.active_connection!.send_to_client({
+        action: PM_ACTION.ATTACHED,
+        channel: msg.channel,
+        channelSerial: 'sync1:',
+        flags: HAS_OBJECTS,
+      });
+      mockWs.active_connection!.send_to_client(buildObjectSyncMessage(msg.channel, 'sync1:', STANDARD_POOL_OBJECTS));
+    } else if (msg.action === PM_ACTION.OBJECT) {
+      const serials = (msg.state || []).map((_: any, i: number) => ackSerial(msg.msgSerial, i));
+      mockWs.active_connection!.send_to_client(buildAckMessage(msg.msgSerial, serials));
+    }
+  });
 }

setupSyncedChannelNoAck would similarly delegate to setupChannel with an ATTACH-only handler.

🤖 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/uts/objects/helpers/standard_test_pool.ts` around lines 387 - 486,
Extract the duplicated MockWebSocket connection, client initialization,
connection wait, channel creation, and root retrieval from setupSyncedChannel
and setupSyncedChannelNoAck into a shared helper such as setupChannel that
accepts the channel name and onMessageFromClient behavior; have both functions
delegate to it, supplying their respective ACK-enabled and ATTACH-only handlers
while preserving the returned SyncedChannelResult.
src/plugins/liveobjects/objectspool.ts (1)

51-53: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider using a Set for O(1) membership checks.

objectIds.includes(x) is O(n) per pool entry, making the overall complexity O(P×N). Converting objectIds to a Set would reduce this to O(P+N).

♻️ Proposed refactor
   deleteExtraObjectIds(objectIds: string[]): void {
     const poolObjectIds = [...this._pool.keys()];
-    const extraObjectIds = poolObjectIds.filter((x) => !objectIds.includes(x) && x !== ROOT_OBJECT_ID);
+    const receivedIds = new Set(objectIds);
+    const extraObjectIds = poolObjectIds.filter((x) => !receivedIds.has(x) && x !== ROOT_OBJECT_ID);
     extraObjectIds.forEach((x) => this._pool.delete(x));
🤖 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/plugins/liveobjects/objectspool.ts` around lines 51 - 53, Update
deleteExtraObjectIds to convert objectIds into a Set before filtering
poolObjectIds, then use set membership instead of objectIds.includes(x) while
preserving the ROOT_OBJECT_ID exclusion.
🤖 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/plugins/liveobjects/objectspool.ts`:
- Around line 47-53: The GC eviction path in _onGCInterval can remove
ROOT_OBJECT_ID after it is tombstoned. Add a ROOT_OBJECT_ID exclusion to the
tombstone cleanup filter or condition, matching the guard in
deleteExtraObjectIds, so the root is never deleted from the pool.

---

Nitpick comments:
In `@src/plugins/liveobjects/objectspool.ts`:
- Around line 51-53: Update deleteExtraObjectIds to convert objectIds into a Set
before filtering poolObjectIds, then use set membership instead of
objectIds.includes(x) while preserving the ROOT_OBJECT_ID exclusion.

In `@test/uts/objects/helpers/standard_test_pool.ts`:
- Around line 387-486: Extract the duplicated MockWebSocket connection, client
initialization, connection wait, channel creation, and root retrieval from
setupSyncedChannel and setupSyncedChannelNoAck into a shared helper such as
setupChannel that accepts the channel name and onMessageFromClient behavior;
have both functions delegate to it, supplying their respective ACK-enabled and
ATTACH-only handlers while preserving the returned SyncedChannelResult.
🪄 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: 5bca0fb2-0cbe-4e7a-ab72-3647eb61f89d

📥 Commits

Reviewing files that changed from the base of the PR and between 88a3fa0 and a35b2a7.

📒 Files selected for processing (22)
  • src/plugins/liveobjects/objectspool.ts
  • test/uts/deviations.md
  • test/uts/helpers.ts
  • test/uts/objects/helpers/standard_test_pool.ts
  • test/uts/objects/unit/instance.test.ts
  • test/uts/objects/unit/live_counter.test.ts
  • test/uts/objects/unit/live_map.test.ts
  • test/uts/objects/unit/live_map_api.test.ts
  • test/uts/objects/unit/live_object_subscribe.test.ts
  • test/uts/objects/unit/object_id.test.ts
  • test/uts/objects/unit/objects_pool.test.ts
  • test/uts/objects/unit/parent_references.test.ts
  • test/uts/objects/unit/path_object.test.ts
  • test/uts/objects/unit/path_object_mutations.test.ts
  • test/uts/objects/unit/path_object_subscribe.test.ts
  • test/uts/objects/unit/realtime_object.test.ts
  • test/uts/objects/unit/value_types.test.ts
  • test/uts/realtime/unit/auth/auth_callback_errors.test.ts
  • test/uts/realtime/unit/auth/connection_auth.test.ts
  • test/uts/realtime/unit/channels/channel_properties.test.ts
  • test/uts/realtime/unit/channels/channel_server_initiated_detach.test.ts
  • test/uts/realtime/unit/presence/realtime_presence_reentry.test.ts
✅ Files skipped from review due to trivial changes (1)
  • test/uts/objects/unit/object_id.test.ts

Comment on lines +47 to +53
*
* @spec RTO5c2 - remove objects whose ids were not received during the sync sequence
* @spec RTO5c2a - the root object must never be removed (RTO3b), even if absent from the sync
*/
deleteExtraObjectIds(objectIds: string[]): void {
const poolObjectIds = [...this._pool.keys()];
const extraObjectIds = poolObjectIds.filter((x) => !objectIds.includes(x));
const extraObjectIds = poolObjectIds.filter((x) => !objectIds.includes(x) && x !== ROOT_OBJECT_ID);

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== files ==\n'
git ls-files 'src/plugins/liveobjects/*' | sed -n '1,120p'

printf '\n== outline objectspool.ts ==\n'
ast-grep outline src/plugins/liveobjects/objectspool.ts --view expanded || true

printf '\n== outline realtimeobject.ts ==\n'
ast-grep outline src/plugins/liveobjects/realtimeobject.ts --view expanded || true

printf '\n== root object references ==\n'
rg -n "ROOT_OBJECT_ID|root" src/plugins/liveobjects -g '!**/*.map'

printf '\n== object delete / tombstone paths ==\n'
rg -n "tombston|OBJECT_DELETE|deleteExtraObjectIds|onGCInterval|gcGracePeriod" src/plugins/liveobjects -g '!**/*.map'

Repository: ably/ably-js

Length of output: 19992


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== objectspool.ts 110-140 ==\n'
sed -n '110,140p' src/plugins/liveobjects/objectspool.ts | cat -n

printf '\n== liveobject.ts 100-150 ==\n'
sed -n '100,150p' src/plugins/liveobjects/liveobject.ts | cat -n

printf '\n== realtimeobject.ts 500-560 ==\n'
sed -n '500,560p' src/plugins/liveobjects/realtimeobject.ts | cat -n

printf '\n== livemap.ts 340-380 ==\n'
sed -n '340,380p' src/plugins/liveobjects/livemap.ts | cat -n

printf '\n== livemap.ts 460-490 ==\n'
sed -n '460,490p' src/plugins/liveobjects/livemap.ts | cat -n

Repository: ably/ably-js

Length of output: 8892


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== root guards in liveobject/livemap/objectspool ==\n'
rg -n "ROOT_OBJECT_ID|getObjectId\\(\\) === ROOT_OBJECT_ID|objectId === ROOT_OBJECT_ID|root object" src/plugins/liveobjects/liveobject.ts src/plugins/liveobjects/livemap.ts src/plugins/liveobjects/objectspool.ts

printf '\n== liveobject.ts 1-100 ==\n'
sed -n '1,100p' src/plugins/liveobjects/liveobject.ts | cat -n

printf '\n== liveobject.ts 150-320 ==\n'
sed -n '150,320p' src/plugins/liveobjects/liveobject.ts | cat -n

printf '\n== livemap.ts 380-460 ==\n'
sed -n '380,460p' src/plugins/liveobjects/livemap.ts | cat -n

Repository: ably/ably-js

Length of output: 16896


Keep the root object out of GC eviction
_onGCInterval still deletes any tombstoned object after the grace period, including ROOT_OBJECT_ID. Since OBJECT_DELETE can tombstone any objectId and there’s no root-specific guard here, the root can be removed from the pool and violate RTO3b. Add the same x !== ROOT_OBJECT_ID protection in the GC path.

🤖 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/plugins/liveobjects/objectspool.ts` around lines 47 - 53, The GC eviction
path in _onGCInterval can remove ROOT_OBJECT_ID after it is tombstoned. Add a
ROOT_OBJECT_ID exclusion to the tombstone cleanup filter or condition, matching
the guard in deleteExtraObjectIds, so the root is never deleted from the pool.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay, we are already avoiding deleting rootObject by checking poolObjectIds.filter((x) => !objectIds.includes(x) && x !== ROOT_OBJECT_ID);, can you double check

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@sacOO7 You're right that deleteExtraObjectIds correctly excludes ROOT_OBJECT_ID now — that part of the diff is fine and addresses RTO5c2/RTO5c2a.

However, my comment was actually about a separate method: _onGCInterval (Lines 119-133), which is untouched by this diff. That method deletes objects purely based on tombstone state and grace period, with no ROOT_OBJECT_ID check:

private _onGCInterval(): void {
  const toDelete: string[] = [];
  for (const [objectId, obj] of this._pool.entries()) {
    if (obj.isTombstoned() && Date.now() - obj.tombstonedAt()! >= this._realtimeObject.gcGracePeriod) {
      toDelete.push(objectId);
      continue;
    }
    obj.onGCInterval();
  }
  toDelete.forEach((x) => this._pool.delete(x));
}

If the root object were ever tombstoned (e.g., via an OBJECT_DELETE operation targeting the root's objectId), this GC sweep would remove it from the pool after the grace period elapses — independent of deleteExtraObjectIds. Since getRoot() retrieves the root purely by looking up ROOT_OBJECT_ID in the pool, losing that entry would break getRoot() and violate RTO3b (root object must never be removed).

So to fully close this out, the same guard would need to be added in _onGCInterval, e.g.:

if (objectId !== ROOT_OBJECT_ID && obj.isTombstoned() && Date.now() - obj.tombstonedAt()! >= this._realtimeObject.gcGracePeriod) {

Let me know if you'd like to fix this in this PR, or if there's an invariant elsewhere that guarantees the root can never actually be tombstoned (in which case this GC path would be moot).

(・ω・) still curious about this one

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay, we need to check the case with respect to spec, thanks for raising

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, need to add UTS test spec for the same 👍

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@sacOO7 Sounds good — adding a UTS test spec (and the corresponding guard in _onGCInterval if the spec confirms the root must be protected there too) will make this coverage complete. Happy to help review that test once it's added, or take a closer look at the spec wording for RTO3b if that'd be useful.

(๑˃̵ᴗ˂̵)و

sacOO7 added 3 commits July 10, 2026 22:32
The spec deprecated RTPO8d (primitive resolution returns undefined) in
favour of RTPO8f: instance() returns an Instance wrapping any resolved
value, LiveObject or primitive. DefaultInstance already supports
primitive values (id returns undefined, value() returns the primitive
directly), so _resolveInstance now wraps unconditionally and never
returns undefined; only path-resolution failure (RTPO8e) yields
undefined from instance().

batch() previously used _resolveInstance() truthiness as its implicit
"is this a LiveObject" check; it now checks the resolved value with an
explicit instanceof guard, preserving the 92007 non-LiveObject and
92005 unresolved-path errors byte-for-byte.

Update the derived UTS test (RTPO8c primitive-null test becomes
RTPO8f/instance-primitive-wrapped-0), the legacy sandbox assertion,
and the AnyPathObject.instance() docstring.

Spec: ably/specification#504
…tch() comment

- Add the missing doc comment on DefaultPathObject.instance() stating the
  RTPO8f contract: wraps LiveObjects and primitives, undefined only when
  the path does not resolve.
- Reword the batch() comment to state the invariant instead of referencing
  _resolveInstance, which batch() no longer calls.
[AIT-1138] fix(liveobjects): `PathObject.instance()` wraps primitives per RTPO8f
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.

3 participants