Fence duplicate room joins from stale leaves

Advance a durable per-user presence revision before acknowledging an already-active room membership. Departure claims now require that generation, so an old leave cannot remove a newly opened tab.\n\nRefs #153

Author: coder13

docs/room-ownership.md

@@ -34,11 +34,12 @@ itself from the per-user socket group before the remaining-tab check, so two
 simultaneous explicit leaves cannot strand membership. If the owner later
 rejoins, they reclaim admin before the join is acknowledged.
 
-The final persisted departure is a MongoDB compare-and-set keyed by the active
-membership revision. This applies across Socket.IO processes: exactly one
-process can claim a leave, emit its room events, and record its metrics. A
-concurrent rejoin advances the revision, so a losing leave is terminal and
-cannot overwrite the new membership.
+The final persisted departure is a MongoDB compare-and-set keyed by room
+membership and the active user's presence revision. This applies across
+Socket.IO processes: exactly one process can claim a leave, emit its room
+events, and record its metrics. Every authenticated duplicate join advances
+that user's durable presence revision before acknowledgment, so an older leave
+is terminal and cannot overwrite an active new tab.
 
 An empty room persists `admin: null`, but the established `UPDATE_ADMIN`
 Socket.IO event is emitted only for a non-null active admin. Spectators and

server/models/room.js

@@ -23,6 +23,17 @@ const selectRoomAdmin = ({ usersInRoom, owner, admin }) => {
   return usersInRoom.find((user) => sameUser(user, admin)) || usersInRoom[0];
 };
 
+const presenceRevisionFor = (room, userKey) => (
+  room.presenceRevision ? room.presenceRevision.get(userKey) || 0 : 0
+);
+
+const advancePresenceRevisionFor = (room, userKey) => {
+  if (!room.presenceRevision) {
+    room.presenceRevision = new Map();
+  }
+  room.presenceRevision.set(userKey, presenceRevisionFor(room, userKey) + 1);
+};
+
 // const lengths = {
 
 // };
@@ -109,6 +120,11 @@ const Room = new mongoose.Schema({
     type: Number,
     default: 0,
   },
+  presenceRevision: {
+    type: Map,
+    of: Number,
+    default: {},
+  },
   admin: { type: mongoose.Schema.Types.ObjectId, ref: 'User' },
   owner: { type: mongoose.Schema.Types.ObjectId, ref: 'User' },
   type: {
@@ -210,6 +226,7 @@ Room.methods.addUser = async function (user, spectating, updateAdmin) {
 
   this.inRoom.set(user.id.toString(), true);
   this.membershipRevision = (this.membershipRevision || 0) + 1;
+  advancePresenceRevisionFor(this, user.id.toString());
 
   if (this.waitingForCount === 0) {
     this.waitingFor.set(user.id.toString(), true);
@@ -234,6 +251,7 @@ Room.methods.dropUser = async function (user, updateAdmin) {
   this.inRoom.set(user.id.toString(), false);
   this.waitingFor.set(user.id.toString(), false);
   this.membershipRevision = (this.membershipRevision || 0) + 1;
+  advancePresenceRevisionFor(this, user.id.toString());
 
   await this.updateAdminIfNeeded(updateAdmin);
 
@@ -247,10 +265,14 @@ Room.methods.dropUser = async function (user, updateAdmin) {
 Room.methods.dropUserAtomically = async function (
   user,
   expectedMembershipRevision = this.membershipRevision || 0,
+  expectedPresenceRevision = presenceRevisionFor(this, user.id.toString()),
 ) {
   const userKey = user.id.toString();
   const membershipRevision = this.membershipRevision || 0;
-  if (!this.inRoom.get(userKey) || expectedMembershipRevision !== membershipRevision) {
+  const presenceRevision = presenceRevisionFor(this, userKey);
+  if (!this.inRoom.get(userKey)
+    || expectedMembershipRevision !== membershipRevision
+    || expectedPresenceRevision !== presenceRevision) {
     return null;
   }
 
@@ -268,13 +290,29 @@ Room.methods.dropUserAtomically = async function (
     _id: this._id,
     [`inRoom.${userKey}`]: true,
   };
+  const revisionConditions = [];
   if (membershipRevision > 0) {
     condition.membershipRevision = membershipRevision;
   } else {
-    condition.$or = [
-      { membershipRevision: 0 },
-      { membershipRevision: { $exists: false } },
-    ];
+    revisionConditions.push({
+      $or: [
+        { membershipRevision: 0 },
+        { membershipRevision: { $exists: false } },
+      ],
+    });
+  }
+  if (presenceRevision > 0) {
+    condition[`presenceRevision.${userKey}`] = presenceRevision;
+  } else {
+    revisionConditions.push({
+      $or: [
+        { [`presenceRevision.${userKey}`]: 0 },
+        { [`presenceRevision.${userKey}`]: { $exists: false } },
+      ],
+    });
+  }
+  if (revisionConditions.length > 0) {
+    condition.$and = revisionConditions;
   }
 
   const update = {
@@ -285,6 +323,7 @@ Room.methods.dropUserAtomically = async function (
     },
     $inc: {
       membershipRevision: 1,
+      [`presenceRevision.${userKey}`]: 1,
     },
   };
   if (usersInRoom.length === 0 && this.type === 'normal') {
@@ -308,6 +347,20 @@ Room.methods.dropUserAtomically = async function (
   return { room: updatedRoom, adminChanged };
 };
 
+Room.methods.advancePresenceRevision = function (userId) {
+  const userKey = userId.toString();
+  return this.constructor.findOneAndUpdate({
+    _id: this._id,
+    [`inRoom.${userKey}`]: true,
+  }, {
+    $inc: {
+      [`presenceRevision.${userKey}`]: 1,
+    },
+  }, {
+    new: true,
+  }).populate('users').populate('admin').populate('owner');
+};
+
 Room.methods.banUser = async function (userId) {
   this.banned.set(userId.toString(), true);
   return this.dropUser({ id: userId });

server/models/room.test.js

@@ -283,6 +283,7 @@ describe('room security helpers', () => {
       inRoom: new Map([['101', true], ['202', true]]),
       type: 'normal',
       membershipRevision: 7,
+      presenceRevision: new Map([['101', 7]]),
       constructor: model,
     };
 
@@ -292,13 +293,17 @@ describe('room security helpers', () => {
       _id: room._id,
       'inRoom.101': true,
       membershipRevision: 7,
+      'presenceRevision.101': 7,
     }, {
       $set: {
         'inRoom.101': false,
         'waitingFor.101': false,
         admin: nextAdmin._id,
       },
-      $inc: { membershipRevision: 1 },
+      $inc: {
+        membershipRevision: 1,
+        'presenceRevision.101': 1,
+      },
     }, { new: true });
     expect(departure).toEqual({ room: updatedRoom, adminChanged: true });
     expect(mirrorRoomChanges).toHaveBeenCalledWith(updatedRoom, {
@@ -309,6 +314,30 @@ describe('room security helpers', () => {
     });
   });
 
+  it('advances an active user presence revision atomically', async () => {
+    const updatedRoom = { id: 'room-one' };
+    const query = {
+      populate: jest.fn(() => query),
+      then: (resolve, reject) => Promise.resolve(updatedRoom).then(resolve, reject),
+    };
+    const model = {
+      findOneAndUpdate: jest.fn(() => query),
+    };
+    const room = {
+      _id: 'room-one',
+      constructor: model,
+    };
+
+    await expect(Room.methods.advancePresenceRevision.call(room, 101)).resolves.toBe(updatedRoom);
+
+    expect(model.findOneAndUpdate).toHaveBeenCalledWith({
+      _id: room._id,
+      'inRoom.101': true,
+    }, {
+      $inc: { 'presenceRevision.101': 1 },
+    }, { new: true });
+  });
+
   it('persists and announces the owner reclaiming admin controls', async () => {
     const owner = { id: 101 };
     const room = {

server/socket/namespaces/rooms.js

@@ -88,6 +88,7 @@ module.exports = (io, middlewares) => {
       roomId: room._id,
       userId: user.id,
       membershipRevision: room.membershipRevision,
+      presenceRevision: room.presenceRevision.get(user.id.toString()),
       leaveReason: 'disconnect',
     })));
   }
@@ -126,13 +127,14 @@ module.exports = (io, middlewares) => {
   }
 
   async function finalizeUserDeparture({
-    roomId, userId, connectionId, leaveReason, membershipRevision,
+    roomId, userId, connectionId, leaveReason, membershipRevision, presenceRevision,
   }) {
     const userKey = userId.toString();
 
     const room = await fetchRoom(roomId);
     if (!room || !room.inRoom.get(userKey)
-      || room.membershipRevision !== membershipRevision) {
+      || room.membershipRevision !== membershipRevision
+      || room.presenceRevision.get(userKey) !== presenceRevision) {
       return false;
     }
 
@@ -143,7 +145,11 @@ module.exports = (io, middlewares) => {
 
     // The membership revision is the departure generation. A failed claim is
     // terminal: retrying it after a rejoin would remove the new membership.
-    const departure = await room.dropUserAtomically({ id: userId }, membershipRevision);
+    const departure = await room.dropUserAtomically(
+      { id: userId },
+      membershipRevision,
+      presenceRevision,
+    );
     if (!departure) {
       return false;
     }
@@ -372,6 +378,7 @@ module.exports = (io, middlewares) => {
           userId: socket.userId,
           connectionId: socket.id,
           membershipRevision: socket.room.membershipRevision,
+          presenceRevision: socket.room.presenceRevision.get(socket.userId.toString()),
           leaveReason,
         });
       } catch (e) {
@@ -423,18 +430,42 @@ module.exports = (io, middlewares) => {
         });
       }
 
-      const r = await activeRoom.addUser(socket.user, spectating, sendAdminUpdate);
+      const presentRoom = await activeRoom.advancePresenceRevision(socket.userId);
+      if (presentRoom) {
+        socket.room = presentRoom;
+        await metrics.beginRoomVisit({
+          room: presentRoom,
+          userId: socket.userId,
+          connectionId: socket.id,
+          activeUserCount: presentRoom.usersLength,
+        });
+        return cb(null, joinRoomMask(presentRoom));
+      }
 
+      const currentRoom = await fetchRoom(room._id);
+      if (!currentRoom) {
+        return cb({
+          statusCode: 404,
+          message: 'Room no longer exists',
+        });
+      }
+      const r = await currentRoom.addUser(socket.user, spectating, sendAdminUpdate);
       if (!r) {
-        // The user is already present in persisted room state.
-        socket.room = activeRoom;
+        const fencedRoom = await currentRoom.advancePresenceRevision(socket.userId);
+        if (!fencedRoom) {
+          return cb({
+            statusCode: 409,
+            message: 'Room membership changed while joining',
+          });
+        }
+        socket.room = fencedRoom;
         await metrics.beginRoomVisit({
-          room: activeRoom,
+          room: fencedRoom,
           userId: socket.userId,
           connectionId: socket.id,
-          activeUserCount: activeRoom.usersLength,
+          activeUserCount: fencedRoom.usersLength,
         });
-        return cb(null, joinRoomMask(activeRoom));
+        return cb(null, joinRoomMask(fencedRoom));
       }
 
       socket.room = r;
@@ -1072,6 +1103,7 @@ module.exports = (io, middlewares) => {
             userId: socket.userId,
             connectionId: socket.id,
             membershipRevision: socket.room.membershipRevision,
+            presenceRevision: socket.room.presenceRevision.get(socket.userId.toString()),
             leaveReason: 'disconnect',
           });
         } else if (socket.room) {