fix: stop cyclic descendant traversal before side effects

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

@@ -133,7 +133,8 @@ export class PageService {
       ydoc = createYdocFromJson(prosemirrorJson);
     }
 
-    const page = await this.pageRepo.insertPage({
+    const page = await this.pageRepo.insertPage(
+      {
         slugId: generateSlugId(),
         title: createPageDto.title,
         position: await this.nextPagePosition(
@@ -150,7 +151,9 @@ export class PageService {
         content,
         textContent,
         ydoc,
-    }, trx);
+      },
+      trx,
+    );
 
     if (trx) {
       // Add the watcher inside the caller's transaction so the async worker
@@ -1034,19 +1037,29 @@ export class PageService {
       .withRecursive('page_descendants', (db) =>
         db
           .selectFrom('pages')
-          .select(['id'])
+          .select([
+            'id',
+            sql<string[]>`ARRAY[id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
+          ])
           .where('id', '=', pageId)
           .unionAll((exp) =>
             exp
               .selectFrom('pages as p')
-              .select(['p.id'])
+              .select([
-              .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'),
+                'p.id',
+                sql<string[]>`pd.traversal_path || p.id`.as('traversalPath'),
+                sql<boolean>`p.id = ANY(pd.traversal_path)`.as('isCycle'),
+              ])
+              .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId')
+              .where('pd.isCycle', '=', false),
           ),
       )
       .selectFrom('page_descendants')
-      .selectAll()
+      .select(['id', 'isCycle'])
       .execute();
 
+    assertAcyclicPageTraversal(descendants, pageId);
     const pageIds = descendants.map((d) => d.id);
 
     // Queue attachment deletion for all pages with unique job IDs to prevent duplicates

apps/server/src/core/page/services/trash-cleanup.service.ts

@@ -5,6 +5,8 @@ import { KyselyDB } from '@docmost/db/types/kysely.types';
 import { InjectQueue } from '@nestjs/bullmq';
 import { Queue } from 'bullmq';
 import { QueueJob, QueueName } from '../../../integrations/queue/constants';
+import { assertAcyclicPageTraversal } from '../../../database/helpers/page-hierarchy-cycle';
+import { sql } from 'kysely';
 
 const DEFAULT_RETENTION_DAYS = 30;
 
@@ -76,19 +78,29 @@ export class TrashCleanupService {
       .withRecursive('page_descendants', (db) =>
         db
           .selectFrom('pages')
-          .select(['id'])
+          .select([
+            'id',
+            sql<string[]>`ARRAY[id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
+          ])
           .where('id', '=', pageId)
           .unionAll((exp) =>
             exp
               .selectFrom('pages as p')
-              .select(['p.id'])
+              .select([
-              .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'),
+                'p.id',
+                sql<string[]>`pd.traversal_path || p.id`.as('traversalPath'),
+                sql<boolean>`p.id = ANY(pd.traversal_path)`.as('isCycle'),
+              ])
+              .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId')
+              .where('pd.isCycle', '=', false),
           ),
       )
       .selectFrom('page_descendants')
-      .selectAll()
+      .select(['id', 'isCycle'])
       .execute();
 
+    assertAcyclicPageTraversal(descendants, pageId);
     const pageIds = descendants.map((d) => d.id);
 
     this.logger.debug(

apps/server/src/database/repos/page/page.repo.ts

@@ -16,6 +16,10 @@ import { jsonArrayFrom, jsonObjectFrom } from 'kysely/helpers/postgres';
 import { SpaceMemberRepo } from '@docmost/db/repos/space/space-member.repo';
 import { EventEmitter2 } from '@nestjs/event-emitter';
 import { EventName } from '../../../common/events/event.contants';
+import {
+  assertAcyclicPageTraversal,
+  stripPageTraversalMetadata,
+} from '../../helpers/page-hierarchy-cycle';
 
 @Injectable()
 export class PageRepo {
@@ -203,21 +207,31 @@ export class PageRepo {
       .withRecursive('page_descendants', (db) =>
         db
           .selectFrom('pages')
-          .select(['id'])
+          .select([
+            'id',
+            sql<string[]>`ARRAY[id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
+          ])
           .where('id', '=', pageId)
           .where('deletedAt', 'is', null)
           .unionAll((exp) =>
             exp
               .selectFrom('pages as p')
-              .select(['p.id'])
+              .select([
+                'p.id',
+                sql<string[]>`pd.traversal_path || p.id`.as('traversalPath'),
+                sql<boolean>`p.id = ANY(pd.traversal_path)`.as('isCycle'),
+              ])
               .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId')
-              .where('p.deletedAt', 'is', null),
+              .where('p.deletedAt', 'is', null)
+              .where('pd.isCycle', '=', false),
           ),
       )
       .selectFrom('page_descendants')
-      .selectAll()
+      .select(['id', 'isCycle'])
       .execute();
 
+    assertAcyclicPageTraversal(descendants, pageId);
     const pageIds = descendants.map((d) => d.id);
 
     if (pageIds.length > 0) {
@@ -272,19 +286,29 @@ export class PageRepo {
       .withRecursive('page_descendants', (db) =>
         db
           .selectFrom('pages')
-          .select(['id'])
+          .select([
+            'id',
+            sql<string[]>`ARRAY[id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
+          ])
           .where('id', '=', pageId)
           .unionAll((exp) =>
             exp
               .selectFrom('pages as p')
-              .select(['p.id'])
+              .select([
-              .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId'),
+                'p.id',
+                sql<string[]>`pd.traversal_path || p.id`.as('traversalPath'),
+                sql<boolean>`p.id = ANY(pd.traversal_path)`.as('isCycle'),
+              ])
+              .innerJoin('page_descendants as pd', 'pd.id', 'p.parentPageId')
+              .where('pd.isCycle', '=', false),
           ),
       )
       .selectFrom('page_descendants')
-      .selectAll()
+      .select(['id', 'isCycle'])
       .execute();
 
+    assertAcyclicPageTraversal(pages, pageId);
     const pageIds = pages.map((p) => p.id);
 
     // Restore all pages, but only detach the root page if its parent is deleted
@@ -354,7 +378,12 @@ export class PageRepo {
     });
   }
 
-  async getCreatedByPages(creatorId: string, requestingUserId: string, pagination: PaginationOptions, spaceId?: string) {
+  async getCreatedByPages(
+    creatorId: string,
+    requestingUserId: string,
+    pagination: PaginationOptions,
+    spaceId?: string,
+  ) {
     let query = this.db
       .selectFrom('pages')
       .select(this.baseFields)
@@ -365,7 +394,11 @@ export class PageRepo {
     if (spaceId) {
       query = query.where('spaceId', '=', spaceId);
     } else {
-      query = query.where('spaceId', 'in', this.spaceMemberRepo.getUserSpaceIdsQuery(requestingUserId));
+      query = query.where(
+        'spaceId',
+        'in',
+        this.spaceMemberRepo.getUserSpaceIdsQuery(requestingUserId),
+      );
     }
 
     return executeWithCursorPagination(query, {
@@ -491,7 +524,7 @@ export class PageRepo {
     parentPageId: string,
     opts: { includeContent: boolean },
   ) {
-    return this.db
+    const pages = await this.db
       .withRecursive('page_hierarchy', (db) =>
         db
           .selectFrom('pages')
@@ -506,6 +539,8 @@ export class PageRepo {
             'workspaceId',
             'createdAt',
             'updatedAt',
+            sql<string[]>`ARRAY[id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
           ])
           .$if(opts?.includeContent, (qb) => qb.select('content'))
           .where('id', '=', parentPageId)
@@ -524,15 +559,34 @@ export class PageRepo {
                 'p.workspaceId',
                 'p.createdAt',
                 'p.updatedAt',
+                sql<string[]>`ph.traversal_path || p.id`.as('traversalPath'),
+                sql<boolean>`p.id = ANY(ph.traversal_path)`.as('isCycle'),
               ])
               .$if(opts?.includeContent, (qb) => qb.select('p.content'))
               .innerJoin('page_hierarchy as ph', 'p.parentPageId', 'ph.id')
-              .where('p.deletedAt', 'is', null),
+              .where('p.deletedAt', 'is', null)
+              .where('ph.isCycle', '=', false),
           ),
       )
       .selectFrom('page_hierarchy')
-      .selectAll()
+      .select([
+        'id',
+        'slugId',
+        'title',
+        'icon',
+        'position',
+        'parentPageId',
+        'spaceId',
+        'workspaceId',
+        'createdAt',
+        'updatedAt',
+        'isCycle',
+      ])
+      .$if(opts?.includeContent, (qb) => qb.select('content'))
       .execute();
+
+    assertAcyclicPageTraversal(pages, parentPageId);
+    return pages.map((page) => stripPageTraversalMetadata(page));
   }
 
   /**
@@ -540,14 +594,13 @@ export class PageRepo {
    * More efficient than getPageAndDescendants + filtering because:
    * 1. Single DB query (no separate restricted IDs query)
    * 2. Stops traversing at restricted pages (doesn't fetch data to discard)
-   * 3. No in-memory filtering needed
+   * 3. Filters the bounded traversal only after hierarchy validation
    */
   async getPageAndDescendantsExcludingRestricted(
     parentPageId: string,
     opts: { includeContent: boolean },
   ) {
-    return (
+    const pages = await this.db
-      this.db
       .withRecursive('page_hierarchy', (db) =>
         db
           .selectFrom('pages')
@@ -562,6 +615,8 @@ export class PageRepo {
             'pages.spaceId',
             'pages.workspaceId',
             sql<boolean>`page_access.id IS NOT NULL`.as('isRestricted'),
+            sql<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
           ])
           .$if(opts?.includeContent, (qb) => qb.select('pages.content'))
           .where('pages.id', '=', parentPageId)
@@ -581,11 +636,14 @@ export class PageRepo {
                 'p.spaceId',
                 'p.workspaceId',
                 sql<boolean>`page_access.id IS NOT NULL`.as('isRestricted'),
+                sql<string[]>`ph.traversal_path || p.id`.as('traversalPath'),
+                sql<boolean>`p.id = ANY(ph.traversal_path)`.as('isCycle'),
               ])
               .$if(opts?.includeContent, (qb) => qb.select('p.content'))
               .where('p.deletedAt', 'is', null)
               // Only recurse into children of non-restricted pages
-                .where('ph.isRestricted', '=', false),
+              .where('ph.isRestricted', '=', false)
+              .where('ph.isCycle', '=', false),
           ),
       )
       .selectFrom('page_hierarchy')
@@ -598,11 +656,20 @@ export class PageRepo {
         'parentPageId',
         'spaceId',
         'workspaceId',
+        'isRestricted',
+        'isCycle',
       ])
       .$if(opts?.includeContent, (qb) => qb.select('content'))
-        // Filter out restricted pages from the result
+      .execute();
-        .where('isRestricted', '=', false)
+
-        .execute()
+    assertAcyclicPageTraversal(pages, parentPageId);
-    );
+    return pages
+      .filter((page) => !page.isRestricted)
+      .map((page) => {
+        const withoutCycleMetadata = stripPageTraversalMetadata(page);
+        const { isRestricted: _isRestricted, ...publicPage } =
+          withoutCycleMetadata;
+        return publicPage;
+      });
   }
 }

apps/server/test/page-hierarchy-cycle.integration-spec.ts

@@ -1,9 +1,12 @@
 import { randomUUID } from 'node:crypto';
+import { Logger } from '@nestjs/common';
 import { createCache } from 'cache-manager';
 import { PageService } from '../src/core/page/services/page.service';
+import { TrashCleanupService } from '../src/core/page/services/trash-cleanup.service';
 import { ShareService } from '../src/core/share/share.service';
 import { PageHierarchyCycleError } from '../src/database/helpers/page-hierarchy-cycle';
 import { PagePermissionRepo } from '../src/database/repos/page/page-permission.repo';
+import { PageRepo } from '../src/database/repos/page/page.repo';
 import { KyselyDB } from '../src/database/types/kysely.types';
 import { db, withStatementTimeout } from './support/database';
 import {
@@ -13,23 +16,36 @@ import {
   seedTwoPageCycle,
 } from './support/page-hierarchy-fixtures';
 
-function createPageService(connection: KyselyDB): PageService {
+function createPageService(
+  connection: KyselyDB,
+  dependencies: {
+    attachmentQueue?: { add: jest.Mock };
+    eventEmitter?: { emit: jest.Mock };
+  } = {},
+): PageService {
   return new PageService(
     undefined as never,
     undefined as never,
     undefined as never,
     connection,
     undefined as never,
+    dependencies.attachmentQueue as never,
     undefined as never,
     undefined as never,
-    undefined as never,
+    dependencies.eventEmitter as never,
-    undefined as never,
     undefined as never,
     undefined as never,
     undefined as never,
   );
 }
 
+function createPageRepo(
+  connection: KyselyDB,
+  eventEmitter: { emit: jest.Mock } = { emit: jest.fn() },
+): PageRepo {
+  return new PageRepo(connection, undefined as never, eventEmitter as never);
+}
+
 function createShareService(connection: KyselyDB): ShareService {
   return new ShareService(
     undefined as never,
@@ -167,7 +183,251 @@ async function insertShare(pageId: string, includeSubPages: boolean) {
     .executeTakeFirstOrThrow();
 }
 
+async function expectPageHierarchyCycle(
+  operation: Promise<unknown>,
+  rootPageId: string,
+): Promise<void> {
+  const error = await operation.then(
+    () => undefined,
+    (reason: unknown) => reason,
+  );
+
+  expect(error).toBeInstanceOf(PageHierarchyCycleError);
+  expect(error).toEqual(
+    expect.objectContaining({
+      code: 'PAGE_HIERARCHY_CYCLE',
+      rootPageId,
+    }) satisfies Partial<PageHierarchyCycleError>,
+  );
+}
+
+afterEach(() => {
+  jest.restoreAllMocks();
+});
+
 describe('cycle-safe page hierarchy reads', () => {
+  describe('descendant traversal', () => {
+    it('returns every page in an acyclic branching tree exactly once without internal metadata', async () => {
+      const { root, firstChild, secondChild, grandchild } =
+        await seedBranchingDescendantTree();
+      const repo = createPageRepo(db);
+
+      const pages = await repo.getPageAndDescendants(root.id, {
+        includeContent: false,
+      });
+
+      expect(pages.map((page) => page.id).sort()).toEqual(
+        [root.id, firstChild.id, secondChild.id, grandchild.id].sort(),
+      );
+      expect(pages).toHaveLength(4);
+      for (const page of pages) {
+        expect(page).not.toHaveProperty('isCycle');
+        expect(page).not.toHaveProperty('traversalPath');
+      }
+    });
+
+    describe.each([
+      ['a self-cycle', async () => (await seedSelfCycle()).self.id],
+      ['a two-page cycle', async () => (await seedTwoPageCycle()).a.id],
+    ])('%s', (_cycleName, seedCycle) => {
+      it('raises PageHierarchyCycleError from the structural descendant read', async () => {
+        const pageId = await seedCycle();
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPageRepo(connection);
+
+          await expectPageHierarchyCycle(
+            repo.getPageAndDescendants(pageId, { includeContent: false }),
+            pageId,
+          );
+        });
+      });
+
+      it('raises PageHierarchyCycleError from the restricted descendant read', async () => {
+        const pageId = await seedCycle();
+
+        await withStatementTimeout(async (connection) => {
+          const repo = createPageRepo(connection);
+
+          await expectPageHierarchyCycle(
+            repo.getPageAndDescendantsExcludingRestricted(pageId, {
+              includeContent: false,
+            }),
+            pageId,
+          );
+        });
+      });
+    });
+
+    it('preserves restricted-subtree exclusion for an acyclic tree', async () => {
+      const { root, firstChild, secondChild, grandchild } =
+        await seedBranchingDescendantTree();
+      await restrictPage(firstChild.id);
+      const repo = createPageRepo(db);
+
+      const pages = await repo.getPageAndDescendantsExcludingRestricted(
+        root.id,
+        { includeContent: false },
+      );
+
+      expect(pages.map((page) => page.id).sort()).toEqual(
+        [root.id, secondChild.id].sort(),
+      );
+      expect(pages.map((page) => page.id)).not.toContain(firstChild.id);
+      expect(pages.map((page) => page.id)).not.toContain(grandchild.id);
+      for (const page of pages) {
+        expect(page).not.toHaveProperty('isCycle');
+        expect(page).not.toHaveProperty('isRestricted');
+        expect(page).not.toHaveProperty('traversalPath');
+      }
+    });
+
+    it('removePage leaves pages and shares unchanged and emits no event on a cycle', async () => {
+      const { a, b } = await seedTwoPageCycle();
+      const deletedById = await insertTestUser(a.id);
+      const share = await insertShare(b.id, true);
+      const eventEmitter = { emit: jest.fn() };
+
+      await withStatementTimeout(async (connection) => {
+        const repo = createPageRepo(connection, eventEmitter);
+
+        await expectPageHierarchyCycle(
+          repo.removePage(a.id, deletedById, share.workspaceId),
+          a.id,
+        );
+      });
+
+      const storedPages = await db
+        .selectFrom('pages')
+        .select(['id', 'deletedAt', 'deletedById'])
+        .where('id', 'in', [a.id, b.id])
+        .orderBy('id')
+        .execute();
+      const storedShare = await db
+        .selectFrom('shares')
+        .select('id')
+        .where('id', '=', share.id)
+        .executeTakeFirst();
+
+      expect(storedPages).toEqual(
+        [a.id, b.id]
+          .sort()
+          .map((id) => ({ id, deletedAt: null, deletedById: null })),
+      );
+      expect(storedShare).toEqual({ id: share.id });
+      expect(eventEmitter.emit).not.toHaveBeenCalled();
+    });
+
+    it('restorePage leaves deleted pages unchanged and emits no event on a cycle', async () => {
+      const { a, b } = await seedTwoPageCycle();
+      const deletedAt = new Date('2024-01-02T03:04:05.000Z');
+      await db
+        .updateTable('pages')
+        .set({ deletedAt })
+        .where('id', 'in', [a.id, b.id])
+        .execute();
+      const eventEmitter = { emit: jest.fn() };
+
+      await withStatementTimeout(async (connection) => {
+        const repo = createPageRepo(connection, eventEmitter);
+
+        await expectPageHierarchyCycle(
+          repo.restorePage(a.id, randomUUID()),
+          a.id,
+        );
+      });
+
+      const storedPages = await db
+        .selectFrom('pages')
+        .select(['id', 'parentPageId', 'deletedAt'])
+        .where('id', 'in', [a.id, b.id])
+        .orderBy('id')
+        .execute();
+
+      expect(storedPages).toEqual(
+        [
+          { id: a.id, parentPageId: b.id, deletedAt },
+          { id: b.id, parentPageId: a.id, deletedAt },
+        ].sort((left, right) => left.id.localeCompare(right.id)),
+      );
+      expect(eventEmitter.emit).not.toHaveBeenCalled();
+    });
+
+    it('forceDelete leaves pages intact and enqueues and emits nothing on a cycle', async () => {
+      const { a, b } = await seedTwoPageCycle();
+      const attachmentQueue = { add: jest.fn() };
+      const eventEmitter = { emit: jest.fn() };
+
+      await withStatementTimeout(async (connection) => {
+        const pageService = createPageService(connection, {
+          attachmentQueue,
+          eventEmitter,
+        });
+
+        await expectPageHierarchyCycle(
+          pageService.forceDelete(a.id, randomUUID()),
+          a.id,
+        );
+      });
+
+      const storedPageIds = await db
+        .selectFrom('pages')
+        .select('id')
+        .where('id', 'in', [a.id, b.id])
+        .orderBy('id')
+        .execute();
+
+      expect(storedPageIds).toEqual([a.id, b.id].sort().map((id) => ({ id })));
+      expect(attachmentQueue.add).not.toHaveBeenCalled();
+      expect(eventEmitter.emit).not.toHaveBeenCalled();
+    });
+
+    it('trash cleanup logs and skips a corrupt root before continuing with acyclic trash', async () => {
+      const { self } = await seedSelfCycle();
+      const { root, child, grandchild } = await seedAcyclicPageChain();
+      const expiredAt = new Date('2024-01-02T03:04:05.000Z');
+      await db
+        .updateTable('pages')
+        .set({ deletedAt: expiredAt })
+        .where('id', 'in', [self.id, root.id])
+        .execute();
+      const attachmentQueue = { add: jest.fn() };
+      const loggerError = jest
+        .spyOn(Logger.prototype, 'error')
+        .mockImplementation();
+
+      await withStatementTimeout(async (connection) => {
+        const timeoutCleanupService = new TrashCleanupService(
+          connection,
+          attachmentQueue as never,
+        );
+        await timeoutCleanupService.cleanupOldTrash();
+      });
+
+      const corruptPage = await db
+        .selectFrom('pages')
+        .select(['id', 'deletedAt'])
+        .where('id', '=', self.id)
+        .executeTakeFirst();
+      const cleanedPageIds = await db
+        .selectFrom('pages')
+        .select('id')
+        .where('id', 'in', [root.id, child.id, grandchild.id])
+        .execute();
+
+      expect(corruptPage).toEqual({ id: self.id, deletedAt: expiredAt });
+      expect(cleanedPageIds).toEqual([]);
+      expect(attachmentQueue.add).toHaveBeenCalledTimes(3);
+      expect(
+        attachmentQueue.add.mock.calls.map(([, payload]) => payload.pageId),
+      ).toEqual(expect.arrayContaining([root.id, child.id, grandchild.id]));
+      expect(loggerError).toHaveBeenCalledWith(
+        expect.stringContaining(`Failed to cleanup page ${self.id}`),
+        expect.any(String),
+      );
+    });
+  });
+
   describe('PageService.getPageBreadCrumbs', () => {
     it('returns every acyclic breadcrumb exactly once in root-to-child order', async () => {
       const { root, child, grandchild } = await seedAcyclicPageChain();