refactor(ws): single-snapshot move audience to close the restricted-move race (#93)

Implements Option 2 of #93. The restricted branch of broadcastPageMoved
previously resolved its audience twice — emitToAuthorizedUsers and
emitDeleteToUnauthorized each ran an independent fetchSockets +
getUserIdsWithPageAccess — leaving a race window between the two snapshots
where a socket could receive both the move and the delete (leak) or neither
(lost compensating delete).

- ws.service.ts: add emitMoveWithRestrictionSplit() that takes ONE socket
  snapshot and ONE authorization resolution, then partitions the room:
  authorized users get the moveTreeNode, everyone else (unauthorized +
  anonymous) get the compensating deleteTreeNode. Disjoint + complete by
  construction. Remove the now-unused emitToAuthorizedUsers /
  emitDeleteToUnauthorized; keep private broadcastToAuthorizedUsers (still
  used by emitRestrictedAwareToSpace).
- ws-tree.service.ts: broadcastPageMoved restricted branch now drives move +
  delete from the single method.
- specs: assert the single method is used and that fetchSockets /
  getUserIdsWithPageAccess are each called exactly once (single snapshot);
  re-route ws-service.spec to emitTreeEvent after the method removal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

apps/server/src/ws/ws-service.spec.ts

@@ -16,9 +16,9 @@ import {
  *    fan-out per user, sockets with no userId skipped).
  *
  * Both private methods are exercised through their public entry points:
- * spaceHasRestrictions via emitTreeEvent, broadcastToAuthorizedUsers via
+ * spaceHasRestrictions via emitTreeEvent, broadcastToAuthorizedUsers via the
- * emitToAuthorizedUsers. WsService is constructed with mocked cache + repo and a
+ * restricted-page path of emitTreeEvent. WsService is constructed with mocked
- * mocked socket.io server, so no live infra is needed.
+ * cache + repo and a mocked socket.io server, so no live infra is needed.
  */
 
 describe('WsService.spaceHasRestrictions (cache lifecycle, via emitTreeEvent)', () => {
@@ -127,7 +127,7 @@ describe('WsService.spaceHasRestrictions (cache lifecycle, via emitTreeEvent)',
   });
 });
 
-describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUsers)', () => {
+describe('WsService.broadcastToAuthorizedUsers fan-out (via emitTreeEvent restricted path)', () => {
   let service: WsService;
   let pagePermissionRepo: {
     hasRestrictedPagesInSpace: jest.Mock;
@@ -167,6 +167,12 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser
       in: serverIn,
     };
     service.setServer(server as never);
+
+    // Reach broadcastToAuthorizedUsers through emitTreeEvent's restricted path:
+    // the space has restrictions (cache miss -> repo says true) and the page has
+    // a restricted ancestor, so the emit is scoped to the authorized users.
+    pagePermissionRepo.hasRestrictedPagesInSpace.mockResolvedValue(true);
+    pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true);
   });
 
   it('only sockets whose userId is in getUserIdsWithPageAccess receive the event', async () => {
@@ -180,7 +186,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser
     ]);
 
     const data = { operation: 'moveTreeNode' };
-    await service.emitToAuthorizedUsers('space-1', 'page-1', data);
+    await service.emitTreeEvent('space-1', 'page-1', data);
 
     // The authorized set is resolved from the candidate userIds present on the
     // sockets (deduped), then only those users' sockets get the event.
@@ -203,7 +209,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser
     ]);
 
     const data = { operation: 'moveTreeNode' };
-    await service.emitToAuthorizedUsers('space-1', 'page-1', data);
+    await service.emitTreeEvent('space-1', 'page-1', data);
 
     // Both of the authorized user's sockets (e.g. two browser tabs) receive it.
     expect(tab1).toHaveBeenCalledWith('message', data);
@@ -227,7 +233,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser
     ]);
 
     const data = { operation: 'moveTreeNode' };
-    await service.emitToAuthorizedUsers('space-1', 'page-1', data);
+    await service.emitTreeEvent('space-1', 'page-1', data);
 
     expect(okEmit).toHaveBeenCalledWith('message', data);
     expect(anonEmit).not.toHaveBeenCalled();
@@ -241,7 +247,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser
   it('no sockets in the room -> no repo lookup, no emit', async () => {
     fetchSockets.mockResolvedValue([]);
 
-    await service.emitToAuthorizedUsers('space-1', 'page-1', { op: 'x' });
+    await service.emitTreeEvent('space-1', 'page-1', { op: 'x' });
 
     expect(pagePermissionRepo.getUserIdsWithPageAccess).not.toHaveBeenCalled();
   });
@@ -252,7 +258,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser
       { id: 's1', data: { userId: 'u' }, emit: jest.fn() },
     ]);
 
-    await service.emitToAuthorizedUsers('space-7', 'page-1', { op: 'x' });
+    await service.emitTreeEvent('space-7', 'page-1', { op: 'x' });
 
     expect(serverIn).toHaveBeenCalledWith(getSpaceRoomName('space-7'));
   });

apps/server/src/ws/ws-tree.service.spec.ts

@@ -27,8 +27,7 @@ describe('WsTreeService', () => {
   let wsService: {
     emitTreeEvent: jest.Mock;
     emitToSpaceRoom: jest.Mock;
-    emitDeleteToUnauthorized: jest.Mock;
+    emitMoveWithRestrictionSplit: jest.Mock;
-    emitToAuthorizedUsers: jest.Mock;
   };
   let pagePermissionRepo: { hasRestrictedAncestor: jest.Mock };
 
@@ -36,8 +35,7 @@ describe('WsTreeService', () => {
     wsService = {
       emitTreeEvent: jest.fn().mockResolvedValue(undefined),
       emitToSpaceRoom: jest.fn(),
-      emitDeleteToUnauthorized: jest.fn().mockResolvedValue(undefined),
+      emitMoveWithRestrictionSplit: jest.fn().mockResolvedValue(undefined),
-      emitToAuthorizedUsers: jest.fn().mockResolvedValue(undefined),
     };
     pagePermissionRepo = {
       // Default: not restricted, so broadcastPageMoved skips the compensating
@@ -150,14 +148,13 @@ describe('WsTreeService', () => {
 
     await service.broadcastPageMoved(event);
 
-    // Normal path: move goes to the whole room via emitTreeEvent, and neither
+    // Normal path: move goes to the whole room via emitTreeEvent, and the
-    // the authorized-only move path nor the compensating delete fire.
+    // single-snapshot move/delete split does not fire.
     expect(wsService.emitTreeEvent).toHaveBeenCalledTimes(1);
-    expect(wsService.emitToAuthorizedUsers).not.toHaveBeenCalled();
+    expect(wsService.emitMoveWithRestrictionSplit).not.toHaveBeenCalled();
-    expect(wsService.emitDeleteToUnauthorized).not.toHaveBeenCalled();
   });
 
-  it('broadcastPageMoved into a RESTRICTED subtree routes the move to authorized users only AND emits a compensating delete to unauthorized — from one fresh decision', async () => {
+  it('broadcastPageMoved into a RESTRICTED subtree drives the move + compensating delete from ONE single-snapshot split call', async () => {
     // Destination is now under a restricted ancestor.
     pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true);
 
@@ -180,11 +177,18 @@ describe('WsTreeService', () => {
     // which could leak the move to the whole room during the stale-cache window.
     expect(wsService.emitTreeEvent).not.toHaveBeenCalled();
 
-    // The move is delivered to authorized users only.
+    // BOTH the move and the compensating delete are driven from ONE call, so a
-    expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledTimes(1);
+    // single socket/access snapshot partitions the room (no race window).
-    expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledWith(
+    expect(wsService.emitMoveWithRestrictionSplit).toHaveBeenCalledTimes(1);
-      'space-1',
+
-      'page-1',
+    const [spaceId, pageId, movePayload, deletePayload] =
+      wsService.emitMoveWithRestrictionSplit.mock.calls[0];
+
+    expect(spaceId).toBe('space-1');
+    expect(pageId).toBe('page-1');
+
+    // The move payload is the moveTreeNode for the moved page.
+    expect(movePayload).toEqual(
       expect.objectContaining({
         operation: 'moveTreeNode',
         spaceId: 'space-1',
@@ -192,20 +196,23 @@ describe('WsTreeService', () => {
       }),
     );
 
-    // The users who lost access get a deleteTreeNode for the moved node, scoped
+    // The delete payload is the compensating deleteTreeNode, scoped to the same
-    // to the same page id (same fresh authorized set → disjoint from the move).
+    // page id and carrying the OLD parent id (so it disappears from where it was
-    expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledTimes(1);
+    // last visible).
-    expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledWith(
+    expect(deletePayload).toEqual(
-      'space-1',
-      'page-1',
       expect.objectContaining({
         operation: 'deleteTreeNode',
         spaceId: 'space-1',
         payload: {
-          node: expect.objectContaining({ id: 'page-1', slugId: 'slug-1' }),
+          node: expect.objectContaining({
+            id: 'page-1',
+            slugId: 'slug-1',
+            parentPageId: 'old-parent',
+          }),
         },
       }),
     );
+    expect(deletePayload.payload.node.parentPageId).toBe(event.oldParentId);
   });
 
   it('broadcastRefetchRoot emits refetchRootTreeNodeEvent to the space room', async () => {
@@ -339,7 +346,7 @@ describe('WsService.emitTreeEvent', () => {
     );
   });
 
-  it('emitDeleteToUnauthorized sends ONLY to sockets whose user lacks page access', async () => {
+  it('emitMoveWithRestrictionSplit partitions the room from one snapshot: authorized -> move, unauthorized + anonymous -> delete', async () => {
     pagePermissionRepo.getUserIdsWithPageAccess.mockResolvedValue(['user-ok']);
 
     const okEmit = jest.fn();
@@ -348,38 +355,49 @@ describe('WsService.emitTreeEvent', () => {
     const sockets = [
       { id: 's1', data: { userId: 'user-ok' }, emit: okEmit },
       { id: 's2', data: { userId: 'user-no' }, emit: noEmit },
-      // Unauthenticated socket (no userId) — must also receive the delete.
+      // Unauthenticated socket (no userId) — must receive the delete.
       { id: 's3', data: {}, emit: anonEmit },
     ];
     server.in.mockReturnValue({
       fetchSockets: jest.fn().mockResolvedValue(sockets),
     });
 
-    const data = { operation: 'deleteTreeNode' };
+    const movePayload = { operation: 'moveTreeNode' };
-    await service.emitDeleteToUnauthorized('space-1', 'page-1', data);
+    const deletePayload = { operation: 'deleteTreeNode' };
+    await service.emitMoveWithRestrictionSplit(
+      'space-1',
+      'page-1',
+      movePayload,
+      deletePayload,
+    );
 
-    // Authorized user does NOT get the delete (they got the move instead).
+    // Authorized socket gets ONLY the move.
-    expect(okEmit).not.toHaveBeenCalled();
+    expect(okEmit).toHaveBeenCalledWith('message', movePayload);
-    // Unauthorized + anonymous sockets DO get the delete.
+    expect(okEmit).not.toHaveBeenCalledWith('message', deletePayload);
-    expect(noEmit).toHaveBeenCalledWith('message', data);
+    // Unauthorized + anonymous sockets get ONLY the delete.
-    expect(anonEmit).toHaveBeenCalledWith('message', data);
+    expect(noEmit).toHaveBeenCalledWith('message', deletePayload);
+    expect(noEmit).not.toHaveBeenCalledWith('message', movePayload);
+    expect(anonEmit).toHaveBeenCalledWith('message', deletePayload);
+    expect(anonEmit).not.toHaveBeenCalledWith('message', movePayload);
   });
 });
 
 describe('move-into-restricted disjointness contract (WsTreeService + real WsService)', () => {
-  // CONTRACT: a move under a restricted ancestor PARTITIONS the room. The
+  // CONTRACT: a move under a restricted ancestor PARTITIONS the room from a
-  // authorized set (gets the moveTreeNode via emitToAuthorizedUsers) and its
+  // SINGLE snapshot. emitMoveWithRestrictionSplit performs exactly one
-  // complement (gets the deleteTreeNode via emitDeleteToUnauthorized) are
+  // fetchSockets + one getUserIdsWithPageAccess; the authorized set (gets the
-  // disjoint and together cover every socket — and an anonymous (no-userId)
+  // moveTreeNode) and its complement (gets the deleteTreeNode) are disjoint and
-  // socket lands in the delete set. We wire a REAL WsService (only its repo,
+  // together cover every socket — and an anonymous (no-userId) socket lands in
-  // cache and socket server mocked) so both broadcasts run against the SAME fixed
+  // the delete set. We wire a REAL WsService (only its repo, cache and socket
-  // socket set, the way they do in production.
+  // server mocked) so the partition runs against the SAME fixed socket set, the
+  // way it does in production.
   let treeService: WsTreeService;
   let pagePermissionRepo: {
     hasRestrictedPagesInSpace: jest.Mock;
     hasRestrictedAncestor: jest.Mock;
     getUserIdsWithPageAccess: jest.Mock;
   };
+  let fetchSockets: jest.Mock;
 
   // Fixed room: two authorized users (one with two sockets), one unauthorized
   // user, one anonymous socket.
@@ -429,11 +447,12 @@ describe('move-into-restricted disjointness contract (WsTreeService + real WsSer
     }).compile();
 
     const wsService = module.get<WsService>(WsService);
+    // Capture fetchSockets so the test can assert the SINGLE-snapshot contract:
+    // exactly one fetchSockets call drives the whole partition.
+    fetchSockets = jest.fn().mockResolvedValue(sockets);
     const server = {
       to: jest.fn().mockReturnValue({ emit: jest.fn() }),
-      in: jest.fn().mockReturnValue({
+      in: jest.fn().mockReturnValue({ fetchSockets }),
-        fetchSockets: jest.fn().mockResolvedValue(sockets),
-      }),
     };
     wsService.setServer(server as never);
 
@@ -469,5 +488,12 @@ describe('move-into-restricted disjointness contract (WsTreeService + real WsSer
     // The anonymous socket specifically lands in the DELETE set, never the move.
     expect(deleteSet.has('s-anon')).toBe(true);
     expect(moveSet.has('s-anon')).toBe(false);
+
+    // SINGLE SNAPSHOT: the whole partition (move + compensating delete) is driven
+    // from exactly ONE fetchSockets and exactly ONE getUserIdsWithPageAccess.
+    // This is what closes the race window — there is no second, independent
+    // snapshot that could disagree with the first.
+    expect(fetchSockets).toHaveBeenCalledTimes(1);
+    expect(pagePermissionRepo.getUserIdsWithPageAccess).toHaveBeenCalledTimes(1);
   });
 });

apps/server/src/ws/ws-tree.service.ts

@@ -131,22 +131,20 @@ export class WsTreeService {
     }
 
     // Restricted case: a move can push a previously-visible page UNDER a
-    // restricted ancestor. Route the move to authorized users ONLY (same fresh
+    // restricted ancestor. The move (to authorized users) and the compensating
-    // getUserIdsWithPageAccess set the delete uses) and send the compensating
+    // delete (to everyone else) are now driven from ONE socket/access snapshot:
-    // delete to everyone else. Both sets come from one fresh decision, so they
+    // emitMoveWithRestrictionSplit performs a single fetchSockets + a single
-    // are guaranteed disjoint: authorized users get exactly the moveTreeNode,
+    // getUserIdsWithPageAccess and partitions the room from that one snapshot.
-    // unauthorized users get exactly the deleteTreeNode, nobody gets both.
+    // This eliminates the race window that existed when the move and the delete
+    // each resolved the audience independently — a socket could otherwise have
+    // landed in both sets (leaking the restricted node) or in neither (losing the
+    // compensating delete). Authorized users get exactly the moveTreeNode,
+    // everyone else (unauthorized + anonymous) gets exactly the deleteTreeNode.
     //
     // Users who LOSE visibility need the delete because otherwise the node would
     // linger in their tree at its old parent with its real title/slugId/icon
     // (existence + metadata leak).
-    await this.wsService.emitToAuthorizedUsers(
+    await this.wsService.emitMoveWithRestrictionSplit(node.spaceId, node.id, movePayload, {
-      node.spaceId,
-      node.id,
-      movePayload,
-    );
-
-    await this.wsService.emitDeleteToUnauthorized(node.spaceId, node.id, {
       operation: 'deleteTreeNode',
       spaceId: node.spaceId,
       payload: {

apps/server/src/ws/ws.service.ts

@@ -118,19 +118,42 @@ export class WsService {
     this.server.to(getSpaceRoomName(spaceId)).emit('message', data);
   }
 
-  // Broadcast `data` (a deleteTreeNode) to every socket in the space room whose
+  // Single-snapshot move broadcast. This is the ONE place that fans out a move
-  // user is NOT authorized to see `pageId`. Used to compensate a move that pushes
+  // under a restricted ancestor together with its compensating delete, resolving
-  // a previously-visible page UNDER a restricted ancestor: authorized users get
+  // the audience EXACTLY ONCE so the two never disagree.
-  // the moveTreeNode (via emitTreeEvent), everyone else gets a deleteTreeNode so
+  //
-  // the now-restricted node disappears from their tree instead of lingering with
+  // It takes a SINGLE socket snapshot (`this.server.in(room).fetchSockets()` is
-  // its real title/slugId/icon. The two event sets are disjoint by construction
+  // called exactly once) and a SINGLE authorization resolution
-  // (a user is either authorized or not), so no socket receives both.
+  // (`getUserIdsWithPageAccess` is called exactly once). From that one snapshot it
-  async emitDeleteToUnauthorized(
+  // partitions the room into two groups and emits to each:
+  //   - authorized users (their userId is in the authorized set) receive
+  //     `movePayload` (the moveTreeNode);
+  //   - everyone else — unauthorized users AND anonymous/no-userId sockets —
+  //     receive `deletePayload` (the compensating deleteTreeNode) so a now-hidden
+  //     node disappears from their tree instead of lingering with its real
+  //     title/slugId/icon.
+  // Because both groups are derived from the same socket array and the same
+  // authorized set, the partition is guaranteed DISJOINT (no socket gets both)
+  // and COMPLETE (every socket gets exactly one). This closes the race window
+  // that existed when the move and the compensating delete each ran their own
+  // independent fetchSockets + getUserIdsWithPageAccess: between those two
+  // snapshots a socket could connect/disconnect or its access change, so a socket
+  // could end up in both sets (leaking the restricted node, then no delete) or in
+  // neither (losing the compensating delete).
+  //
+  // It deliberately does NOT consult the cached spaceHasRestrictions: the caller
+  // (broadcastPageMoved) has already established, freshly and uncached, that the
+  // page is restricted, so we must not risk a stale cache fanning the move out to
+  // the whole room.
+  async emitMoveWithRestrictionSplit(
     spaceId: string,
     pageId: string,
-    data: any,
+    movePayload: any,
+    deletePayload: any,
   ): Promise<void> {
     const room = getSpaceRoomName(spaceId);
+
+    // ONE socket snapshot for the whole partition.
     const sockets = await this.server.in(room).fetchSockets();
     if (sockets.length === 0) return;
 
@@ -141,39 +164,26 @@ export class WsService {
           .filter((id): id is string => !!id),
       ),
     );
-    if (userIds.length === 0) return;
 
-    const authorizedUserIds =
+    // ONE authorization resolution for the whole partition.
-      await this.pagePermissionRepo.getUserIdsWithPageAccess(pageId, userIds);
+    const authorizedUserIds = userIds.length
+      ? await this.pagePermissionRepo.getUserIdsWithPageAccess(pageId, userIds)
+      : [];
     const authorizedSet = new Set(authorizedUserIds);
 
     for (const socket of sockets) {
       const userId = socket.data.userId as string;
-      // Unauthenticated sockets (no userId) cannot see restricted content; send
+      if (userId && authorizedSet.has(userId)) {
-      // them the delete too so a leaked node can't linger.
+        // Authorized: deliver the move.
-      if (!userId || !authorizedSet.has(userId)) {
+        socket.emit('message', movePayload);
-        socket.emit('message', data);
+      } else {
+        // Unauthorized OR anonymous (no userId): deliver the compensating
+        // delete so the now-hidden node can't linger.
+        socket.emit('message', deletePayload);
       }
     }
   }
 
-  // Server-origin broadcast of `data` to exactly the users in the space room who
-  // ARE authorized to see `pageId`. This is the counterpart of
-  // emitDeleteToUnauthorized: both resolve the authorized set from the SAME
-  // fetchSockets + getUserIdsWithPageAccess call shape, so a caller that drives
-  // both from one decision gets two disjoint sets (authorized vs. not) with no
-  // socket in both. Unlike emitTreeEvent, this does NOT consult the cached
-  // spaceHasRestrictions: the caller already knows the page is restricted, so we
-  // must not risk a stale cache fanning the move out to the whole room.
-  async emitToAuthorizedUsers(
-    spaceId: string,
-    pageId: string,
-    data: any,
-  ): Promise<void> {
-    const room = getSpaceRoomName(spaceId);
-    await this.broadcastToAuthorizedUsers(room, null, pageId, data);
-  }
-
   private async broadcastToAuthorizedUsers(
     room: string,
     excludeSocketId: string | null,