Skip to content

Commit 06cf68f

Browse files
committed
fix(realtime): always roll back a failed table join; re-elect file-doc seeder on reclaim
Two review findings: - Table join: the catch skipped rollback when superseded, but a socket.join that landed before addUserToRoom threw leaves the socket in the Socket.IO room with no matching socket->room map entry — unreclaimable by any later op (cleanup keys off the map). Under serialization the skip is unnecessary (the newer op hasn't committed), so always roll back. Simpler + fixes the strand. - File-doc reclaim: fully evicting the prior socket didn't release the seeder role if it held it, so electSeederIfNeeded (which no-ops while seederSocketId is set) never re-elected and an unseeded doc stayed empty until the deadline. Clear the role on eviction so the join's election picks a new seeder. + 2 regression tests.
1 parent 43cfd6f commit 06cf68f

4 files changed

Lines changed: 45 additions & 7 deletions

File tree

apps/realtime/src/handlers/file-doc.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,22 @@ describe('setupWorkspaceFileDocHandlers', () => {
612612
expect(sent.some((m) => m.event === FILE_DOC_EVENTS.MESSAGE)).toBe(false)
613613
})
614614

615+
it('re-elects a seeder when the reclaimed socket held the seeder role', async () => {
616+
const { io, sent } = createIo()
617+
const a = setup('socket-a', io)
618+
await a.handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 7 })
619+
// a is the sole owner of an unseeded doc → elected seeder.
620+
expect(sent.find((m) => m.event === FILE_DOC_EVENTS.SEED_REQUEST)?.target).toBe('socket-a')
621+
sent.length = 0
622+
623+
const b = setup('socket-b', io) // same user-1 reconnecting, reusing client id 7
624+
await b.handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 7 })
625+
626+
// The reclaim evicts a (the seeder) and releases the role, so the join's election picks b —
627+
// the doc gets seeded instead of waiting out the deadline.
628+
expect(sent.find((m) => m.event === FILE_DOC_EVENTS.SEED_REQUEST)?.target).toBe('socket-b')
629+
})
630+
615631
it('does not drop the current document when a switch is rejected for a foreign client id', async () => {
616632
const { io } = createIo()
617633
const a = setup('socket-a', io) // user-1

apps/realtime/src/handlers/file-doc.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,10 @@ export function setupWorkspaceFileDocHandlers(
481481
awarenessProtocol.removeAwarenessStates(entry.awareness, [owner.clientId], null)
482482
socketToRoomName.delete(otherSid)
483483
io.in(otherSid).socketsLeave(name)
484+
// If the evicted socket held the seeder role, release it so the election at the end of
485+
// this join re-elects (electSeederIfNeeded no-ops while seederSocketId is set) — otherwise
486+
// an unseeded document would stay empty until the seed deadline expires.
487+
if (entry.seederSocketId === otherSid) entry.seederSocketId = null
484488
}
485489

486490
// Only now that the rebind is guaranteed to succeed, leave a previously-joined document if

apps/realtime/src/handlers/tables.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,4 +297,24 @@ describe('setupTablesHandlers', () => {
297297
expect(roomManager.removeUserFromRoom).toHaveBeenCalledWith(TABLE_ROOM, 'socket-1')
298298
expect(roomManager.broadcastPresenceUpdate).toHaveBeenCalledWith(TABLE_ROOM, 'socket-1')
299299
})
300+
301+
it('rolls back the Socket.IO membership when a join fails mid-commit', async () => {
302+
const { socket, handlers } = createSocket()
303+
const roomManager = createRoomManager({
304+
// socket.join lands first, then the presence write throws — the socket is now in the
305+
// Socket.IO room with no matching socket→room map entry, unreclaimable by any later op.
306+
addUserToRoom: vi.fn().mockRejectedValue(new Error('redis down')),
307+
})
308+
setupTablesHandlers(socket as unknown as SetupArg, roomManager)
309+
310+
await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-1' })
311+
312+
// The catch must always roll back the partial membership, not skip it.
313+
expect(socket.leave).toHaveBeenCalledWith('table:table-1')
314+
expect(roomManager.removeUserFromRoom).toHaveBeenCalledWith(TABLE_ROOM, 'socket-1')
315+
expect(socket.emit).toHaveBeenCalledWith(
316+
TABLE_PRESENCE_EVENTS.JOIN_ERROR,
317+
expect.objectContaining({ code: 'JOIN_FAILED' })
318+
)
319+
})
300320
})

apps/realtime/src/handlers/tables.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -223,13 +223,11 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR
223223
logger.info(`User ${userId} (${userName}) joined table room ${tableId}`)
224224
} catch (error) {
225225
logger.error('Error joining table room:', error)
226-
// If a newer JOIN/LEAVE superseded this one while it ran, skip the rollback + error: the
227-
// next serialized op cleans up any partial registration via its leave-prior, and the client
228-
// already moved off this table so the error is moot. A disconnect (not a supersession) still
229-
// falls through and rolls back — hence the generation-only check, not the full `superseded`.
230-
if (joinGeneration !== joinAttempt) return
231-
// Roll back any partial join so a failed attempt can't leave the socket in the
232-
// Socket.IO room or a stale presence entry behind, before signalling a retry.
226+
// Always roll back a partial join: cleanup keys off the socket→room map, so a `socket.join`
227+
// that landed without a matching `addUserToRoom` (a throw in between) would otherwise leave
228+
// the socket stranded in the Socket.IO room, unreclaimable by any later op. Safe to run even
229+
// when superseded — serialization means the newer op hasn't committed yet, so this touches
230+
// only this join's own (this-table) state, never the newer op's room.
233231
try {
234232
const room = tableRoom(tableId)
235233
socket.leave(roomName(room))

0 commit comments

Comments
 (0)