fix(realtime): treat unconfirmed removals as failures and refresh role cache on fresh verify

Treat any null removal result as a failed cleanup (the sweep always passes
the target room, so null only means failure — including with expired
mapping keys), move the same-room rejoin guard to a synchronous check
immediately before the removal, and record verifyWorkflowAccess's fresh
decision into the role cache so a re-granted user is not blocked by a
stale cached revocation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: icecrasher321

apps/realtime/src/access-revalidation.test.ts

@@ -30,7 +30,16 @@ interface FakeSocket {
 function makeSocket(id: string, userId: string | undefined, workflowId?: string): FakeSocket {
   const rooms = new Set<string>([id])
   if (workflowId) rooms.add(workflowId)
-  return { id, userId, rooms, emit: vi.fn(), leave: vi.fn() }
+  return {
+    id,
+    userId,
+    rooms,
+    emit: vi.fn(),
+    // Socket.IO's leave removes the room from `rooms` synchronously.
+    leave: vi.fn((room: string) => {
+      rooms.delete(room)
+    }),
+  }
 }
 
 function makeManager(sockets: FakeSocket[], presence: Partial<UserPresence>[] = []) {
@@ -179,9 +188,29 @@ describe('access-revalidation sweep', () => {
     expect(socket.leave).toHaveBeenCalledWith('wf-1')
     expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
 
-    // The evicted socket is out of the room now, so membership scans no longer
-    // see it — the retry queue must drive the cleanup to completion.
-    socket.rooms = new Set(['sock-1'])
+    // The evicted socket left the room, so membership scans no longer see it —
+    // the retry queue must drive the cleanup to completion.
+    await sweep.runOnce()
+    sweep.stop()
+
+    expect(manager.removeUserFromRoom).toHaveBeenCalledTimes(2)
+    expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith('wf-1')
+  })
+
+  it('defers cleanup when removal fails with expired socket mappings', async () => {
+    const socket = makeSocket('sock-1', 'user-1', 'wf-1')
+    const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
+    // Mapping keys already expired (lookup resolves null) AND the removal fails
+    // (the Redis manager swallows the transport error into null) — the failed
+    // removal must still defer instead of reading as success.
+    manager.removeUserFromRoom.mockResolvedValueOnce(null)
+    mockResolveRole.mockResolvedValue(null)
+
+    const sweep = startAccessRevalidationSweep(manager)
+    await sweep.runOnce()
+
+    expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
+
     await sweep.runOnce()
     sweep.stop()
 
@@ -204,8 +233,7 @@ describe('access-revalidation sweep', () => {
     expect(socket.leave).toHaveBeenCalledWith('wf-1')
     expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
 
-    // Next pass: the socket left the room, and the removal now succeeds.
-    socket.rooms = new Set(['sock-1'])
+    // Next pass: the removal now succeeds and the cleanup completes.
     await sweep.runOnce()
     sweep.stop()
 
@@ -240,8 +268,9 @@ describe('access-revalidation sweep', () => {
     await sweep.runOnce()
     expect(socket.leave).toHaveBeenCalledWith('wf-1')
 
-    // Access restored and the socket re-joined (still in the room in this
-    // fake): the retry must NOT remove the fresh presence entry.
+    // Access restored and the socket re-joined the same room: the retry must
+    // NOT remove the fresh presence entry that re-join created.
+    socket.rooms.add('wf-1')
     mockResolveRole.mockResolvedValue('read')
     await sweep.runOnce()
     sweep.stop()

apps/realtime/src/access-revalidation.ts

@@ -95,10 +95,20 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR
         return
       }
 
+      // Synchronous re-join guard with no awaits before the removal: if the
+      // socket legitimately re-joined this room after the eviction (access
+      // restored), that join re-added its presence — removal would erase it.
+      if (io.sockets.sockets.get(socketId)?.rooms.has(workflowId)) {
+        pendingCleanups.delete(key)
+        return
+      }
+
       const removed = await roomManager.removeUserFromRoom(socketId, workflowId)
-      if (removed === null && currentWorkflowId !== null) {
-        // The Redis manager swallows transport errors into null — a live
-        // mapping with no reported removal means the removal did not happen.
+      if (removed === null) {
+        // The sweep always passes the target room, and both managers report a
+        // performed removal by returning it — the Redis manager swallows
+        // transport errors into null, so null means the removal did not happen
+        // (even when the socket's mapping keys have already expired).
         throw new Error('room-state removal not confirmed')
       }
 
@@ -114,15 +124,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR
   }
 
   async function retryPendingCleanups(): Promise<void> {
-    for (const [key, { socketId, workflowId }] of pendingCleanups) {
-      const liveSocket = io.sockets.sockets.get(socketId)
-      if (liveSocket?.rooms.has(workflowId)) {
-        // The socket re-joined legitimately after the eviction (access was
-        // restored); that join re-added its presence entry, so there is
-        // nothing stale left to clean and removal would erase live presence.
-        pendingCleanups.delete(key)
-        continue
-      }
+    for (const [, { socketId, workflowId }] of pendingCleanups) {
       await cleanupEvictedSocket(socketId, workflowId)
     }
   }

apps/realtime/src/middleware/permissions.test.ts

@@ -23,10 +23,23 @@ vi.mock('@sim/platform-authz/workflow', () => ({
   authorizeWorkflowByWorkspacePermission: mockAuthorize,
 }))
 
+vi.mock('@sim/db', () => ({
+  db: {
+    select: vi.fn(() => ({
+      from: vi.fn(() => ({
+        where: vi.fn(() => ({
+          limit: vi.fn(async () => [{ workspaceId: 'ws-1', name: 'Test Workflow' }]),
+        })),
+      })),
+    })),
+  },
+}))
+
 import {
   checkRolePermission,
   checkWorkflowOperationPermission,
   resolveCurrentWorkflowRole,
+  verifyWorkflowAccess,
 } from '@/middleware/permissions'
 
 describe('checkRolePermission', () => {
@@ -481,3 +494,28 @@ describe('resolveCurrentWorkflowRole single-flight', () => {
     }
   })
 })
+
+describe('verifyWorkflowAccess role-cache refresh', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('records the fresh decision so a stale cached revocation does not block a re-granted join', async () => {
+    const userId = 'vw-user-1'
+    const workflowId = 'vw-wf-1'
+
+    // A sweep-style resolution records the revocation.
+    mockAuthorize.mockResolvedValue({ allowed: false, workspacePermission: null })
+    expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBeNull()
+
+    // Access is restored and a fresh join-time verify succeeds.
+    mockAuthorize.mockResolvedValue({ allowed: true, workspacePermission: 'write' })
+    const access = await verifyWorkflowAccess(userId, workflowId)
+    expect(access.hasAccess).toBe(true)
+
+    // The pre-join gate's warm read now sees the fresh role, not the stale
+    // cached null recorded before the re-grant.
+    mockAuthorize.mockRejectedValue(new Error('must not re-query'))
+    expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBe('write')
+  })
+})

apps/realtime/src/middleware/permissions.ts

@@ -129,6 +129,21 @@ function purgeExpiredRoles(now: number): void {
   }
 }
 
+/**
+ * Records a freshly-read authoritative decision into the role cache. Every
+ * successful DB read of a user's workspace role goes through this — including
+ * the join-time {@link verifyWorkflowAccess} — so a stale cached revocation
+ * never outlives a newer authoritative read (e.g. a re-granted user re-joining
+ * within the TTL of the sweep's recorded `null`).
+ */
+function recordRoleDecision(key: string, role: string | null): void {
+  const now = Date.now()
+  if (roleCache.size >= MAX_ROLE_CACHE_ENTRIES) {
+    purgeExpiredRoles(now)
+  }
+  roleCache.set(key, { role, expiresAt: now + ROLE_REVALIDATION_TTL_MS })
+}
+
 async function resolveRoleUncached(
   key: string,
   userId: string,
@@ -142,11 +157,7 @@ async function resolveRoleUncached(
       action: 'read',
     })
     const role = authorization.allowed ? (authorization.workspacePermission ?? null) : null
-    const now = Date.now()
-    if (roleCache.size >= MAX_ROLE_CACHE_ENTRIES) {
-      purgeExpiredRoles(now)
-    }
-    roleCache.set(key, { role, expiresAt: now + ROLE_REVALIDATION_TTL_MS })
+    recordRoleDecision(key, role)
     return role
   } catch (error) {
     logger.warn(
@@ -226,6 +237,11 @@ export async function checkWorkflowOperationPermission(
  * Returns `hasAccess: false` only for genuine denials (workflow missing/archived
  * or no workspace permission). Transient failures (DB errors) are rethrown so the
  * caller can report them as retryable instead of a permanent access denial.
+ *
+ * The fresh authorization decision is recorded into the role cache, so the
+ * pre-join re-check and the eviction sweep see it immediately — in particular,
+ * a user whose access was revoked and then restored is not blocked by the
+ * sweep's stale cached revocation for the remainder of its TTL.
  */
 export async function verifyWorkflowAccess(
   userId: string,
@@ -253,6 +269,11 @@ export async function verifyWorkflowAccess(
       action: 'read',
     })
 
+    recordRoleDecision(
+      `${userId}:${workflowId}`,
+      authorization.allowed ? (authorization.workspacePermission ?? null) : null
+    )
+
     if (!authorization.allowed || !authorization.workspacePermission) {
       logger.warn(
         `User ${userId} is not permitted to access workflow ${workflowId}: ${authorization.message}`