diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index bb387b31..d5947f8d 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/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: } 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 diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index ff205350..3204ac6c 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/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 - // page within its own subtree. Without this, an MCP/REST/agent caller (or a - // 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'); - } + // Cheap self-move guard (no DB) — keep before the transaction. + if (dto.parentPageId && dto.parentPageId === dto.pageId) { + throw new BadRequestException('Cannot move a page into its own subtree'); } - const updateResult = await this.pageRepo.updatePage( - { - 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, - ); + 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'), + }; + + 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) { - const ancestors = await this.db + async getPageBreadCrumbs(childPageId: string, trx?: KyselyTransaction) { + const ancestors = await dbOrTx(this.db, trx) .withRecursive('page_ancestors', (db) => db .selectFrom('pages') diff --git a/apps/server/test/integration/page-move-cycle-concurrency.int-spec.ts b/apps/server/test/integration/page-move-cycle-concurrency.int-spec.ts new file mode 100644 index 00000000..2a0c57b7 --- /dev/null +++ b/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; + 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); + }); +});