diff --git a/apps/server/src/core/page/services/trash-cleanup.service.ts b/apps/server/src/core/page/services/trash-cleanup.service.ts index aa5ff6d1e..9c09d5f00 100644 --- a/apps/server/src/core/page/services/trash-cleanup.service.ts +++ b/apps/server/src/core/page/services/trash-cleanup.service.ts @@ -33,15 +33,8 @@ export class TrashCleanupService { .where('deletedAt', 'is', null) .execute(); - const cleanupCandidates: Array<{ - pageId: string; - pageIds: string[]; - }> = []; - const excludedPageIds = new Set(); + let totalCleaned = 0; - // Preflight every candidate before any cleanup mutates the snapshot. - // This lets a corrupt root exclude its whole reachable component even - // when one of its descendants is also an independently expired page. for (const workspace of workspaces) { const retentionDays = workspace.trashRetentionDays ?? DEFAULT_RETENTION_DAYS; @@ -54,53 +47,39 @@ export class TrashCleanupService { .select(['id']) .where('workspaceId', '=', workspace.id) .where('deletedAt', '<', retentionDate) + .orderBy('id') .execute(); for (const page of oldDeletedPages) { - let descendants: Array<{ id: string; isCycle: boolean }> | undefined; + let pageIds: string[]; try { - descendants = await this.getPageDescendants(page.id); + const ancestors = await this.getPageAncestors(page.id); + assertAcyclicPageTraversal(ancestors, page.id); + + const descendants = await this.getPageDescendants(page.id); assertAcyclicPageTraversal(descendants, page.id); - cleanupCandidates.push({ - pageId: page.id, - pageIds: descendants.map((descendant) => descendant.id), - }); + pageIds = descendants.map((descendant) => descendant.id); } catch (error) { if (error instanceof PageHierarchyCycleError) { - for (const descendant of descendants ?? []) { - excludedPageIds.add(descendant.id); - } + this.logCleanupError(page.id, error); + continue; } + throw error; + } + + if (pageIds.length === 0) { + continue; + } + + try { + totalCleaned += await this.cleanupPage(page.id, pageIds); + } catch (error) { this.logCleanupError(page.id, error); } } } - let totalCleaned = 0; - const cleanedPageIds = new Set(); - - for (const candidate of cleanupCandidates) { - const pageIds = candidate.pageIds.filter( - (pageId) => - !excludedPageIds.has(pageId) && !cleanedPageIds.has(pageId), - ); - - if (pageIds.length === 0) { - continue; - } - - try { - await this.cleanupPage(candidate.pageId, pageIds); - for (const pageId of pageIds) { - cleanedPageIds.add(pageId); - } - totalCleaned++; - } catch (error) { - this.logCleanupError(candidate.pageId, error); - } - } - this.logger.debug( totalCleaned > 0 ? `Trash cleanup completed: ${totalCleaned} pages cleaned` @@ -114,6 +93,36 @@ export class TrashCleanupService { } } + private async getPageAncestors(pageId: string) { + return this.db + .withRecursive('page_ancestors', (db) => + db + .selectFrom('pages') + .select([ + 'id', + 'parentPageId', + sql`ARRAY[id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) + .where('id', '=', pageId) + .unionAll((exp) => + exp + .selectFrom('pages as p') + .select([ + 'p.id', + 'p.parentPageId', + sql`pa.traversal_path || p.id`.as('traversalPath'), + sql`p.id = ANY(pa.traversal_path)`.as('isCycle'), + ]) + .innerJoin('page_ancestors as pa', 'pa.parentPageId', 'p.id') + .where('pa.isCycle', '=', false), + ), + ) + .selectFrom('page_ancestors') + .select(['id', 'isCycle']) + .execute(); + } + private async getPageDescendants(pageId: string) { // Get all descendants using recursive CTE (including the page itself) return this.db @@ -167,14 +176,17 @@ export class TrashCleanupService { } try { - if (pageIds.length > 0) { - await this.db.deleteFrom('pages').where('id', 'in', pageIds).execute(); - } + const result = await this.db + .deleteFrom('pages') + .where('id', 'in', pageIds) + .executeTakeFirst(); + return Number(result.numDeletedRows); } catch (error) { // Log but don't throw - pages might have been deleted by another node this.logger.warn( `Error deleting pages, they may have been already deleted: ${error instanceof Error ? error.message : 'Unknown error'}`, ); + return 0; } } diff --git a/apps/server/test/page-hierarchy-cycle.integration-spec.ts b/apps/server/test/page-hierarchy-cycle.integration-spec.ts index 8584d5c25..0f28e3506 100644 --- a/apps/server/test/page-hierarchy-cycle.integration-spec.ts +++ b/apps/server/test/page-hierarchy-cycle.integration-spec.ts @@ -1,6 +1,7 @@ import { randomUUID } from 'node:crypto'; import { Logger } from '@nestjs/common'; import { createCache } from 'cache-manager'; +import { sql } from 'kysely'; 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'; @@ -392,6 +393,7 @@ describe('cycle-safe page hierarchy reads', () => { const corruptSideChild = await db .insertInto('pages') .values({ + id: 'dddddddd-dddd-4ddd-bddd-dddddddddddd', parentPageId: self.id, slugId: randomUUID(), spaceId: context.spaceId, @@ -403,6 +405,7 @@ describe('cycle-safe page hierarchy reads', () => { const healthyRoot = await db .insertInto('pages') .values({ + id: 'eeeeeeee-eeee-4eee-beee-eeeeeeeeeeee', slugId: randomUUID(), spaceId: context.spaceId, title: 'Healthy cleanup root', @@ -413,6 +416,7 @@ describe('cycle-safe page hierarchy reads', () => { const healthyChild = await db .insertInto('pages') .values({ + id: 'ffffffff-ffff-4fff-bfff-ffffffffffff', parentPageId: healthyRoot.id, slugId: randomUUID(), spaceId: context.spaceId, @@ -478,17 +482,123 @@ describe('cycle-safe page hierarchy reads', () => { expect(queuedPageIds).not.toEqual( expect.arrayContaining([self.id, corruptSideChild.id]), ); - expect(loggerError).toHaveBeenCalledTimes(1); + expect(loggerError).toHaveBeenCalledTimes(2); expect(loggerError).toHaveBeenCalledWith( `Failed to cleanup page ${self.id}: Cyclic page hierarchy detected`, expect.stringContaining( 'PageHierarchyCycleError: Cyclic page hierarchy detected', ), ); - expect(loggerError.mock.invocationCallOrder[0]).toBeLessThan( + expect(loggerError).toHaveBeenCalledWith( + `Failed to cleanup page ${corruptSideChild.id}: Cyclic page hierarchy detected`, + expect.stringContaining( + 'PageHierarchyCycleError: Cyclic page hierarchy detected', + ), + ); + expect(Math.max(...loggerError.mock.invocationCallOrder)).toBeLessThan( attachmentQueue.add.mock.invocationCallOrder[0], ); }); + + it('trash cleanup aborts before later mutation when hierarchy validation unexpectedly fails', async () => { + const { root } = await seedAcyclicPageChain(); + const context = await db + .selectFrom('pages') + .select(['spaceId', 'workspaceId']) + .where('id', '=', root.id) + .executeTakeFirstOrThrow(); + const expiredAt = new Date('2024-01-02T03:04:05.000Z'); + const slowRootId = '00000000-0000-4000-8000-000000000001'; + const firstHealthyRootId = 'eeeeeeee-eeee-4eee-beee-eeeeeeeeeeee'; + const secondHealthyRootId = 'ffffffff-ffff-4fff-bfff-ffffffffffff'; + await db + .insertInto('pages') + .values([ + { + deletedAt: expiredAt, + id: slowRootId, + slugId: randomUUID(), + spaceId: context.spaceId, + title: 'Slow validation root', + workspaceId: context.workspaceId, + }, + { + deletedAt: expiredAt, + id: firstHealthyRootId, + slugId: randomUUID(), + spaceId: context.spaceId, + title: 'First later healthy root', + workspaceId: context.workspaceId, + }, + { + deletedAt: expiredAt, + id: secondHealthyRootId, + slugId: randomUUID(), + spaceId: context.spaceId, + title: 'Second later healthy root', + workspaceId: context.workspaceId, + }, + ]) + .execute(); + const chainSeed = randomUUID(); + await sql` + INSERT INTO pages ( + id, + slug_id, + title, + parent_page_id, + space_id, + workspace_id + ) + SELECT + md5(${chainSeed} || '-page-' || step::text)::uuid, + md5(${chainSeed} || '-slug-' || step::text)::uuid, + 'Slow validation descendant ' || step::text, + CASE + WHEN step = 1 THEN ${slowRootId}::uuid + ELSE md5(${chainSeed} || '-page-' || (step - 1)::text)::uuid + END, + ${context.spaceId}::uuid, + ${context.workspaceId}::uuid + FROM generate_series(1, 10000) AS step + `.execute(db); + const attachmentQueue = { add: jest.fn() }; + const loggerError = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(); + + await withStatementTimeout(async (connection) => { + await sql`SET statement_timeout = '25ms'`.execute(connection); + const cleanupService = new TrashCleanupService( + connection, + attachmentQueue as never, + ); + + await cleanupService.cleanupOldTrash(); + }); + + const storedExpiredIds = await db + .selectFrom('pages') + .select('id') + .where('id', 'in', [ + slowRootId, + firstHealthyRootId, + secondHealthyRootId, + ]) + .orderBy('id') + .execute(); + + expect(storedExpiredIds).toEqual([ + { id: slowRootId }, + { id: firstHealthyRootId }, + { id: secondHealthyRootId }, + ]); + expect(attachmentQueue.add).not.toHaveBeenCalled(); + expect(loggerError).toHaveBeenCalledWith( + 'Trash cleanup job failed', + expect.stringContaining('canceling statement due to statement timeout'), + ); + }); }); describe('PageService.getPageBreadCrumbs', () => {