fix: db lock operations (#2479)

* fix: advisory lock for page move

* fix: lock role count check

apps/server/src/core/page/services/page.service.ts

@@ -1,5 +1,6 @@
 import {
   BadRequestException,
+  ConflictException,
   Injectable,
   Logger,
   NotFoundException,
@@ -20,7 +21,7 @@ import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
 import { MovePageDto } from '../dto/move-page.dto';
 import { generateSlugId } from '../../../common/helpers';
 import { getPageTitle } from '../../../common/helpers';
-import { executeTx } from '@docmost/db/utils';
+import { dbOrTx, executeTx } from '@docmost/db/utils';
 import { AttachmentRepo } from '@docmost/db/repos/attachment/attachment.repo';
 import { v7 as uuid7 } from 'uuid';
 import {
@@ -174,10 +175,14 @@ export class PageService {
     return page;
   }
 
-  async nextPagePosition(spaceId: string, parentPageId?: string) {
+  async nextPagePosition(
+    spaceId: string,
+    parentPageId?: string,
+    trx?: KyselyTransaction,
+  ) {
     let pagePosition: string;
 
-    const lastPageQuery = this.db
+    const lastPageQuery = dbOrTx(this.db, trx)
       .selectFrom('pages')
       .select(['position'])
       .where('spaceId', '=', spaceId)
@@ -391,35 +396,46 @@ export class PageService {
   }
 
   async movePageToSpace(rootPage: Page, spaceId: string, userId: string) {
-    let childPageIds: string[] = [];
+    return executeTx(this.db, async (trx) => {
+      await this.pageRepo.lockPageHierarchySpaces(
+        [rootPage.spaceId, spaceId],
+        trx,
+      );
 
-    const allPages = await this.pageRepo.getPageAndDescendants(rootPage.id, {
-      includeContent: false,
-    });
+      const currentRootPage = await this.pageRepo.findById(rootPage.id, {
+        trx,
+      });
+      if (!currentRootPage || currentRootPage.deletedAt) {
+        throw new NotFoundException('Page to move not found');
+      }
+      if (currentRootPage.spaceId !== rootPage.spaceId) {
+        throw new ConflictException('Page location changed; retry the move');
+      }
 
-    // Filter to only accessible pages while maintaining tree integrity
-    const accessiblePages = await this.filterAccessibleTreePages(
-      allPages,
-      rootPage.id,
-      userId,
-      rootPage.spaceId,
-    );
-    const accessibleIds = new Set(accessiblePages.map((p) => p.id));
+      const allPages = await this.pageRepo.getPageAndDescendants(
+        currentRootPage.id,
+        { includeContent: false, trx },
+      );
+      const accessiblePages = await this.filterAccessibleTreePages(
+        allPages,
+        currentRootPage.id,
+        userId,
+        currentRootPage.spaceId,
+      );
+      const accessibleIds = new Set(accessiblePages.map((p) => p.id));
+      const pagesToOrphan = allPages.filter(
+        (p) =>
+          !accessibleIds.has(p.id) &&
+          p.parentPageId &&
+          accessibleIds.has(p.parentPageId),
+      );
 
-    // Find inaccessible pages whose parent is being moved - these need to be orphaned
-    const pagesToOrphan = allPages.filter(
-      (p) =>
-        !accessibleIds.has(p.id) &&
-        p.parentPageId &&
-        accessibleIds.has(p.parentPageId),
-    );
-
-    await executeTx(this.db, async (trx) => {
       // Orphan inaccessible child pages (make them root pages in original space)
       for (const page of pagesToOrphan) {
         const orphanPosition = await this.nextPagePosition(
-          rootPage.spaceId,
+          currentRootPage.spaceId,
           null,
+          trx,
         );
         await this.pageRepo.updatePage(
           { parentPageId: null, position: orphanPosition },
@@ -429,16 +445,18 @@ export class PageService {
       }
 
       // Update root page
-      const nextPosition = await this.nextPagePosition(spaceId);
+      const nextPosition = await this.nextPagePosition(spaceId, null, trx);
       await this.pageRepo.updatePage(
         { spaceId, parentPageId: null, position: nextPosition },
-        rootPage.id,
+        currentRootPage.id,
         trx,
       );
 
       const pageIdsToMove = accessiblePages.map((p) => p.id);
 
-      childPageIds = pageIdsToMove.filter((id) => id !== rootPage.id);
+      const childPageIds = pageIdsToMove.filter(
+        (id) => id !== currentRootPage.id,
+      );
 
       if (pageIdsToMove.length > 1) {
         // Update sub pages (all accessible pages except root)
@@ -501,7 +519,7 @@ export class PageService {
           {
             pageIds: pageIdsToMove,
             spaceId,
-            workspaceId: rootPage.workspaceId,
+            workspaceId: currentRootPage.workspaceId,
           },
           {
             attempts: 2,
@@ -512,9 +530,9 @@ export class PageService {
           },
         );
       }
-    });
 
-    return { childPageIds };
+      return { childPageIds };
+    });
   }
 
   async duplicatePage(
@@ -825,31 +843,59 @@ export class PageService {
       throw new BadRequestException('A page cannot be its own parent');
     }
 
-    let parentPageId = null;
-    if (movedPage.parentPageId === dto.parentPageId) {
-      parentPageId = undefined;
-    } else {
-      // changing the page's parent
-      if (dto.parentPageId) {
-        const parentPage = await this.pageRepo.findById(dto.parentPageId);
-        if (
-          !parentPage ||
-          parentPage.deletedAt ||
-          parentPage.spaceId !== movedPage.spaceId
-        ) {
-          throw new NotFoundException('Parent page not found');
-        }
-        parentPageId = parentPage.id;
-      }
-    }
+    await executeTx(this.db, async (trx) => {
+      await this.pageRepo.lockPageHierarchySpaces(
+        [movedPage.spaceId],
+        trx,
+      );
 
-    await this.pageRepo.updatePage(
-      {
-        position: dto.position,
-        parentPageId: parentPageId,
-      },
-      dto.pageId,
-    );
+      const currentPage = await this.pageRepo.findById(dto.pageId, { trx });
+      if (!currentPage || currentPage.deletedAt) {
+        throw new NotFoundException('Moved page not found');
+      }
+      if (currentPage.spaceId !== movedPage.spaceId) {
+        throw new ConflictException('Page location changed; retry the move');
+      }
+
+      let parentPageId = null;
+      if (currentPage.parentPageId === dto.parentPageId) {
+        parentPageId = undefined;
+      } else {
+        if (dto.parentPageId) {
+          const parentPage = await this.pageRepo.findById(dto.parentPageId, {
+            trx,
+          });
+          if (
+            !parentPage ||
+            parentPage.deletedAt ||
+            parentPage.spaceId !== currentPage.spaceId
+          ) {
+            throw new NotFoundException('Parent page not found');
+          }
+          if (
+            await this.pageRepo.isPageDescendant(
+              dto.pageId,
+              parentPage.id,
+              trx,
+            )
+          ) {
+            throw new BadRequestException(
+              'A page cannot be moved under its descendant',
+            );
+          }
+          parentPageId = parentPage.id;
+        }
+      }
+
+      await this.pageRepo.updatePage(
+        {
+          position: dto.position,
+          parentPageId: parentPageId,
+        },
+        dto.pageId,
+        trx,
+      );
+    });
   }
 
   async getPageBreadCrumbs(childPageId: string) {

apps/server/src/core/space/services/space-member.service.ts

@@ -10,7 +10,7 @@ import { SpaceMemberRepo } from '@docmost/db/repos/space/space-member.repo';
 import { GroupUserRepo } from '@docmost/db/repos/group/group-user.repo';
 import { AddSpaceMembersDto } from '../dto/add-space-members.dto';
 import { InjectKysely } from 'nestjs-kysely';
-import { Space, SpaceMember, User } from '@docmost/db/types/entity.types';
+import { Space, User } from '@docmost/db/types/entity.types';
 import { SpaceRepo } from '@docmost/db/repos/space/space.repo';
 import { RemoveSpaceMemberDto } from '../dto/remove-space-member.dto';
 import { UpdateSpaceMemberRoleDto } from '../dto/update-space-member-role.dto';
@@ -218,41 +218,18 @@ export class SpaceMemberService {
     dto: RemoveSpaceMemberDto,
     workspaceId: string,
   ): Promise<void> {
-    const space = await this.spaceRepo.findById(dto.spaceId, workspaceId);
-    if (!space) {
-      throw new NotFoundException('Space not found');
-    }
+    const memberTypeId = dto.userId
+      ? { userId: dto.userId }
+      : dto.groupId
+        ? { groupId: dto.groupId }
+        : null;
 
-    let spaceMember: SpaceMember = null;
-
-    if (dto.userId) {
-      spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId(
-        dto.spaceId,
-        {
-          userId: dto.userId,
-        },
-      );
-    } else if (dto.groupId) {
-      spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId(
-        dto.spaceId,
-        {
-          groupId: dto.groupId,
-        },
-      );
-    } else {
+    if (!memberTypeId) {
       throw new BadRequestException(
         'Please provide a valid userId or groupId to remove',
       );
     }
 
-    if (!spaceMember) {
-      throw new NotFoundException('Space membership not found');
-    }
-
-    if (spaceMember.role === SpaceRole.ADMIN) {
-      await this.validateLastAdmin(dto.spaceId);
-    }
-
     let affectedUserIds: string[] = [];
     if (dto.userId) {
       affectedUserIds = [dto.userId];
@@ -262,7 +239,29 @@ export class SpaceMemberService {
       );
     }
 
-    await executeTx(this.db, async (trx) => {
+    const { space, spaceMember } = await executeTx(this.db, async (trx) => {
+      const space = await this.spaceRepo.findById(
+        dto.spaceId,
+        workspaceId,
+        { withLock: true, trx },
+      );
+      if (!space) {
+        throw new NotFoundException('Space not found');
+      }
+
+      const spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId(
+        dto.spaceId,
+        memberTypeId,
+        trx,
+      );
+      if (!spaceMember) {
+        throw new NotFoundException('Space membership not found');
+      }
+
+      if (spaceMember.role === SpaceRole.ADMIN) {
+        await this.validateLastAdmin(dto.spaceId, trx);
+      }
+
       await this.spaceMemberRepo.removeSpaceMemberById(
         spaceMember.id,
         dto.spaceId,
@@ -280,6 +279,8 @@ export class SpaceMemberService {
         dto.spaceId,
         { trx },
       );
+
+      return { space, spaceMember };
     });
 
     this.auditService.log({
@@ -304,48 +305,40 @@ export class SpaceMemberService {
     dto: UpdateSpaceMemberRoleDto,
     workspaceId: string,
   ): Promise<void> {
-    const space = await this.spaceRepo.findById(dto.spaceId, workspaceId);
-    if (!space) {
-      throw new NotFoundException('Space not found');
-    }
+    const memberTypeId = dto.userId
+      ? { userId: dto.userId }
+      : dto.groupId
+        ? { groupId: dto.groupId }
+        : null;
 
-    let spaceMember: SpaceMember = null;
-
-    if (dto.userId) {
-      spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId(
-        dto.spaceId,
-        {
-          userId: dto.userId,
-        },
-      );
-    } else if (dto.groupId) {
-      spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId(
-        dto.spaceId,
-        {
-          groupId: dto.groupId,
-        },
-      );
-    } else {
+    if (!memberTypeId) {
       throw new BadRequestException(
         'Please provide a valid userId or groupId to remove',
       );
     }
 
-    if (!spaceMember) {
-      throw new NotFoundException('Space membership not found');
-    }
+    const result = await executeTx(this.db, async (trx) => {
+      const space = await this.spaceRepo.findById(
+        dto.spaceId,
+        workspaceId,
+        { withLock: true, trx },
+      );
+      if (!space) {
+        throw new NotFoundException('Space not found');
+      }
 
-    if (spaceMember.role === dto.role) {
-      return;
-    }
+      const spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId(
+        dto.spaceId,
+        memberTypeId,
+        trx,
+      );
+      if (!spaceMember) {
+        throw new NotFoundException('Space membership not found');
+      }
 
-    await executeTx(this.db, async (trx) => {
-      await trx
-        .selectFrom('spaces')
-        .select('id')
-        .where('id', '=', dto.spaceId)
-        .forUpdate()
-        .executeTakeFirst();
+      if (spaceMember.role === dto.role) {
+        return { changed: false, space, spaceMember };
+      }
 
       if (spaceMember.role === SpaceRole.ADMIN) {
         await this.validateLastAdmin(dto.spaceId, trx);
@@ -357,8 +350,16 @@ export class SpaceMemberService {
         dto.spaceId,
         trx,
       );
+
+      return { changed: true, space, spaceMember };
     });
 
+    if (!result.changed) {
+      return;
+    }
+
+    const { space, spaceMember } = result;
+
     this.auditService.log({
       event: AuditEvent.SPACE_MEMBER_ROLE_CHANGED,
       resourceType: AuditResource.SPACE_MEMBER,
@@ -387,7 +388,7 @@ export class SpaceMemberService {
       spaceId,
       trx,
     );
-    if (spaceOwnerCount === 1) {
+    if (spaceOwnerCount <= 1) {
       throw new BadRequestException(
         'There must be at least one space admin with full access',
       );

apps/server/src/core/workspace/services/workspace.service.ts

@@ -747,44 +747,61 @@ export class WorkspaceService {
     userRoleDto: UpdateWorkspaceUserRoleDto,
     workspaceId: string,
   ) {
-    const user = await this.userRepo.findById(userRoleDto.userId, workspaceId);
-
     const newRole = userRoleDto.role.toLowerCase();
+    const result = await executeTx(this.db, async (trx) => {
+      const workspace = await this.workspaceRepo.findById(workspaceId, {
+        withLock: true,
+        trx,
+      });
+      if (!workspace) {
+        throw new NotFoundException('Workspace not found');
+      }
 
-    if (!user) {
-      throw new BadRequestException('Workspace member not found');
-    }
-
-    // prevent ADMIN from managing OWNER role
-    if (
-      isAdminActingOnOwner(authUser.role, newRole) ||
-      isAdminActingOnOwner(authUser.role, user.role)
-    ) {
-      throw new ForbiddenException();
-    }
-
-    if (user.role === newRole) {
-      return user;
-    }
-
-    const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId(
-      UserRole.OWNER,
-      workspaceId,
-    );
-
-    if (user.role === UserRole.OWNER && workspaceOwnerCount === 1) {
-      throw new BadRequestException(
-        'There must be at least one workspace owner',
+      const user = await this.userRepo.findById(
+        userRoleDto.userId,
+        workspaceId,
+        { trx },
       );
+      if (!user) {
+        throw new BadRequestException('Workspace member not found');
+      }
+
+      if (
+        isAdminActingOnOwner(authUser.role, newRole) ||
+        isAdminActingOnOwner(authUser.role, user.role)
+      ) {
+        throw new ForbiddenException();
+      }
+
+      if (user.role === newRole) {
+        return { changed: false, user };
+      }
+
+      if (
+        user.role === UserRole.OWNER &&
+        !user.deletedAt &&
+        !user.deactivatedAt
+      ) {
+        await this.validateLastWorkspaceOwner(workspaceId, trx);
+      }
+
+      await this.userRepo.updateUser(
+        {
+          role: newRole,
+        },
+        user.id,
+        workspaceId,
+        trx,
+      );
+
+      return { changed: true, user };
+    });
+
+    if (!result.changed) {
+      return result.user;
     }
 
-    await this.userRepo.updateUser(
-      {
-        role: newRole,
-      },
-      user.id,
-      workspaceId,
-    );
+    const { user } = result;
 
     this.auditService.log({
       event: AuditEvent.USER_ROLE_CHANGED,
@@ -848,40 +865,38 @@ export class WorkspaceService {
     userId: string,
     workspaceId: string,
   ): Promise<void> {
-    const user = await this.userRepo.findById(userId, workspaceId);
+    const user = await executeTx(this.db, async (trx) => {
+      const workspace = await this.workspaceRepo.findById(workspaceId, {
+        withLock: true,
+        trx,
+      });
+      if (!workspace) {
+        throw new NotFoundException('Workspace not found');
+      }
 
-    if (!user || user.deletedAt) {
-      throw new BadRequestException('Workspace member not found');
-    }
+      const user = await this.userRepo.findById(userId, workspaceId, { trx });
+      if (!user || user.deletedAt) {
+        throw new BadRequestException('Workspace member not found');
+      }
 
-    if (user.deactivatedAt) {
-      throw new BadRequestException('User is already deactivated');
-    }
+      if (user.deactivatedAt) {
+        throw new BadRequestException('User is already deactivated');
+      }
 
-    if (authUser.id === userId) {
-      throw new BadRequestException('You cannot deactivate yourself');
-    }
+      if (authUser.id === userId) {
+        throw new BadRequestException('You cannot deactivate yourself');
+      }
 
-    if (isAdminActingOnOwner(authUser.role, user.role)) {
-      throw new BadRequestException(
-        'You cannot deactivate a user with owner role',
-      );
-    }
-
-    if (user.role === UserRole.OWNER) {
-      const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId(
-        UserRole.OWNER,
-        workspaceId,
-      );
-
-      if (workspaceOwnerCount === 1) {
+      if (isAdminActingOnOwner(authUser.role, user.role)) {
         throw new BadRequestException(
-          'There must be at least one workspace owner',
+          'You cannot deactivate a user with owner role',
         );
       }
-    }
 
-    await executeTx(this.db, async (trx) => {
+      if (user.role === UserRole.OWNER) {
+        await this.validateLastWorkspaceOwner(workspaceId, trx);
+      }
+
       await this.userRepo.updateUser(
         { deactivatedAt: new Date() },
         userId,
@@ -889,6 +904,8 @@ export class WorkspaceService {
         trx,
       );
       await this.userSessionRepo.revokeByUserId(userId, workspaceId, trx);
+
+      return user;
     });
 
     this.auditService.log({
@@ -951,32 +968,34 @@ export class WorkspaceService {
     userId: string,
     workspaceId: string,
   ): Promise<void> {
-    const user = await this.userRepo.findById(userId, workspaceId);
+    const user = await executeTx(this.db, async (trx) => {
+      const workspace = await this.workspaceRepo.findById(workspaceId, {
+        withLock: true,
+        trx,
+      });
+      if (!workspace) {
+        throw new NotFoundException('Workspace not found');
+      }
 
-    if (!user || user.deletedAt) {
-      throw new BadRequestException('Workspace member not found');
-    }
+      const user = await this.userRepo.findById(userId, workspaceId, { trx });
+      if (!user || user.deletedAt) {
+        throw new BadRequestException('Workspace member not found');
+      }
 
-    const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId(
-      UserRole.OWNER,
-      workspaceId,
-    );
+      if (authUser.id === userId) {
+        throw new BadRequestException('You cannot delete yourself');
+      }
 
-    if (user.role === UserRole.OWNER && workspaceOwnerCount === 1) {
-      throw new BadRequestException(
-        'There must be at least one workspace owner',
-      );
-    }
+      if (isAdminActingOnOwner(authUser.role, user.role)) {
+        throw new BadRequestException(
+          'You cannot delete a user with owner role',
+        );
+      }
 
-    if (authUser.id === userId) {
-      throw new BadRequestException('You cannot delete yourself');
-    }
+      if (user.role === UserRole.OWNER && !user.deactivatedAt) {
+        await this.validateLastWorkspaceOwner(workspaceId, trx);
+      }
 
-    if (isAdminActingOnOwner(authUser.role, user.role)) {
-      throw new BadRequestException('You cannot delete a user with owner role');
-    }
-
-    await executeTx(this.db, async (trx) => {
       await this.userRepo.updateUser(
         {
           name: 'Deleted user',
@@ -1009,6 +1028,8 @@ export class WorkspaceService {
       });
 
       await this.userSessionRepo.revokeByUserId(userId, workspaceId, trx);
+
+      return user;
     });
 
     this.auditService.log({
@@ -1030,4 +1051,20 @@ export class WorkspaceService {
       // empty
     }
   }
+
+  private async validateLastWorkspaceOwner(
+    workspaceId: string,
+    trx: KyselyTransaction,
+  ): Promise<void> {
+    const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId(
+      UserRole.OWNER,
+      workspaceId,
+      trx,
+    );
+    if (workspaceOwnerCount <= 1) {
+      throw new BadRequestException(
+        'There must be at least one workspace owner',
+      );
+    }
+  }
 }