fix(page): serialize concurrent moves so opposing re-parents can't form a cycle (#159)

Red-team finding #9: two opposing moves racing each other — A drags X under Y
while B drags Y under X — each passed a cycle check built from a stale snapshot
and both committed, leaving X.parent=Y AND Y.parent=X. That cycle has no path to
root, so the recursive ancestor CTEs break and both subtrees disappear from the
sidebar. The cycle guard (breadcrumb read) and the parent-update write were not
in a transaction.

For a genuine re-parent under a concrete parent (the only cycle-capable path),
wrap the guard + update in one executeTx transaction and:
- lock the moved page and the destination parent rows FOR UPDATE
  (pageRepo.findById(id, { withLock: true, trx })) in a canonical, id-sorted
  order so the two opposing moves serialize instead of deadlocking;
- re-run the ancestor cycle check INSIDE the transaction, so the loser sees the
  winner's committed re-parent and is rejected with the existing
  "Cannot move a page into its own subtree" error.
Same-parent reorder (parentPageId undefined) and move-to-root (null) keep the
plain non-transactional update — neither can create a cycle. The phantom-
broadcast gate and PAGE_MOVED emit are unchanged. getPageBreadCrumbs gains an
optional trx so its recursive CTE can run inside the locked transaction.

Scope: this closes the two-party opposing-move race (finding #9). A longer
3-party chain (A→B, B→C, C→A) is out of scope and left as a known limitation.

Tests:
- unit (page.service.spec.ts): assert the lock wiring — findById called with
  { withLock: true, trx } for both ids in sorted order, getPageBreadCrumbs and
  updatePage receive the trx — while keeping the self-move / cycle-reject and
  legitimate-move guarantees.
- integration (page-move-cycle-concurrency.int-spec.ts, real Postgres in CI):
  two opposing concurrent moves resolve to exactly one success + one rejection
  and never persist a cycle.

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

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

@@ -40,11 +40,27 @@ describe('PageService', () => {
     // getPageBreadCrumbs are mockable, while every other collaborator stays a
     // bare stub. We only need to drive the three cycle-guard branches, so we
     // mock minimally rather than standing up the whole DI graph.
+    // A permissive chainable Proxy stands in for the Kysely trx so the
+    // FOR-UPDATE lock query chain inside the transaction resolves. Mirrors the
+    // pattern used by the movePageToSpace() spec below.
+    const makeChain = () => {
+      const c: any = new Proxy(function () {}, {
+        get: (_t, p) =>
+          p === 'then'
+            ? undefined
+            : p === 'execute' || p === 'executeTakeFirst'
+              ? () => Promise.resolve([])
+              : () => c,
+      });
+      return c;
+    };
+
     const makeService = (overrides?: {
       breadcrumbs?: Array<{ id: string }>;
     }) => {
       const pageRepo = {
-        // Destination parent lookup: a valid, non-deleted, same-space page.
+        // Destination parent lookup: a valid, non-deleted, same-space page. Also
+        // serves the FOR-UPDATE lock reads inside the transaction.
         findById: jest.fn().mockResolvedValue({
           id: 'dest-parent',
           deletedAt: null,
@@ -57,11 +73,19 @@ describe('PageService', () => {
 
       const eventEmitter = { emit: jest.fn() };
 
+      // Re-parenting under a concrete parent now runs through
+      // executeTx(this.db, ...), which calls db.transaction().execute(fn). The
+      // trxStub is the value handed to the callback (the locked transaction).
+      const trxStub = makeChain();
+      const db = {
+        transaction: () => ({ execute: (fn: any) => fn(trxStub) }),
+      };
+
       const svc = new PageService(
         pageRepo as any, // pageRepo
         {} as any, // pagePermissionRepo
         {} as any, // attachmentRepo
-        {} as any, // db
+        db as any, // db
         {} as any, // storageService
         {} as any, // attachmentQueue
         {} as any, // aiQueue
@@ -79,7 +103,7 @@ describe('PageService', () => {
         .spyOn(svc, 'getPageBreadCrumbs')
         .mockResolvedValue((overrides?.breadcrumbs ?? []) as any);
 
-      return { svc, pageRepo, eventEmitter };
+      return { svc, pageRepo, eventEmitter, trxStub };
     };
 
     // movePage takes `movedPage` as a param. Keep its parentPageId distinct from
@@ -146,6 +170,45 @@ describe('PageService', () => {
       await expect(svc.movePage(dto, makeMovedPage())).resolves.not.toThrow();
       expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
     });
+
+    it('serializes a legitimate re-parent under FOR UPDATE in canonical lock order (#159 #9)', async () => {
+      // Destination's ancestor chain does NOT contain the moved page -> no cycle.
+      const { svc, pageRepo, trxStub } = makeService({
+        breadcrumbs: [{ id: 'dest-parent' }, { id: 'root' }],
+      });
+      const getBreadcrumbsSpy = jest.spyOn(svc, 'getPageBreadCrumbs');
+      const dto: MovePageDto = {
+        pageId: 'page-1',
+        position: VALID_POSITION,
+        parentPageId: 'dest-parent',
+      };
+
+      await expect(svc.movePage(dto, makeMovedPage())).resolves.not.toThrow();
+
+      // Both rows are locked FOR UPDATE inside the transaction: findById is
+      // called with { withLock: true, trx: <the locked tx> } for the moved page
+      // and the destination parent.
+      const lockCalls = pageRepo.findById.mock.calls.filter(
+        (c: any[]) => c[1]?.withLock === true,
+      );
+      expect(lockCalls).toHaveLength(2);
+      for (const call of lockCalls) {
+        expect(call[1].withLock).toBe(true);
+        expect(call[1].trx).toBe(trxStub);
+      }
+
+      // Locks are acquired in a canonical (id-sorted) order so the two opposing
+      // moves serialize without deadlocking.
+      const lockedIds = lockCalls.map((c: any[]) => c[0]);
+      expect(lockedIds).toEqual(['page-1', 'dest-parent'].sort());
+
+      // The cycle re-check runs inside the locked transaction (trx passed).
+      expect(getBreadcrumbsSpy).toHaveBeenCalledWith('dest-parent', trxStub);
+
+      // The update is written inside the same transaction (trx is the 3rd arg).
+      expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
+      expect(pageRepo.updatePage.mock.calls[0][2]).toBe(trxStub);
+    });
   });
 
   describe('agent provenance stamping (#143)', () => {
@@ -259,6 +322,19 @@ describe('PageService', () => {
 
     describe('movePage() → updatePage', () => {
       const VALID_POSITION = 'a0';
+      // Re-parenting under a concrete parent runs through executeTx(this.db, ...);
+      // a permissive chainable Proxy stands in for the locked Kysely trx.
+      const makeChain = () => {
+        const c: any = new Proxy(function () {}, {
+          get: (_t, p) =>
+            p === 'then'
+              ? undefined
+              : p === 'execute' || p === 'executeTakeFirst'
+                ? () => Promise.resolve([])
+                : () => c,
+        });
+        return c;
+      };
       const run = async (provenance: any) => {
         const pageRepo = {
           findById: jest.fn().mockResolvedValue({
@@ -268,9 +344,12 @@ describe('PageService', () => {
           }),
           updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }),
         };
+        const trxStub = makeChain();
         const svc = makeSvc({
           pageRepo,
-          db: {} as any,
+          db: {
+            transaction: () => ({ execute: (fn: any) => fn(trxStub) }),
+          } as any,
         });
         // Legitimate move: destination ancestors do NOT include the moved page.
         jest

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

@@ -15,13 +15,13 @@ import {
   executeWithCursorPagination,
 } from '@docmost/db/pagination/cursor-pagination';
 import { InjectKysely } from 'nestjs-kysely';
-import { KyselyDB } from '@docmost/db/types/kysely.types';
+import { KyselyDB, KyselyTransaction } from '@docmost/db/types/kysely.types';
 import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
 import { MovePageDto } from '../dto/move-page.dto';
 import { shapeSidebarPagesTree } from './sidebar-pages-tree.util';
 import { generateSlugId } from '../../../common/helpers';
 import { getPageTitle } from '../../../common/helpers';
-import { executeTx } from '@docmost/db/utils';
+import { executeTx, dbOrTx } from '@docmost/db/utils';
 import { AttachmentRepo } from '@docmost/db/repos/attachment/attachment.repo';
 import { v7 as uuid7 } from 'uuid';
 import {
@@ -915,34 +915,49 @@ export class PageService {
       }
     }
 
-    // Server-side cycle guard: a page may not be moved into itself or into any
+    // Cheap self-move guard (no DB) — keep before the transaction.
-    // page within its own subtree. Without this, an MCP/REST/agent caller (or a
+    if (dto.parentPageId && dto.parentPageId === dto.pageId) {
-    // fast drag racing the client check) could persist a cycle and broadcast it.
-    // Only relevant when re-parenting under a concrete parent; moving to root
-    // (parentPageId null/undefined) can never create a cycle.
-    if (dto.parentPageId) {
-      if (dto.parentPageId === dto.pageId) {
       throw new BadRequestException('Cannot move a page into its own subtree');
     }
-      // Walk the destination parent's ancestor chain (reusing the breadcrumb
-      // ancestor CTE). If the page being moved appears among those ancestors,
-      // the destination lives inside the moved page's subtree -> cycle.
-      const destAncestors = await this.getPageBreadCrumbs(dto.parentPageId);
-      if (destAncestors.some((ancestor) => ancestor.id === dto.pageId)) {
-        throw new BadRequestException('Cannot move a page into its own subtree');
-      }
-    }
 
-    const updateResult = await this.pageRepo.updatePage(
+    const updateValues = {
-      {
       position: dto.position,
       parentPageId: parentPageId,
       // Agent-edit provenance: annotate the source on an agent move. A normal
       // user request leaves the existing source value unchanged.
       ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'),
-      },
+    };
-      dto.pageId,
+
+    let updateResult;
+    if (typeof parentPageId === 'string') {
+      // Genuine re-parent under a concrete parent: the ONLY path that can create
+      // a cycle. Two opposing moves (A: X under Y, B: Y under X) racing each
+      // other could each pass a cycle check built from a stale snapshot and
+      // persist a cycle (#159 finding #9). Serialize them: lock the moved page
+      // and the destination parent FOR UPDATE in a canonical (id-sorted) order
+      // so they cannot deadlock, then run the cycle check INSIDE the transaction
+      // against the now-committed state.
+      updateResult = await executeTx(this.db, async (trx) => {
+        // Both opposing moves touch the same two rows {pageId, parentPageId};
+        // a fixed lock order forces one to wait for the other to commit.
+        const lockIds = [dto.pageId, parentPageId].sort();
+        for (const id of lockIds) {
+          await this.pageRepo.findById(id, { withLock: true, trx });
+        }
+        // Re-read the destination's ancestor chain within the locked tx: it now
+        // reflects any concurrent re-parent that committed before we got the lock.
+        const destAncestors = await this.getPageBreadCrumbs(parentPageId, trx);
+        if (destAncestors.some((ancestor) => ancestor.id === dto.pageId)) {
+          throw new BadRequestException(
+            'Cannot move a page into its own subtree',
           );
+        }
+        return this.pageRepo.updatePage(updateValues, dto.pageId, trx);
+      });
+    } else {
+      // Same-parent reorder or move-to-root: no cycle possible, no lock needed.
+      updateResult = await this.pageRepo.updatePage(updateValues, dto.pageId);
+    }
 
     // Guard against a phantom broadcast: if the row was concurrently deleted or
     // otherwise not updated, skip the PAGE_MOVED event so we don't replay a move
@@ -981,8 +996,8 @@ export class PageService {
     });
   }
 
-  async getPageBreadCrumbs(childPageId: string) {
+  async getPageBreadCrumbs(childPageId: string, trx?: KyselyTransaction) {
-    const ancestors = await this.db
+    const ancestors = await dbOrTx(this.db, trx)
       .withRecursive('page_ancestors', (db) =>
         db
           .selectFrom('pages')

apps/server/test/integration/page-move-cycle-concurrency.int-spec.ts

@@ -0,0 +1,122 @@
+import { Kysely } from 'kysely';
+import { BadRequestException } from '@nestjs/common';
+import { PageRepo } from '@docmost/db/repos/page/page.repo';
+import { PageService } from '../../src/core/page/services/page.service';
+import {
+  getTestDb,
+  destroyTestDb,
+  createWorkspace,
+  createSpace,
+  createPage,
+} from './db';
+
+/**
+ * #159 finding #9 — concurrent opposing page moves must not create a cycle.
+ *
+ * User A drags X under Y while user B simultaneously drags Y under X. Before the
+ * fix, the cycle guard (a breadcrumb/ancestor read) and the parent-update write
+ * were NOT in a transaction, so both moves ran against a stale snapshot, both
+ * passed their cycle check, and both committed -> X.parent=Y AND Y.parent=X: a
+ * cycle with no path to root, which breaks the recursive ancestor CTEs and makes
+ * both subtrees vanish from the sidebar.
+ *
+ * The fix wraps the cycle check + update in one READ COMMITTED transaction and
+ * locks the two involved rows FOR UPDATE in a canonical (id-sorted) order, then
+ * re-runs the cycle check inside the lock. One move wins; the loser sees the
+ * committed re-parent and trips the "own subtree" guard.
+ *
+ * NOTE: this runs against real Postgres in CI (the integration suite). There is
+ * no local Postgres in dev, so it is not expected to run locally.
+ */
+describe('movePage concurrent opposing moves do not create a cycle [integration]', () => {
+  let db: Kysely<any>;
+  let workspaceId: string;
+  let spaceId: string;
+
+  beforeAll(async () => {
+    db = getTestDb();
+    workspaceId = (await createWorkspace(db)).id;
+    spaceId = (await createSpace(db, workspaceId)).id;
+  });
+
+  afterAll(async () => {
+    await destroyTestDb();
+  });
+
+  it('lets exactly one opposing move win and never persists a cycle', async () => {
+    // Real collaborators against the test DB. movePage only touches db-backed
+    // methods on pageRepo plus the db itself and the event emitter (stubbed).
+    const pageRepo = new PageRepo(
+      db as any,
+      {} as any,
+      { emit: () => undefined } as any,
+    );
+    const svc = new PageService(
+      pageRepo as any, // pageRepo
+      {} as any, // pagePermissionRepo
+      {} as any, // attachmentRepo
+      db as any, // db
+      {} as any, // storageService
+      {} as any, // attachmentQueue
+      {} as any, // aiQueue
+      {} as any, // generalQueue
+      { emit: () => undefined } as any, // eventEmitter
+      {} as any, // collaborationGateway
+      {} as any, // watcherService
+      {} as any, // transclusionService
+    );
+
+    // Seed R (root), X and Y as children of R.
+    const root = await createPage(db, { workspaceId, spaceId, title: 'R' });
+    const pageX = await createPage(db, { workspaceId, spaceId, title: 'X' });
+    const pageY = await createPage(db, { workspaceId, spaceId, title: 'Y' });
+
+    // createPage does not set parentPageId; wire X.parent=R and Y.parent=R and
+    // give each a valid fractional-index position.
+    await db
+      .updateTable('pages')
+      .set({ parentPageId: root.id, position: 'a0' })
+      .where('id', 'in', [pageX.id, pageY.id])
+      .execute();
+
+    // The Page snapshots movePage receives (parentPageId must be R so each move
+    // is a genuine re-parent rather than a same-parent reorder).
+    const movedX = await pageRepo.findById(pageX.id);
+    const movedY = await pageRepo.findById(pageY.id);
+
+    // Two opposing moves racing: A moves X under Y, B moves Y under X.
+    const results = await Promise.allSettled([
+      svc.movePage(
+        { pageId: pageX.id, parentPageId: pageY.id, position: 'a1' },
+        movedX,
+      ),
+      svc.movePage(
+        { pageId: pageY.id, parentPageId: pageX.id, position: 'a1' },
+        movedY,
+      ),
+    ]);
+
+    // Exactly one fulfilled and one rejected; the rejection is the cycle guard.
+    const fulfilled = results.filter((r) => r.status === 'fulfilled');
+    const rejected = results.filter((r) => r.status === 'rejected');
+    expect(fulfilled).toHaveLength(1);
+    expect(rejected).toHaveLength(1);
+
+    const reason = (rejected[0] as PromiseRejectedResult).reason;
+    expect(reason).toBeInstanceOf(BadRequestException);
+    expect(String(reason?.message)).toContain('own subtree');
+
+    // No cycle persisted: re-fetch X and Y and assert NOT (X->Y AND Y->X).
+    const xNow = await pageRepo.findById(pageX.id);
+    const yNow = await pageRepo.findById(pageY.id);
+    const isCycle =
+      xNow.parentPageId === pageY.id && yNow.parentPageId === pageX.id;
+    expect(isCycle).toBe(false);
+
+    // Exactly one re-parent took effect: one of X/Y still points at R, the other
+    // points at its sibling.
+    const xWon = xNow.parentPageId === pageY.id && yNow.parentPageId === root.id;
+    const yWon = yNow.parentPageId === pageX.id && xNow.parentPageId === root.id;
+    expect(xWon || yWon).toBe(true);
+  });
+});