Ensure we do not clobber a newer RR with an older unthreaded one (#3617)

* Ensure we do not clobber a newer RR with an older unthreaded one

or vice versa

* Fix test

spec/unit/read-receipt.spec.ts

@@ -16,7 +16,7 @@ limitations under the License.
 
 import MockHttpBackend from "matrix-mock-request";
 
-import { MAIN_ROOM_TIMELINE, ReceiptType } from "../../src/@types/read_receipts";
+import { MAIN_ROOM_TIMELINE, ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts";
 import { MatrixClient } from "../../src/client";
 import { EventType, MatrixEvent, Room } from "../../src/matrix";
 import { synthesizeReceipt } from "../../src/models/read-receipt";
@@ -220,4 +220,42 @@ describe("Read receipt", () => {
             expect(content.thread_id).toEqual(destinationId);
         });
     });
+
+    describe("addReceiptToStructure", () => {
+        it("should not allow an older unthreaded receipt to clobber a `main` threaded one", () => {
+            const userId = client.getSafeUserId();
+            const room = new Room(ROOM_ID, client, userId);
+
+            const unthreadedReceipt: WrappedReceipt = {
+                eventId: "$olderEvent",
+                data: {
+                    ts: 1234567880,
+                },
+            };
+            const mainTimelineReceipt: WrappedReceipt = {
+                eventId: "$newerEvent",
+                data: {
+                    ts: 1234567890,
+                },
+            };
+
+            room.addReceiptToStructure(
+                mainTimelineReceipt.eventId,
+                ReceiptType.ReadPrivate,
+                userId,
+                mainTimelineReceipt.data,
+                false,
+            );
+            expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId);
+
+            room.addReceiptToStructure(
+                unthreadedReceipt.eventId,
+                ReceiptType.ReadPrivate,
+                userId,
+                unthreadedReceipt.data,
+                false,
+            );
+            expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId);
+        });
+    });
 });

spec/unit/room.spec.ts

@@ -3112,10 +3112,10 @@ describe("Room", function () {
                 it("should give precedence to m.read.private", () => {
                     room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => {
                         if (receiptType === ReceiptType.ReadPrivate) {
-                            return { eventId: "eventId1" } as WrappedReceipt;
+                            return { eventId: "eventId1", data: { ts: 123 } };
                         }
                         if (receiptType === ReceiptType.Read) {
-                            return { eventId: "eventId2" } as WrappedReceipt;
+                            return { eventId: "eventId2", data: { ts: 123 } };
                         }
                         return null;
                     };

src/models/read-receipt.ts

@@ -57,9 +57,10 @@ export abstract class ReadReceipt<
     // which pass in an event ID and get back some receipts, so we also store
     // a pre-cached list for this purpose.
     // Map: receipt type → user Id → receipt
-    private receipts = new MapWithDefault<string, Map<string, [WrappedReceipt | null, WrappedReceipt | null]>>(
-        () => new Map(),
-    );
+    private receipts = new MapWithDefault<
+        string,
+        Map<string, [realReceipt: WrappedReceipt | null, syntheticReceipt: WrappedReceipt | null]>
+    >(() => new Map());
     private receiptCacheByEventId: ReceiptCache = new Map();
 
     public abstract getUnfilteredTimelineSet(): EventTimelineSet;
@@ -85,6 +86,13 @@ export abstract class ReadReceipt<
         return syntheticReceipt ?? realReceipt;
     }
 
+    private compareReceipts(a: WrappedReceipt, b: WrappedReceipt): number {
+        // Try compare them in our unfiltered timeline set order, falling back to receipt timestamp which should be
+        // relatively sane as receipts are set only by the originating homeserver so as long as its clock doesn't
+        // jump around then it should be valid.
+        return this.getUnfilteredTimelineSet().compareEventOrdering(a.eventId, b.eventId) ?? a.data.ts - b.data.ts;
+    }
+
     /**
      * Get the ID of the event that a given user has read up to, or null if we
      * have received no read receipts from them.
@@ -99,19 +107,13 @@ export abstract class ReadReceipt<
         // receipt type here again. IMHO this should be done by the server in
         // some more intelligent manner or the client should just use timestamps
 
-        const timelineSet = this.getUnfilteredTimelineSet();
         const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read);
         const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate);
 
         // If we have both, compare them
         let comparison: number | null | undefined;
         if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) {
-            comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId);
-        }
-
-        // If we didn't get a comparison try to compare the ts of the receipts
-        if (!comparison && publicReadReceipt?.data?.ts && privateReadReceipt?.data?.ts) {
-            comparison = publicReadReceipt?.data?.ts - privateReadReceipt?.data?.ts;
+            comparison = this.compareReceipts(publicReadReceipt, privateReadReceipt);
         }
 
         // The public receipt is more likely to drift out of date so the private
@@ -142,20 +144,20 @@ export abstract class ReadReceipt<
             existingReceipt = pair[ReceiptPairSyntheticIndex] ?? pair[ReceiptPairRealIndex];
         }
 
+        const wrappedReceipt: WrappedReceipt = {
+            eventId,
+            data: receipt,
+        };
+
         if (existingReceipt) {
-            // we only want to add this receipt if we think it is later than the one we already have.
+            // We only want to add this receipt if we think it is later than the one we already have.
             // This is managed server-side, but because we synthesize RRs locally we have to do it here too.
-            const ordering = this.getUnfilteredTimelineSet().compareEventOrdering(existingReceipt.eventId, eventId);
-            if (ordering !== null && ordering >= 0) {
+            const ordering = this.compareReceipts(existingReceipt, wrappedReceipt);
+            if (ordering >= 0) {
                 return;
             }
         }
 
-        const wrappedReceipt: WrappedReceipt = {
-            eventId,
-            data: receipt,
-        };
-
         const realReceipt = synthetic ? pair[ReceiptPairRealIndex] : wrappedReceipt;
         const syntheticReceipt = synthetic ? wrappedReceipt : pair[ReceiptPairSyntheticIndex];