Use server-provided timestamp to sweep old tombstones

Use `serialTimestamp` field from ObjectMessage to know when a tombstone
was created for an object or a map entry.

Resolves PUB-1837

Author: VeskeR

src/plugins/objects/livecounter.ts

@@ -7,7 +7,6 @@ import {
   ObjectOperation,
   ObjectOperationAction,
   ObjectsCounterOp,
-  ObjectState,
 } from './objectmessage';
 import { Objects } from './objects';
 
@@ -37,9 +36,9 @@ export class LiveCounter extends LiveObject<LiveCounterData, LiveCounterUpdate>
    *
    * @internal
    */
-  static fromObjectState(objects: Objects, objectState: ObjectState<ObjectData>): LiveCounter {
-    const obj = new LiveCounter(objects, objectState.objectId);
-    obj.overrideWithObjectState(objectState);
+  static fromObjectState(objects: Objects, objectMessage: ObjectMessage): LiveCounter {
+    const obj = new LiveCounter(objects, objectMessage.object!.objectId);
+    obj.overrideWithObjectState(objectMessage);
     return obj;
   }
 
@@ -216,7 +215,7 @@ export class LiveCounter extends LiveObject<LiveCounterData, LiveCounterUpdate>
         break;
 
       case ObjectOperationAction.OBJECT_DELETE:
-        update = this._applyObjectDelete();
+        update = this._applyObjectDelete(msg);
         break;
 
       default:
@@ -234,7 +233,12 @@ export class LiveCounter extends LiveObject<LiveCounterData, LiveCounterUpdate>
    * @internal
    * @spec RTLC6
    */
-  overrideWithObjectState(objectState: ObjectState<ObjectData>): LiveCounterUpdate | LiveObjectUpdateNoop {
+  overrideWithObjectState(objectMessage: ObjectMessage): LiveCounterUpdate | LiveObjectUpdateNoop {
+    const objectState = objectMessage.object;
+    if (objectState == null) {
+      throw new this._client.ErrorInfo(`Missing object state; LiveCounter objectId=${this.getObjectId()}`, 92000, 500);
+    }
+
     if (objectState.objectId !== this.getObjectId()) {
       throw new this._client.ErrorInfo(
         `Invalid object state: object state objectId=${objectState.objectId}; LiveCounter objectId=${this.getObjectId()}`,
@@ -274,7 +278,7 @@ export class LiveCounter extends LiveObject<LiveCounterData, LiveCounterUpdate>
     const previousDataRef = this._dataRef;
     if (objectState.tombstone) {
       // tombstone this object and ignore the data from the object state message
-      this.tombstone();
+      this.tombstone(objectMessage);
     } else {
       // override data for this object with data from the object state
       this._createOperationIsMerged = false; // RTLC6b

src/plugins/objects/livemap.ts

@@ -14,7 +14,6 @@ import {
   ObjectsMapEntry,
   ObjectsMapOp,
   ObjectsMapSemantics,
-  ObjectState,
 } from './objectmessage';
 import { Objects } from './objects';
 
@@ -42,9 +41,6 @@ export type LiveMapObjectData = ObjectIdObjectData | ValueObjectData;
 
 export interface LiveMapEntry {
   tombstone: boolean;
-  /**
-   * Can't use serial from the operation that deleted the entry for the same reason as for {@link LiveObject} tombstones, see explanation there.
-   */
   tombstonedAt: number | undefined;
   timeserial: string | undefined;
   data: LiveMapObjectData | undefined;
@@ -84,12 +80,9 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
    *
    * @internal
    */
-  static fromObjectState<T extends API.LiveMapType>(
-    objects: Objects,
-    objectState: ObjectState<ObjectData>,
-  ): LiveMap<T> {
-    const obj = new LiveMap<T>(objects, objectState.map?.semantics!, objectState.objectId);
-    obj.overrideWithObjectState(objectState);
+  static fromObjectState<T extends API.LiveMapType>(objects: Objects, objectMessage: ObjectMessage): LiveMap<T> {
+    const obj = new LiveMap<T>(objects, objectMessage.object!.map!.semantics!, objectMessage.object!.objectId);
+    obj.overrideWithObjectState(objectMessage);
     return obj;
   }
 
@@ -455,12 +448,12 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
           // leave an explicit return here, so that TS knows that update object is always set after the switch statement.
           return;
         } else {
-          update = this._applyMapRemove(op.mapOp, opSerial);
+          update = this._applyMapRemove(op.mapOp, opSerial, msg.serialTimestamp);
         }
         break;
 
       case ObjectOperationAction.OBJECT_DELETE:
-        update = this._applyObjectDelete();
+        update = this._applyObjectDelete(msg);
         break;
 
       default:
@@ -478,7 +471,12 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
    * @internal
    * @spec RTLM6
    */
-  overrideWithObjectState(objectState: ObjectState<ObjectData>): LiveMapUpdate<T> | LiveObjectUpdateNoop {
+  overrideWithObjectState(objectMessage: ObjectMessage): LiveMapUpdate<T> | LiveObjectUpdateNoop {
+    const objectState = objectMessage.object;
+    if (objectState == null) {
+      throw new this._client.ErrorInfo(`Missing object state; LiveMap objectId=${this.getObjectId()}`, 92000, 500);
+    }
+
     if (objectState.objectId !== this.getObjectId()) {
       throw new this._client.ErrorInfo(
         `Invalid object state: object state objectId=${objectState.objectId}; LiveMap objectId=${this.getObjectId()}`,
@@ -534,7 +532,7 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
     const previousDataRef = this._dataRef;
     if (objectState.tombstone) {
       // tombstone this object and ignore the data from the object state message
-      this.tombstone();
+      this.tombstone(objectMessage);
     } else {
       // override data for this object with data from the object state
       this._createOperationIsMerged = false; // RTLM6b
@@ -644,7 +642,7 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
       let update: LiveMapUpdate<T> | LiveObjectUpdateNoop;
       if (entry.tombstone === true) {
         // RTLM6d1b - entry in MAP_CREATE op is removed, try to apply MAP_REMOVE op
-        update = this._applyMapRemove({ key }, opSerial);
+        update = this._applyMapRemove({ key }, opSerial, entry.serialTimestamp);
       } else {
         // RTLM6d1a - entry in MAP_CREATE op is not removed, try to set it via MAP_SET op
         update = this._applyMapSet({ key, data: entry.data }, opSerial);
@@ -726,7 +724,7 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
         Utils.isNil(op.data.string))
     ) {
       throw new ErrorInfo(
-        `Invalid object data for MAP_SET op on objectId=${this.getObjectId()} on key=${op.key}`,
+        `Invalid object data for MAP_SET op on objectId=${this.getObjectId()} on key="${op.key}"`,
         92000,
         500,
       );
@@ -779,6 +777,7 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
   private _applyMapRemove(
     op: ObjectsMapOp<ObjectData>,
     opSerial: string | undefined,
+    opTimestamp: number | undefined,
   ): LiveMapUpdate<T> | LiveObjectUpdateNoop {
     const existingEntry = this._dataRef.data.get(op.key);
     // RTLM8a
@@ -793,17 +792,30 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
       return { noop: true };
     }
 
+    let tombstonedAt: number;
+    if (opTimestamp != null) {
+      tombstonedAt = opTimestamp;
+    } else {
+      this._client.Logger.logAction(
+        this._client.logger,
+        this._client.Logger.LOG_MINOR,
+        'LiveMap._applyMapRemove()',
+        `map key has been removed but no "serialTimestamp" found in the message, using local clock instead; key="${op.key}", objectId=${this.getObjectId()}`,
+      );
+      tombstonedAt = Date.now(); // best-effort estimate since no timestamp provided by the server
+    }
+
     if (existingEntry) {
       // RTLM8a2
       existingEntry.tombstone = true; // RTLM8a2c
-      existingEntry.tombstonedAt = Date.now();
+      existingEntry.tombstonedAt = tombstonedAt;
       existingEntry.timeserial = opSerial; // RTLM8a2b
       existingEntry.data = undefined; // RTLM8a2a
     } else {
       // RTLM8b, RTLM8b1
       const newEntry: LiveMapEntry = {
         tombstone: true, // RTLM8b2
-        tombstonedAt: Date.now(),
+        tombstonedAt: tombstonedAt,
         timeserial: opSerial,
         data: undefined,
       };
@@ -869,12 +881,27 @@ export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData,
         }
       }
 
+      let tombstonedAt: number | undefined;
+      if (entry.tombstone === true) {
+        if (entry.serialTimestamp != null) {
+          tombstonedAt = entry.serialTimestamp;
+        } else {
+          this._client.Logger.logAction(
+            this._client.logger,
+            this._client.Logger.LOG_MINOR,
+            'LiveMap._liveMapDataFromMapEntries()',
+            `map key is removed but no "serialTimestamp" found, using local clock instead; key="${key}", objectId=${this.getObjectId()}`,
+          );
+          tombstonedAt = Date.now(); // best-effort estimate since no timestamp provided by the server
+        }
+      }
+
       const liveDataEntry: LiveMapEntry = {
         timeserial: entry.timeserial,
         data: liveData,
         // consider object as tombstoned only if we received an explicit flag stating that. otherwise it exists
         tombstone: entry.tombstone === true,
-        tombstonedAt: entry.tombstone === true ? Date.now() : undefined,
+        tombstonedAt,
       };
 
       liveMapData.data.set(key, liveDataEntry);

src/plugins/objects/liveobject.ts

@@ -1,6 +1,6 @@
 import type BaseClient from 'common/lib/client/baseclient';
 import type EventEmitter from 'common/lib/util/eventemitter';
-import { ObjectData, ObjectMessage, ObjectOperation, ObjectState } from './objectmessage';
+import { ObjectData, ObjectMessage, ObjectOperation } from './objectmessage';
 import { Objects } from './objects';
 
 export enum LiveObjectSubscriptionEvent {
@@ -51,14 +51,6 @@ export abstract class LiveObject<
   protected _siteTimeserials: Record<string, string>;
   protected _createOperationIsMerged: boolean;
   private _tombstone: boolean;
-  /**
-   * Even though the {@link ObjectMessage.serial} value from the operation that deleted the object contains the timestamp value,
-   * the serial should be treated as an opaque string on the client, meaning we should not attempt to parse it.
-   *
-   * Therefore, we need to set our own timestamp using local clock when the object is deleted client-side.
-   * Strictly speaking, this does make an assumption about the client clock not being too heavily skewed behind the server,
-   * but it is an acceptable compromise for the time being, as the likelihood of encountering a race here is pretty low given the grace periods we use.
-   */
   private _tombstonedAt: number | undefined;
 
   protected constructor(
@@ -159,9 +151,19 @@ export abstract class LiveObject<
    *
    * @internal
    */
-  tombstone(): TUpdate {
+  tombstone(objectMessage: ObjectMessage): TUpdate {
     this._tombstone = true;
-    this._tombstonedAt = Date.now();
+    if (objectMessage.serialTimestamp != null) {
+      this._tombstonedAt = objectMessage.serialTimestamp;
+    } else {
+      this._client.Logger.logAction(
+        this._client.logger,
+        this._client.Logger.LOG_MINOR,
+        'LiveObject.tombstone()',
+        `object has been tombstoned but no "serialTimestamp" found in the message, using local clock instead; objectId=${this.getObjectId()}`,
+      );
+      this._tombstonedAt = Date.now(); // best-effort estimate since no timestamp provided by the server
+    }
     const update = this.clearData();
     this._lifecycleEvents.emit(LiveObjectLifecycleEvent.deleted);
 
@@ -210,8 +212,8 @@ export abstract class LiveObject<
     return !siteSerial || opSerial > siteSerial;
   }
 
-  protected _applyObjectDelete(): TUpdate {
-    return this.tombstone();
+  protected _applyObjectDelete(objectMessage: ObjectMessage): TUpdate {
+    return this.tombstone(objectMessage);
   }
 
   /**
@@ -221,7 +223,7 @@ export abstract class LiveObject<
    */
   abstract applyOperation(op: ObjectOperation<ObjectData>, msg: ObjectMessage): void;
   /**
-   * Overrides internal data for this LiveObject with data from the given object state.
+   * Overrides internal data for this LiveObject with object state from the given object message.
    * Provided object state should hold a valid data for current LiveObject, e.g. counter data for LiveCounter, map data for LiveMap.
    *
    * Object states are received during sync sequence, and sync sequence is a source of truth for the current state of the objects,
@@ -232,7 +234,7 @@ export abstract class LiveObject<
    *
    * @internal
    */
-  abstract overrideWithObjectState(objectState: ObjectState<ObjectData>): TUpdate | LiveObjectUpdateNoop;
+  abstract overrideWithObjectState(objectMessage: ObjectMessage): TUpdate | LiveObjectUpdateNoop;
   /**
    * @internal
    */

src/plugins/objects/objectmessage.ts

@@ -99,6 +99,8 @@ export interface ObjectsMapEntry<TData> {
    * and treat it as the "earliest possible" serial for comparison purposes.
    */
   timeserial?: string; // OME2b
+  /** A timestamp from the {@link timeserial} field. Only present if {@link tombstone} is `true` */
+  serialTimestamp?: number; // OME2d
   /** The data that represents the value of the map entry. */
   data?: TData; // OME2c
 }
@@ -344,6 +346,7 @@ function copyMsg(
     connectionId: msg.connectionId,
     timestamp: msg.timestamp,
     serial: msg.serial,
+    serialTimestamp: msg.serialTimestamp,
     siteCode: msg.siteCode,
   };
 
@@ -385,6 +388,8 @@ export class ObjectMessage {
   object?: ObjectState<ObjectData>; // OM2g
   /** An opaque string that uniquely identifies this object message. */
   serial?: string; // OM2h
+  /** A timestamp from the {@link serial} field. */
+  serialTimestamp?: number; // OM2j
   /** An opaque string used as a key to update the map of serial values on an object. */
   siteCode?: string; // OM2i
 
@@ -479,6 +484,8 @@ export class WireObjectMessage {
   object?: ObjectState<WireObjectData>; // OM2g
   /** An opaque string that uniquely identifies this object message. */
   serial?: string; // OM2h
+  /** A timestamp from the {@link serial} field. */
+  serialTimestamp?: number; // OM2j
   /** An opaque string used as a key to update the map of serial values on an object. */
   siteCode?: string; // OM2i
 

src/plugins/objects/objects.ts

@@ -393,7 +393,7 @@ export class Objects {
 
       // RTO5c1a
       if (existingObject) {
-        const update = existingObject.overrideWithObjectState(entry.objectState); // RTO5c1a1
+        const update = existingObject.overrideWithObjectState(entry.objectMessage); // RTO5c1a1
         // store updates to call subscription callbacks for all of them once the sync sequence is completed.
         // this will ensure that clients get notified about the changes only once everything has been applied.
         existingObjectUpdates.push({ object: existingObject, update });
@@ -406,11 +406,11 @@ export class Objects {
       const objectType = entry.objectType;
       switch (objectType) {
         case 'LiveCounter':
-          newObject = LiveCounter.fromObjectState(this, entry.objectState); // RTO5c1b1a
+          newObject = LiveCounter.fromObjectState(this, entry.objectMessage); // RTO5c1b1a
           break;
 
         case 'LiveMap':
-          newObject = LiveMap.fromObjectState(this, entry.objectState); // RTO5c1b1b
+          newObject = LiveMap.fromObjectState(this, entry.objectMessage); // RTO5c1b1b
           break;
 
         default:

src/plugins/objects/syncobjectsdatapool.ts

@@ -1,10 +1,10 @@
 import type BaseClient from 'common/lib/client/baseclient';
 import type RealtimeChannel from 'common/lib/client/realtimechannel';
-import { ObjectData, ObjectMessage, ObjectState } from './objectmessage';
+import { ObjectMessage } from './objectmessage';
 import { Objects } from './objects';
 
 export interface LiveObjectDataEntry {
-  objectState: ObjectState<ObjectData>;
+  objectMessage: ObjectMessage;
   objectType: 'LiveMap' | 'LiveCounter';
 }
 
@@ -64,9 +64,9 @@ export class SyncObjectsDataPool {
       const objectState = objectMessage.object;
 
       if (objectState.counter) {
-        this._pool.set(objectState.objectId, this._createLiveCounterDataEntry(objectState));
+        this._pool.set(objectState.objectId, this._createLiveCounterDataEntry(objectMessage));
       } else if (objectState.map) {
-        this._pool.set(objectState.objectId, this._createLiveMapDataEntry(objectState));
+        this._pool.set(objectState.objectId, this._createLiveMapDataEntry(objectMessage));
       } else {
         this._client.Logger.logAction(
           this._client.logger,
@@ -78,18 +78,18 @@ export class SyncObjectsDataPool {
     }
   }
 
-  private _createLiveCounterDataEntry(objectState: ObjectState<ObjectData>): LiveCounterDataEntry {
+  private _createLiveCounterDataEntry(objectMessage: ObjectMessage): LiveCounterDataEntry {
     const newEntry: LiveCounterDataEntry = {
-      objectState,
+      objectMessage,
       objectType: 'LiveCounter',
     };
 
     return newEntry;
   }
 
-  private _createLiveMapDataEntry(objectState: ObjectState<ObjectData>): LiveMapDataEntry {
+  private _createLiveMapDataEntry(objectMessage: ObjectMessage): LiveMapDataEntry {
     const newEntry: LiveMapDataEntry = {
-      objectState,
+      objectMessage,
       objectType: 'LiveMap',
     };