Make stale departure claims terminal

Carry the membership revision captured when a socket leaves through reconnect cleanup and reject a departure when that generation no longer matches. This prevents a losing old finalizer from removing a later rejoin.\n\nRefs #153

Author: coder13

docs/room-ownership.md

@@ -37,8 +37,8 @@ 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 retries from current
-state and cannot overwrite the new membership.
+concurrent rejoin advances the revision, so a losing leave is terminal and
+cannot overwrite the new membership.
 
 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

@@ -244,9 +244,13 @@ Room.methods.dropUser = async function (user, updateAdmin) {
   return this.save();
 };
 
-Room.methods.dropUserAtomically = async function (user) {
+Room.methods.dropUserAtomically = async function (
+  user,
+  expectedMembershipRevision = this.membershipRevision || 0,
+) {
   const userKey = user.id.toString();
-  if (!this.inRoom.get(userKey)) {
+  const membershipRevision = this.membershipRevision || 0;
+  if (!this.inRoom.get(userKey) || expectedMembershipRevision !== membershipRevision) {
     return null;
   }
 
@@ -260,14 +264,10 @@ Room.methods.dropUserAtomically = async function (user) {
     admin: this.admin,
   });
   const adminChanged = !sameUser(this.admin, nextAdmin);
-  const membershipRevision = this.membershipRevision || 0;
   const condition = {
     _id: this._id,
     [`inRoom.${userKey}`]: true,
   };
-  if (this.updatedAt) {
-    condition.updatedAt = this.updatedAt;
-  }
   if (membershipRevision > 0) {
     condition.membershipRevision = membershipRevision;
   } else {

server/models/room.test.js

@@ -275,7 +275,6 @@ describe('room security helpers', () => {
     const model = {
       findOneAndUpdate: jest.fn(() => query),
     };
-    const updatedAt = new Date('2026-07-13T04:30:00.000Z');
     const room = {
       _id: 'room-one',
       owner,
@@ -284,7 +283,6 @@ describe('room security helpers', () => {
       inRoom: new Map([['101', true], ['202', true]]),
       type: 'normal',
       membershipRevision: 7,
-      updatedAt,
       constructor: model,
     };
 
@@ -294,7 +292,6 @@ describe('room security helpers', () => {
       _id: room._id,
       'inRoom.101': true,
       membershipRevision: 7,
-      updatedAt,
     }, {
       $set: {
         'inRoom.101': false,

server/socket/namespaces/rooms.js

@@ -87,6 +87,7 @@ module.exports = (io, middlewares) => {
     return rooms.flatMap((room) => room.usersInRoom.map((user) => ({
       roomId: room._id,
       userId: user.id,
+      membershipRevision: room.membershipRevision,
       leaveReason: 'disconnect',
     })));
   }
@@ -125,32 +126,24 @@ module.exports = (io, middlewares) => {
   }
 
   async function finalizeUserDeparture({
-    roomId, userId, connectionId, leaveReason,
+    roomId, userId, connectionId, leaveReason, membershipRevision,
   }) {
     const userKey = userId.toString();
 
-    // The compare-and-set in dropUserAtomically is the cross-process claim.
-    // A losing process re-reads so it never overwrites a concurrent rejoin.
-    async function claimDeparture() {
-      const room = await fetchRoom(roomId);
-      if (!room || !room.inRoom.get(userKey)) {
-        return null;
-      }
-
-      if (leaveReason === 'disconnect'
-        && await hasActiveSocketsForUserRoom(userId, roomId)) {
-        return null;
-      }
-
-      const departure = await room.dropUserAtomically({ id: userId });
-      if (!departure) {
-        return claimDeparture();
-      }
+    const room = await fetchRoom(roomId);
+    if (!room || !room.inRoom.get(userKey)
+      || room.membershipRevision !== membershipRevision) {
+      return false;
+    }
 
-      return departure;
+    if (leaveReason === 'disconnect'
+      && await hasActiveSocketsForUserRoom(userId, roomId)) {
+      return false;
     }
 
-    const departure = await claimDeparture();
+    // 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);
     if (!departure) {
       return false;
     }
@@ -378,6 +371,7 @@ module.exports = (io, middlewares) => {
           roomId: socket.roomId,
           userId: socket.userId,
           connectionId: socket.id,
+          membershipRevision: socket.room.membershipRevision,
           leaveReason,
         });
       } catch (e) {
@@ -1077,6 +1071,7 @@ module.exports = (io, middlewares) => {
             roomId: socket.roomId,
             userId: socket.userId,
             connectionId: socket.id,
+            membershipRevision: socket.room.membershipRevision,
             leaveReason: 'disconnect',
           });
         } else if (socket.room) {

server/socket/namespaces/rooms.test.js

@@ -71,6 +71,7 @@ const makeRoom = (overrides = {}) => ({
   inRoom: new Map(),
   banned: new Map(),
   registered: new Map(),
+  membershipRevision: 7,
   requireRevealedIdentity: false,
   usersLength: 1,
   authenticate: jest.fn().mockResolvedValue(true),
@@ -378,14 +379,15 @@ describe('room namespace departures and moderation', () => {
       roomId: room._id,
       userId: 101,
       connectionId: 'host-socket',
+      membershipRevision: room.membershipRevision,
       leaveReason: 'explicit',
     });
 
     expect(connect.roomChannel.emit).not.toHaveBeenCalledWith(Protocol.UPDATE_ADMIN, null);
     expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101);
   });
 
-  it('emits metrics and leave events only for the process that claims a departure', async () => {
+  it('does not let a losing departure remove a later rejoin', async () => {
     const room = makeRoom({
       private: false,
       password: null,
@@ -394,41 +396,47 @@ describe('room namespace departures and moderation', () => {
     const updatedRoom = {
       ...room,
       inRoom: new Map([['101', false]]),
+      membershipRevision: 8,
       usersLength: 0,
       doneWithScramble: jest.fn().mockReturnValue(false),
     };
-    let claimed = false;
-    room.dropUserAtomically = jest.fn(async () => {
-      if (claimed) {
-        return null;
-      }
-      claimed = true;
-      return { room: updatedRoom, adminChanged: false };
+    room.dropUserAtomically = jest.fn().mockResolvedValue({
+      room: updatedRoom,
+      adminChanged: false,
     });
     const connect = setup(new Map([[room._id, room]]));
-    const persistedRoom = {
+    let persistedRoom = room;
+    Room.findById.mockImplementation(() => queryResult(persistedRoom));
+    const reconnectGrace = createReconnectGrace.mock.results[0].value;
+
+    await expect(reconnectGrace.finalizeDeparture({
+      roomId: room._id,
+      userId: 101,
+      connectionId: 'winner-socket',
+      membershipRevision: 7,
+      leaveReason: 'explicit',
+    })).resolves.toBe(true);
+
+    const rejoinedRoom = {
       ...room,
-      inRoom: new Map([['101', false]]),
+      admin: { id: 101 },
+      inRoom: new Map([['101', true]]),
+      membershipRevision: 9,
     };
-    Room.findById.mockImplementation(() => queryResult(claimed ? persistedRoom : room));
-    const reconnectGrace = createReconnectGrace.mock.results[0].value;
+    persistedRoom = rejoinedRoom;
 
-    await Promise.all([
-      reconnectGrace.finalizeDeparture({
-        roomId: room._id,
-        userId: 101,
-        connectionId: 'socket-on-process-a',
-        leaveReason: 'explicit',
-      }),
-      reconnectGrace.finalizeDeparture({
-        roomId: room._id,
-        userId: 101,
-        connectionId: 'socket-on-process-b',
-        leaveReason: 'explicit',
-      }),
-    ]);
+    await expect(reconnectGrace.finalizeDeparture({
+      roomId: room._id,
+      userId: 101,
+      connectionId: 'loser-socket',
+      membershipRevision: 7,
+      leaveReason: 'explicit',
+    })).resolves.toBe(false);
 
-    expect(room.dropUserAtomically).toHaveBeenCalledTimes(2);
+    expect(room.dropUserAtomically).toHaveBeenCalledTimes(1);
+    expect(rejoinedRoom.inRoom.get('101')).toBe(true);
+    expect(rejoinedRoom.admin).toEqual({ id: 101 });
+    expect(rejoinedRoom.membershipRevision).toBe(updatedRoom.membershipRevision + 1);
     expect(metrics.endRoomVisit).toHaveBeenCalledTimes(1);
     expect(connect.roomChannel.emit).toHaveBeenCalledTimes(1);
     expect(connect.roomChannel.emit).toHaveBeenCalledWith(Protocol.USER_LEFT, 101);
@@ -468,6 +476,7 @@ describe('room namespace departures and moderation', () => {
     expect(reconnectGrace.finalize).toHaveBeenCalledWith(expect.objectContaining({
       roomId: room._id,
       userId: 101,
+      membershipRevision: room.membershipRevision,
       leaveReason: 'explicit',
     }));
   });