[PUB-1837] Use server-provided timestamp to sweep old tombstones#2054
[PUB-1837] Use server-provided timestamp to sweep old tombstones#2054
Conversation
|
Warning Rate limit exceeded@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughThe changes update the handling of tombstoned objects and map entries to consistently use a server-provided numeric timestamp ( Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Client
participant LiveObject/LiveMap
Server->>Client: Send ObjectMessage (with serialTimestamp)
Client->>LiveObject/LiveMap: Apply operation (pass ObjectMessage)
LiveObject/LiveMap->>LiveObject/LiveMap: Tombstone object/entry (use serialTimestamp if present)
LiveObject/LiveMap->>Client: Expose tombstonedAt property (from serialTimestamp or local clock)
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/2054/bundle-report
VeskeR
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/plugins/objects/livemap.ts (1)
692-692: Nice improvement to error message formatting.Adding quotes around the key value improves error message clarity, especially for keys containing spaces or special characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/plugins/objects/livecounter.ts(4 hunks)src/plugins/objects/livemap.ts(9 hunks)src/plugins/objects/liveobject.ts(5 hunks)src/plugins/objects/objectmessage.ts(4 hunks)src/plugins/objects/objects.ts(2 hunks)src/plugins/objects/syncobjectsdatapool.ts(3 hunks)test/common/modules/objects_helper.js(2 hunks)test/common/modules/private_api_recorder.js(1 hunks)test/realtime/objects.test.js(6 hunks)
🧰 Additional context used
🧠 Learnings (9)
test/common/modules/private_api_recorder.js (2)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file `test/realtime/live_objects.test.js`, the field name `regionalTimeserial` is correct and should not be changed to `timeserial`.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
src/plugins/objects/objects.ts (4)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file `test/realtime/live_objects.test.js`, the field name `regionalTimeserial` is correct and should not be changed to `timeserial`.
src/plugins/objects/objectmessage.ts (2)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file `test/realtime/live_objects.test.js`, the field name `regionalTimeserial` is correct and should not be changed to `timeserial`.
test/common/modules/objects_helper.js (3)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file `test/realtime/live_objects.test.js`, the field name `regionalTimeserial` is correct and should not be changed to `timeserial`.
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
src/plugins/objects/syncobjectsdatapool.ts (6)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
Learnt from: VeskeR
PR: ably/ably-js#1880
File: liveobjects.d.ts:26-26
Timestamp: 2024-10-08T15:37:26.765Z
Learning: For plugins like `LiveObjects`, the `any` type is intentionally used to maintain flexibility.
Learnt from: VeskeR
PR: ably/ably-js#2052
File: ably.d.ts:2608-2621
Timestamp: 2025-06-27T08:37:06.539Z
Learning: In ably.d.ts, null values are not supported in LiveMap objects. The PrimitiveObjectValue type correctly excludes null even though JsonScalar includes it, as this reflects the intentional design limitation of the LiveMap implementation.
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-10-22T13:26:59.680Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
test/realtime/objects.test.js (3)
undefined
<retrieved_learning>
Learnt from: VeskeR
PR: #1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file test/realtime/live_objects.test.js, the field name regionalTimeserial is correct and should not be changed to timeserial.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as test/common/modules/live_objects_helper.js, concerns about assignments within expressions are not applicable.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1926
File: test/realtime/live_objects.test.js:2078-2117
Timestamp: 2024-11-28T02:13:58.582Z
Learning: In this project, Mocha tests have an inherent default timeout of 60 seconds, so adding explicit timeouts in the test code is unnecessary.
</retrieved_learning>
src/plugins/objects/liveobject.ts (5)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file `test/realtime/live_objects.test.js`, the field name `regionalTimeserial` is correct and should not be changed to `timeserial`.
Learnt from: VeskeR
PR: ably/ably-js#2052
File: ably.d.ts:2608-2621
Timestamp: 2025-06-27T08:37:06.539Z
Learning: In ably.d.ts, null values are not supported in LiveMap objects. The PrimitiveObjectValue type correctly excludes null even though JsonScalar includes it, as this reflects the intentional design limitation of the LiveMap implementation.
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
src/plugins/objects/livecounter.ts (3)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-10-22T13:26:59.680Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
src/plugins/objects/livemap.ts (4)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
Learnt from: VeskeR
PR: ably/ably-js#2052
File: ably.d.ts:2608-2621
Timestamp: 2025-06-27T08:37:06.539Z
Learning: In ably.d.ts, null values are not supported in LiveMap objects. The PrimitiveObjectValue type correctly excludes null even though JsonScalar includes it, as this reflects the intentional design limitation of the LiveMap implementation.
Learnt from: VeskeR
PR: ably/ably-js#1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file `test/realtime/live_objects.test.js`, the field name `regionalTimeserial` is correct and should not be changed to `timeserial`.
🧬 Code Graph Analysis (2)
src/plugins/objects/objects.ts (2)
src/plugins/objects/livecounter.ts (1)
LiveCounter(22-355)src/plugins/objects/livemap.ts (1)
LiveMap(48-909)
src/plugins/objects/liveobject.ts (1)
src/plugins/objects/objectmessage.ts (1)
ObjectMessage(356-437)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-npm-package
🔇 Additional comments (24)
src/plugins/objects/objectmessage.ts (4)
93-94: LGTM: Consistent addition of serialTimestamp propertyThe
serialTimestampproperty is correctly added to theObjectsMapEntryinterface with proper typing and documentation. The comment appropriately indicates it's only present whentombstoneistrue.
337-337: LGTM: Proper property copying in copyMsg functionThe
serialTimestampproperty is correctly copied in thecopyMsgfunction, maintaining consistency with other properties being copied.
376-377: LGTM: Consistent addition to ObjectMessage classThe
serialTimestampproperty is properly added to theObjectMessageclass with appropriate typing and documentation indicating its derivation from theserialfield.
464-465: LGTM: Consistent addition to WireObjectMessage classThe
serialTimestampproperty is properly added to theWireObjectMessageclass, maintaining consistency with theObjectMessageclass.test/common/modules/private_api_recorder.js (1)
19-19: LGTM: Proper addition of new private API identifierThe new
'call.LiveObject.tombstonedAt'identifier is correctly added to support testing of the new tombstone timestamp functionality. It follows the established naming pattern for private API identifiers.src/plugins/objects/objects.ts (3)
403-403: LGTM: Consistent refactor to use ObjectMessageThe change from
entry.objectStatetoentry.objectMessageis correct and aligns with the broader refactor to useObjectMessageinstead ofObjectState. TheoverrideWithObjectStatemethod signature in the relevant code snippets confirms it now acceptsObjectMessage.
416-416: LGTM: Consistent refactor for LiveCounter creationThe change to use
entry.objectMessageis correct and consistent with theLiveCounter.fromObjectStatemethod signature shown in the relevant code snippets, which now acceptsObjectMessage.
420-420: LGTM: Consistent refactor for LiveMap creationThe change to use
entry.objectMessageis correct and consistent with theLiveMap.fromObjectStatemethod signature shown in the relevant code snippets, which now acceptsObjectMessage.test/realtime/objects.test.js (7)
780-780: LGTM: Improved test descriptions for clarity.The terminology has been updated from "object state 'tombstone' property" to the clearer "tombstone=true" for an object, which better reflects the actual implementation and is more consistent with the codebase terminology.
Also applies to: 834-834, 887-887
940-980: Well-implemented test for serialTimestamp handling in OBJECT_SYNC.This test thoroughly validates that tombstoned objects created during OBJECT_SYNC sequences properly use the
serialTimestampwhen provided. The test correctly:
- Sets up a tombstoned counter with a specific
serialTimestamp- Accesses private API to verify the object is in the pool
- Validates the
tombstonedAtproperty matches the provided timestampThe use of
helper.recordPrivateApi()properly documents the private API access.
982-1022: Good fallback test for missing serialTimestamp.This test appropriately validates the fallback behavior when
serialTimestampis not provided. The timing check usingMath.abs(tombstonedAt - Date.now()) < 1000is reasonable for test stability while still ensuring the local clock is used.
1024-1100: Comprehensive map entry tombstone timestamp tests.These two test cases properly cover both the main path (with
serialTimestamp) and fallback path (withoutserialTimestamp) for map entry tombstoning during OBJECT_SYNC. The tests correctly access the internal map data structure to verify tombstone state.
1625-1673: Well-structured MAP_REMOVE tombstone timestamp tests.These tests provide excellent coverage for the MAP_REMOVE operation's tombstone timestamp handling. The pattern of testing both
serialTimestampprovided and fallback scenarios is consistent with other similar tests in the suite.
2129-2201: Thorough OBJECT_DELETE tombstone timestamp validation.The OBJECT_DELETE tests follow the established pattern and properly validate:
- Object creation and verification before deletion
- Tombstone state verification after deletion
- Timestamp handling for both
serialTimestampprovided and fallback casesThe tests appropriately use private API access to verify internal object pool state.
1018-1020: Verify timing tolerance is appropriate for test stability.The 1000ms tolerance for local clock timestamp validation (
Math.abs(tombstonedAt - Date.now()) < 1000) appears across multiple tests. While this should be sufficient for normal test execution, consider if this tolerance is appropriate for slower CI environments.The current tolerance seems reasonable, but monitor for potential flaky test failures if this proves insufficient in practice.
Also applies to: 1096-1098, 1669-1671, 2196-2198
test/common/modules/objects_helper.js (2)
243-249: LGTM! Proper propagation of serialTimestamp.The method correctly propagates the
serialTimestampfield to each object message, maintaining consistency with the existing pattern forserialandsiteCodefields.
260-265: LGTM! Consistent handling of serialTimestamp in state messages.The implementation correctly sets the
serialTimestampon each object message in the state array, which aligns with the PR's objective of propagating server-provided timestamps.src/plugins/objects/syncobjectsdatapool.ts (1)
3-97: Clean refactor to use ObjectMessage throughout.The changes correctly update the
SyncObjectsDataPoolto work withObjectMessageinstead ofObjectState, maintaining the same validation logic while enabling access to additional message fields likeserialTimestamp. The error handling and logging remain intact.src/plugins/objects/livecounter.ts (2)
236-241: Excellent null safety implementation.The method properly validates the object state existence with a clear error message before proceeding. The tombstone call correctly passes the full
objectMessageto enable access toserialTimestamp.Also applies to: 281-281
39-41: Safe to keep non-null assertion in fromObjectStateThe
overrideWithObjectStatecall immediately checks forobjectMessage.object == nulland throws a clearErrorInfoif missing, andfromObjectStateis only ever invoked for initial state (COUNTER_CREATE) messages whereobjectis guaranteed to exist. No additional null check is needed.src/plugins/objects/liveobject.ts (1)
154-166: Well-implemented timestamp handling with proper fallback.The tombstone method correctly prioritizes the server-provided
serialTimestampand falls back to local clock time with appropriate logging. This aligns perfectly with the PR's objective of using server timestamps for tombstone timing.src/plugins/objects/livemap.ts (3)
736-765: Consistent timestamp handling for map entry removal.The implementation correctly handles the
opTimestampparameter with proper fallback to local clock and logging. The pattern is consistent with the tombstone handling inLiveObject.
837-850: Proper timestamp initialization for tombstoned map entries.The implementation correctly sets
tombstonedAtfromserialTimestampwhen creating map data from entries, with consistent fallback and logging behavior.
73-75: Verify non-null assertions in LiveMap.fromObjectStateThe
objectMessage.object!.map!andobjectMessage.object!.objectIdassertions insrc/plugins/objects/livemap.ts(lines 73–75) mirror those inLiveCounter.fromObjectState, but could throw ifobjectormapis ever undefined. Please ensure that:
objectMessage.objectand itsmapfield are guaranteed to be populated before callingfromObjectState.- You document this invariant in the code or add an explicit runtime check with a clear error if the assertion fails.
mschristensen
left a comment
mschristensen
left a comment
There was a problem hiding this comment.
Nice, minor suggestion but approving ahead of that :)
VeskeR
force-pushed
the
PUB-1702/objects-server-gc
branch
from
49ae8c8 to
79c3f75
Compare
VeskeR
force-pushed
the
PUB-1837/server-provided-tombstone-timestamp
branch
from
7356070 to
b0ed710
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2054/bundle-report
VeskeR
force-pushed
the
PUB-1837/server-provided-tombstone-timestamp
branch
from
b0ed710 to
8313fbe
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2054/bundle-report
Use `serialTimestamp` field from ObjectMessage to know when a tombstone was created for an object or a map entry. Resolves PUB-1837
VeskeR
force-pushed
the
PUB-1837/server-provided-tombstone-timestamp
branch
from
8313fbe to
7c7033b
Compare
2 participants
Use
serialTimestampfield from ObjectMessage to know when a tombstone was created for an object or a map entry.Resolves PUB-1837
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores