diff --git a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts index 0e4442decfa..076977e13c5 100644 --- a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts @@ -207,6 +207,73 @@ describe('WorkflowReviewRequestRepository', () => { }); }); + describe('findOpenRequestsForWorkflows', () => { + let queryBuilder: Mocked>; + + beforeEach(() => { + queryBuilder = mock>(); + queryBuilder.innerJoin.mockReturnThis(); + queryBuilder.addSelect.mockReturnThis(); + queryBuilder.where.mockReturnThis(); + queryBuilder.andWhere.mockReturnThis(); + queryBuilder.getRawAndEntities.mockResolvedValue({ entities: [], raw: [] }); + (entityManager.createQueryBuilder as Mock).mockReturnValue(queryBuilder); + }); + + it('returns an empty list without querying when no workflow ids are given', async () => { + const result = await repo.findOpenRequestsForWorkflows([], {}); + + expect(result).toEqual([]); + expect(entityManager.createQueryBuilder).not.toHaveBeenCalled(); + }); + + it('scopes to the given workflows and to open requests only', async () => { + await repo.findOpenRequestsForWorkflows(['workflow-1', 'workflow-2'], {}); + + expect(queryBuilder.where).toHaveBeenCalledWith( + 'requestWorkflow.workflowId IN (:...workflowIds)', + { workflowIds: ['workflow-1', 'workflow-2'] }, + ); + expect(queryBuilder.andWhere).toHaveBeenCalledWith('request.state = :state', { + state: 'open', + }); + }); + + it('maps each request to the linked workflows it was matched by', async () => { + queryBuilder.getRawAndEntities.mockResolvedValue({ + entities: [ + mock({ id: 'req-1' }), + mock({ id: 'req-2' }), + ], + raw: [ + { request_id: 'req-1', linkedWorkflowId: 'workflow-1' }, + { request_id: 'req-1', linkedWorkflowId: 'workflow-2' }, + { request_id: 'req-2', linkedWorkflowId: 'workflow-2' }, + ], + }); + + const result = await repo.findOpenRequestsForWorkflows(['workflow-1', 'workflow-2'], {}); + + expect(result).toHaveLength(2); + expect(result[0]).toMatchObject({ workflowIds: ['workflow-1', 'workflow-2'] }); + expect(result[0].request.id).toBe('req-1'); + expect(result[1]).toMatchObject({ workflowIds: ['workflow-2'] }); + expect(result[1].request.id).toBe('req-2'); + }); + + it("reads through the context's transaction manager", async () => { + const transactionManager = mock(); + (transactionManager.createQueryBuilder as Mock).mockReturnValue(queryBuilder); + + await repo.findOpenRequestsForWorkflows(['workflow-1'], { + trx: new TypeOrmTransaction(transactionManager), + }); + + expect(transactionManager.createQueryBuilder).toHaveBeenCalled(); + expect(entityManager.createQueryBuilder).not.toHaveBeenCalled(); + }); + }); + describe('findById', () => { it("reads through the context's transaction manager", async () => { const transactionManager = mock(); @@ -238,7 +305,7 @@ describe('WorkflowReviewRequestRepository', () => { expect(result).toBe(rows); expect(repo.createQueryBuilder).toHaveBeenCalledWith('review'); - expect(queryBuilder.where).toHaveBeenCalledWith('review.createdById = :requesterId', { + expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.createdById = :requesterId', { requesterId: 'user-1', }); expect(queryBuilder.take).toHaveBeenCalledWith(15); @@ -257,7 +324,10 @@ describe('WorkflowReviewRequestRepository', () => { expect(result).toBe(rows); expect(repo.createQueryBuilder).toHaveBeenCalledWith('review'); - expect(queryBuilder.where).not.toHaveBeenCalled(); + expect(queryBuilder.andWhere).not.toHaveBeenCalledWith( + expect.stringContaining('review.createdById'), + expect.anything(), + ); expect(queryBuilder.orderBy).toHaveBeenCalledWith('review.createdAt', 'DESC'); expect(queryBuilder.addOrderBy).toHaveBeenCalledWith('review.id', 'ASC'); expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.state = :state', { @@ -279,7 +349,7 @@ describe('WorkflowReviewRequestRepository', () => { expect(result).toBe(rows); expect(repo.createQueryBuilder).toHaveBeenCalledWith('review'); - expect(queryBuilder.where).toHaveBeenCalledWith( + expect(queryBuilder.andWhere).toHaveBeenCalledWith( '(review.projectId IN (:...projectIds) OR review.createdById = :requesterId)', { projectIds: ['proj-1', 'proj-2'], requesterId: 'user-1' }, ); @@ -291,6 +361,17 @@ describe('WorkflowReviewRequestRepository', () => { expect(queryBuilder.take).toHaveBeenCalledWith(15); }); + it('excludes open requests whose link rows are gone, even at global scope', async () => { + queryBuilder.getMany.mockResolvedValueOnce([]); + + await repo.findManyForInbox({ projectIds: null, requesterId: 'user-1', limit: 15 }); + + // SQL shape (state != open OR EXISTS) is asserted by the inbox integration tests + expect(queryBuilder.andWhere).toHaveBeenCalledWith(expect.any(Function), { + openState: 'open', + }); + }); + it('applies the keyset boundary carried in the cursor without an anchor lookup', async () => { const findOneSpy = vi.spyOn(repo, 'findOne'); queryBuilder.getMany.mockResolvedValueOnce([]); @@ -322,7 +403,7 @@ describe('WorkflowReviewRequestRepository', () => { expect(queryBuilder.select).toHaveBeenCalledWith('review.state', 'state'); expect(queryBuilder.addSelect).toHaveBeenCalledWith('COUNT(*)', 'count'); expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state'); - expect(queryBuilder.where).toHaveBeenCalledWith('review.createdById = :requesterId', { + expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.createdById = :requesterId', { requesterId: 'user-1', }); }); @@ -336,7 +417,10 @@ describe('WorkflowReviewRequestRepository', () => { const result = await repo.countByStateForInbox({ projectIds: null, requesterId: 'user-1' }); expect(result).toEqual({ open: 3, closed: 12 }); - expect(queryBuilder.where).not.toHaveBeenCalled(); + expect(queryBuilder.andWhere).not.toHaveBeenCalledWith( + expect.stringContaining('review.createdById'), + expect.anything(), + ); expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state'); }); @@ -352,12 +436,22 @@ describe('WorkflowReviewRequestRepository', () => { }); expect(result).toEqual({ open: 1, closed: 4 }); - expect(queryBuilder.where).toHaveBeenCalledWith( + expect(queryBuilder.andWhere).toHaveBeenCalledWith( '(review.projectId IN (:...projectIds) OR review.createdById = :requesterId)', { projectIds: ['proj-1', 'proj-2'], requesterId: 'user-1' }, ); }); + it('excludes open requests whose link rows are gone, even at global scope', async () => { + queryBuilder.getRawMany.mockResolvedValueOnce([]); + + await repo.countByStateForInbox({ projectIds: null, requesterId: 'user-1' }); + + expect(queryBuilder.andWhere).toHaveBeenCalledWith(expect.any(Function), { + openState: 'open', + }); + }); + it('defaults absent states to zero', async () => { queryBuilder.getRawMany.mockResolvedValueOnce([]); diff --git a/packages/@n8n/db/src/repositories/shared-workflow.repository.ts b/packages/@n8n/db/src/repositories/shared-workflow.repository.ts index a2b6d406dee..dd4488fe911 100644 --- a/packages/@n8n/db/src/repositories/shared-workflow.repository.ts +++ b/packages/@n8n/db/src/repositories/shared-workflow.repository.ts @@ -5,7 +5,7 @@ import { type Scope, type WorkflowSharingRole, } from '@n8n/permissions'; -import { DataSource, Repository, In, Not } from '@n8n/typeorm'; +import { DataSource, In, Not } from '@n8n/typeorm'; import type { EntityManager, FindManyOptions, @@ -13,11 +13,13 @@ import type { SelectQueryBuilder, } from '@n8n/typeorm'; +import { BaseRepository } from './base-repository'; import type { User } from '../entities'; import { Project, ProjectRelation, SharedWorkflow } from '../entities'; +import type { OperationContext } from '../services/transaction'; @Service() -export class SharedWorkflowRepository extends Repository { +export class SharedWorkflowRepository extends BaseRepository { constructor(dataSource: DataSource) { super(SharedWorkflow, dataSource.manager); } @@ -136,9 +138,14 @@ export class SharedWorkflowRepository extends Repository { return [...new Set(projectIds)]; } - async getWorkflowOwningProject(workflowId: string) { + /** + * Pass `ctx` when calling from inside a transaction — the read then runs on that + * transaction's connection instead of checking out a second one, which would + * deadlock a single-connection pool. + */ + async getWorkflowOwningProject(workflowId: string, ctx: OperationContext = {}) { return ( - await this.findOne({ + await this.managerFor(ctx).findOne(SharedWorkflow, { where: { workflowId, role: 'workflow:owner' }, relations: { project: true }, }) diff --git a/packages/@n8n/db/src/repositories/workflow-history.repository.ts b/packages/@n8n/db/src/repositories/workflow-history.repository.ts index b36eba486bb..65d0f9f4d9e 100644 --- a/packages/@n8n/db/src/repositories/workflow-history.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-history.repository.ts @@ -5,6 +5,8 @@ import { DiffMetaData, DiffRule, groupWorkflows, SKIP_RULES } from 'n8n-workflow import { WorkflowHistory, WorkflowEntity, WorkflowPublishedVersion } from '../entities'; import { BaseRepository } from './base-repository'; import { WorkflowPublishHistoryRepository } from './workflow-publish-history.repository'; +import { WorkflowReviewRequestWorkflow } from '../entities/workflow-review-request-workflow.ee'; +import { WorkflowReviewRequest } from '../entities/workflow-review-request.ee'; import type { OperationContext } from '../services/transaction'; @Service() @@ -73,6 +75,18 @@ export class WorkflowHistoryRepository extends BaseRepository { .from(WorkflowPublishedVersion, 'wpv') .getQuery(); + // Versions pinned by an open review request must stay reviewable and + // publishable-on-approval. Closed reviews don't need it. + const openReviewPinnedVersionIdsSubquery = this.manager + .createQueryBuilder() + .subQuery() + .select('wrrw.workflowVersionId') + .from(WorkflowReviewRequestWorkflow, 'wrrw') + .innerJoin(WorkflowReviewRequest, 'wrr', 'wrr.id = wrrw.workflowReviewRequestId') + .where("wrr.state = 'open'") + .andWhere('wrrw.workflowVersionId IS NOT NULL') + .getQuery(); + const query = this.manager .createQueryBuilder() .delete() @@ -80,7 +94,8 @@ export class WorkflowHistoryRepository extends BaseRepository { .where('createdAt < :date', { date }) .andWhere(`versionId NOT IN (${currentVersionIdsSubquery})`) .andWhere(`versionId NOT IN (${activeVersionIdsSubquery})`) - .andWhere(`versionId NOT IN (${publishedVersionIdsSubquery})`); + .andWhere(`versionId NOT IN (${publishedVersionIdsSubquery})`) + .andWhere(`versionId NOT IN (${openReviewPinnedVersionIdsSubquery})`); if (preserveNamedVersions) { query.andWhere('name IS NULL'); diff --git a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts index 04fdd27bc59..b559911b68b 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts @@ -41,14 +41,6 @@ export type WorkflowReviewRequestForWorkflowRow = Pick< workflowVersionId: string | null; }; -export type ExistsAnyForInboxOptions = { - /** `null` means all projects (no filter); `[]` means no publish-scoped projects. */ - projectIds: string[] | null; - /** Requesters always see the reviews they created, regardless of project scope. */ - requesterId: string; - state?: WorkflowReviewRequestState; -}; - export type CountByStateForInboxOptions = { /** `null` means all projects (no filter); `[]` means no publish-scoped projects. */ projectIds: string[] | null; @@ -107,6 +99,47 @@ export class WorkflowReviewRequestRepository extends BaseRepository { + const openState: WorkflowReviewRequestState = 'open'; + const closedState: WorkflowReviewRequestState = 'closed'; + const manager = this.managerFor(ctx); + + const orphans = await manager + .createQueryBuilder(WorkflowReviewRequest, 'review') + .select('review.id', 'id') + .where('review.state = :openState', { openState }) + .andWhere((qb) => { + const linkedWorkflowExists = qb + .subQuery() + .select('1') + .from(WorkflowReviewRequestWorkflow, 'requestWorkflow') + .where('requestWorkflow.workflowReviewRequestId = review.id') + .getQuery(); + return `NOT EXISTS ${linkedWorkflowExists}`; + }) + .getRawMany<{ id: string }>(); + + if (orphans.length === 0) return []; + + const ids = orphans.map(({ id }) => id); + // A system close has no closing user; the decision stays as-is. + await manager.update(WorkflowReviewRequest, ids, { + state: closedState, + closedById: null, + }); + + return ids; + } + async findById(id: string, ctx: OperationContext): Promise { return await this.managerFor(ctx).findOne(WorkflowReviewRequest, { where: { id } }); } @@ -182,6 +215,45 @@ export class WorkflowReviewRequestRepository extends BaseRepository> { + if (workflowIds.length === 0) return []; + + const state: WorkflowReviewRequestState = 'open'; + + const { entities, raw } = await this.managerFor(ctx) + .createQueryBuilder(WorkflowReviewRequest, 'request') + .innerJoin( + WorkflowReviewRequestWorkflow, + 'requestWorkflow', + 'requestWorkflow.workflowReviewRequestId = request.id', + ) + .addSelect('requestWorkflow.workflowId', 'linkedWorkflowId') + .where('requestWorkflow.workflowId IN (:...workflowIds)', { workflowIds }) + .andWhere('request.state = :state', { state }) + .getRawAndEntities<{ request_id: string; linkedWorkflowId: string }>(); + + // Raw rows are per (request, workflow) pair; entities are deduplicated. + const workflowIdsByRequestId = new Map(); + for (const row of raw) { + const linked = workflowIdsByRequestId.get(row.request_id) ?? []; + linked.push(row.linkedWorkflowId); + workflowIdsByRequestId.set(row.request_id, linked); + } + + return entities.map((request) => ({ + request, + workflowIds: workflowIdsByRequestId.get(request.id) ?? [], + })); + } + async findManyForInbox(options: FindManyForInboxOptions): Promise { const { projectIds, requesterId, state, limit, cursor } = options; @@ -190,6 +262,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository): void { + const openState: WorkflowReviewRequestState = 'open'; + + queryBuilder.andWhere( + (qb) => { + const linkedWorkflowExists = qb + .subQuery() + .select('1') + .from(WorkflowReviewRequestWorkflow, 'requestWorkflow') + .where('requestWorkflow.workflowReviewRequestId = review.id') + .getQuery(); + return `(review.state != :openState OR EXISTS ${linkedWorkflowExists})`; + }, + { openState }, + ); + } } diff --git a/packages/@n8n/db/src/repositories/workflow.repository.ts b/packages/@n8n/db/src/repositories/workflow.repository.ts index bf5514c7a3a..cde9a38c065 100644 --- a/packages/@n8n/db/src/repositories/workflow.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow.repository.ts @@ -1,7 +1,7 @@ import { GlobalConfig } from '@n8n/config'; import { Service } from '@n8n/di'; import type { Scope } from '@n8n/permissions'; -import { DataSource, Repository, In, Like, Not, IsNull } from '@n8n/typeorm'; +import { DataSource, In, Like, Not, IsNull } from '@n8n/typeorm'; import type { SelectQueryBuilder, UpdateResult, @@ -13,6 +13,7 @@ import type { } from '@n8n/typeorm'; import { PROJECT_ROOT, UserError } from 'n8n-workflow'; +import { BaseRepository } from './base-repository'; import { FolderRepository } from './folder.repository'; import { SharedWorkflowRepository } from './shared-workflow.repository'; import { WorkflowHistoryRepository } from './workflow-history.repository'; @@ -30,6 +31,7 @@ import type { FolderWithWorkflowAndSubFolderCount, ListQuery, } from '../entities/types-db'; +import type { OperationContext } from '../services/transaction'; import { applyWorkflowBooleanSettingFilter } from '../utils/apply-workflow-boolean-setting-filter'; import { isStringArray } from '../utils/is-string-array'; import { TimedQuery } from '../utils/timed-query'; @@ -59,7 +61,7 @@ type WorkflowListResult = { }; @Service() -export class WorkflowRepository extends Repository { +export class WorkflowRepository extends BaseRepository { constructor( dataSource: DataSource, private readonly globalConfig: GlobalConfig, @@ -80,6 +82,23 @@ export class WorkflowRepository extends Repository { }); } + /** + * Archived state of a workflow, or `null` if it no longer exists. + * + * Pass `ctx` when calling from inside a transaction — the read then runs on that + * transaction's connection instead of checking out a second one, which would + * deadlock a single-connection pool. + */ + async findArchivedState( + workflowId: string, + ctx: OperationContext = {}, + ): Promise<{ isArchived: boolean } | null> { + return await this.managerFor(ctx).findOne(WorkflowEntity, { + select: { isArchived: true }, + where: { id: workflowId }, + }); + } + async getAllActiveIds() { const result = await this.find({ select: { id: true }, diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts new file mode 100644 index 00000000000..9d9b71a729b --- /dev/null +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts @@ -0,0 +1,232 @@ +process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS = 'true'; + +import { + createTeamProject, + createWorkflow, + getPersonalProject, + mockInstance, + testDb, +} from '@n8n/backend-test-utils'; +import type { Project, User } from '@n8n/db'; +import { + WorkflowRepository, + WorkflowReviewRequestAuthorRepository, + WorkflowReviewRequestRepository, + WorkflowReviewRequestWorkflowRepository, +} from '@n8n/db'; +import { Container } from '@n8n/di'; +import { v4 as uuid } from 'uuid'; + +import { ActiveWorkflowManager } from '@/active-workflow-manager'; +import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service'; +import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee'; +import { WorkflowService } from '@/workflows/workflow.service'; +import { createOwner } from '@test-integration/db/users'; +import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history'; +import type { SuperAgentTest } from '@test-integration/types'; +import * as utils from '@test-integration/utils'; + +mockInstance(ActiveWorkflowManager); + +const testServer = utils.setupTestServer({ + endpointGroups: ['workflow-reviews', 'workflows'], + enabledFeatures: ['feat:workflowReviews'], + modules: ['workflow-reviews'], +}); + +let owner: User; +let ownerProject: Project; +let ownerAgent: SuperAgentTest; + +let requestRepository: WorkflowReviewRequestRepository; +let linkRepository: WorkflowReviewRequestWorkflowRepository; +let authorRepository: WorkflowReviewRequestAuthorRepository; + +beforeAll(async () => { + await utils.initNodeTypes(); + requestRepository = Container.get(WorkflowReviewRequestRepository); + linkRepository = Container.get(WorkflowReviewRequestWorkflowRepository); + authorRepository = Container.get(WorkflowReviewRequestAuthorRepository); +}); + +beforeEach(async () => { + process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS = 'true'; + testServer.license.enable('feat:workflowReviews'); + + await testDb.truncate([ + 'WorkflowReviewRequestAuthor', + 'WorkflowReviewRequestReviewer', + 'WorkflowReviewRequestWorkflow', + 'WorkflowReviewRequest', + 'SharedWorkflow', + 'WorkflowPublishedVersion', + 'WorkflowPublicationOutbox', + 'WorkflowPublishHistory', + 'WorkflowEntity', + 'WorkflowHistory', + 'ProjectRelation', + 'Project', + 'User', + ]); + + await Container.get(WorkflowReviewPolicyService).set(true); + + owner = await createOwner(); + ownerProject = await getPersonalProject(owner); + ownerAgent = testServer.authAgentFor(owner); +}); + +/** Create a workflow owned by `owner` with a pinned history version. */ +async function createReviewableWorkflow() { + const versionId = uuid(); + const workflow = await createWorkflow({ versionId }, owner); + await createWorkflowHistoryItem(workflow.id, { versionId }); + return { workflow, versionId }; +} + +async function createOpenReview( + workflowId: string, + versionId: string, + overrides: { + state?: 'open' | 'closed'; + decision?: 'pending' | 'changes_requested' | 'approved'; + } = {}, +) { + const request = await requestRepository.createRequest( + { + projectId: ownerProject.id, + title: 'Review before publishing', + createdById: owner.id, + state: overrides.state, + decision: overrides.decision, + }, + {}, + ); + await linkRepository.createWorkflowRow( + { workflowReviewRequestId: request.id, workflowId, workflowVersionId: versionId }, + {}, + ); + await authorRepository.addAuthor({ workflowReviewRequestId: request.id, userId: owner.id }, {}); + return request; +} + +describe('auto-close on workflow archive', () => { + test('archiving closes the open review, leaving the decision unchanged', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId, { + decision: 'changes_requested', + }); + + await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(closed?.decision).toBe('changes_requested'); + expect(closed?.closedById).toBeNull(); + expect(closed?.approvedAt).toBeNull(); + }); + + test('unarchiving does not reopen the review, and the workflow is no longer publish-blocked', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + + await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200); + await ownerAgent.post(`/workflows/${workflow.id}/unarchive`).expect(200); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull(); + }); + + test('an already-closed (approved) review is untouched by archiving', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId, { + state: 'closed', + decision: 'approved', + }); + + await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200); + + const untouched = await requestRepository.findById(request.id, {}); + expect(untouched?.state).toBe('closed'); + expect(untouched?.decision).toBe('approved'); + expect(untouched?.updatedAt).toEqual(request.updatedAt); + }); +}); + +describe('auto-close on workflow transfer', () => { + test('moving the workflow to another project closes the open review', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + const destination = await createTeamProject('Destination', owner); + + await Container.get(EnterpriseWorkflowService).transferWorkflow( + owner, + workflow.id, + destination.id, + ); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(closed?.decision).toBe('pending'); + expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull(); + }); +}); + +describe('auto-close on workflow hard delete', () => { + test('force-deleting a non-archived workflow closes the review instead of orphaning it open', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + + await Container.get(WorkflowService).delete(owner, workflow.id, true); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + // The link row cascaded away with the workflow; the request itself remains, closed. + expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0); + }); + + // A review opened after the pre-delete hook ran loses its link row to the cascade and + // can no longer be found by workflow id. The post-delete sweep is what catches it. + test('a review orphaned by a delete that skipped the hooks is closed by the next delete', async () => { + const orphaned = await createReviewableWorkflow(); + const request = await createOpenReview(orphaned.workflow.id, orphaned.versionId); + + // Delete the row straight from the repository, as a folder-hierarchy cascade does: + // no hook fires, so the request is left open with its link row cascaded away. + await Container.get(WorkflowRepository).delete(orphaned.workflow.id); + expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); + expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0); + + const unrelated = await createReviewableWorkflow(); + await Container.get(WorkflowService).delete(owner, unrelated.workflow.id, true); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(closed?.closedById).toBeNull(); + }); + + test('leaves a review whose workflow still exists open', async () => { + const live = await createReviewableWorkflow(); + const request = await createOpenReview(live.workflow.id, live.versionId); + + const other = await createReviewableWorkflow(); + await Container.get(WorkflowService).delete(owner, other.workflow.id, true); + + expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); + }); +}); + +describe('auto-close with the instance policy disabled', () => { + test('cleanup still runs when the policy toggle is off', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + + await Container.get(WorkflowReviewPolicyService).set(false); + + await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + }); +}); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts new file mode 100644 index 00000000000..81cfbf6e051 --- /dev/null +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts @@ -0,0 +1,181 @@ +import type { Logger } from '@n8n/backend-common'; +import type { + DbLockService, + OperationContext, + Transaction, + WorkflowReviewRequest, + WorkflowReviewRequestRepository, +} from '@n8n/db'; +import { mock } from 'vitest-mock-extended'; + +import type { CollaborationService } from '@/collaboration/collaboration.service'; + +import { WorkflowReviewAutoCloseService } from '../workflow-review-auto-close.service'; + +describe('WorkflowReviewAutoCloseService', () => { + const logger = mock(); + const requestRepository = mock(); + const dbLockService = mock(); + const collaborationService = mock(); + /** The lock's context. Distinct from the root `{}` so tests can tell the two apart. */ + const ctx: OperationContext = { trx: mock() }; + + const service = new WorkflowReviewAutoCloseService( + logger, + requestRepository, + dbLockService, + collaborationService, + ); + + const openRequest = (overrides: Partial = {}) => + mock({ + id: 'req-1', + state: 'open', + decision: 'pending', + closedById: null, + ...overrides, + }); + + beforeEach(() => { + vi.resetAllMocks(); + dbLockService.withLockContext.mockImplementation(async (_id, fn) => await fn(ctx)); + requestRepository.saveRequest.mockImplementation(async (request) => request); + collaborationService.broadcastWorkflowReviewStateChanged.mockResolvedValue(undefined); + }); + + it('closes an open request on archive, leaving the decision and audit fields intact', async () => { + const request = openRequest({ decision: 'changes_requested', updatedById: 'user-2' }); + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + { request, workflowIds: ['wf-1'] }, + ]); + + await service.afterWorkflowArchived('wf-1'); + + expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith(['wf-1'], ctx); + expect(requestRepository.saveRequest).toHaveBeenCalledExactlyOnceWith(request, ctx); + expect(request.state).toBe('closed'); + expect(request.decision).toBe('changes_requested'); + expect(request.closedById).toBeNull(); + expect(request.updatedById).toBe('user-2'); + expect( + collaborationService.broadcastWorkflowReviewStateChanged, + ).toHaveBeenCalledExactlyOnceWith('wf-1'); + }); + + it('closes each open request on transfer and broadcasts once per affected workflow', async () => { + const first = openRequest({ id: 'req-1' }); + const second = openRequest({ id: 'req-2' }); + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + { request: first, workflowIds: ['wf-1'] }, + { request: second, workflowIds: ['wf-2'] }, + ]); + + await service.afterWorkflowsTransferred(['wf-1', 'wf-2', 'wf-3']); + + expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith( + ['wf-1', 'wf-2', 'wf-3'], + ctx, + ); + expect(first.state).toBe('closed'); + expect(second.state).toBe('closed'); + expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledTimes(2); + expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith('wf-1'); + expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith('wf-2'); + }); + + it('closes an open request before its workflow is deleted', async () => { + const request = openRequest(); + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + { request, workflowIds: ['wf-1'] }, + ]); + + await service.beforeWorkflowDeleted('wf-1'); + + expect(request.state).toBe('closed'); + }); + + it('does nothing when no open request is linked — no save, no broadcast', async () => { + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); + + await service.afterWorkflowArchived('wf-1'); + + expect(requestRepository.saveRequest).not.toHaveBeenCalled(); + expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('swallows and logs repository errors instead of failing the workflow mutation', async () => { + requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); + + await expect(service.afterWorkflowArchived('wf-1')).resolves.toBeUndefined(); + + expect(logger.error).toHaveBeenCalled(); + expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled(); + }); + + it('swallows repository errors on transfer too — the move already committed', async () => { + requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); + + await expect(service.afterWorkflowsTransferred(['wf-1'])).resolves.toBeUndefined(); + + expect(logger.error).toHaveBeenCalled(); + }); + + // The pre-delete hook must not swallow: the link rows would cascade away and strand the + // still-open request. + it('rethrows repository errors before a delete, so the delete is called off', async () => { + const error = new Error('db down'); + requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(error); + + await expect(service.beforeWorkflowDeleted('wf-1')).rejects.toThrow(error); + + expect(logger.error).toHaveBeenCalled(); + }); + + describe('afterWorkflowDeleted', () => { + it('closes the requests the delete orphaned, outside any lock', async () => { + requestRepository.closeOrphanedOpenRequests.mockResolvedValue(['req-9']); + + await service.afterWorkflowDeleted('wf-1'); + + // A single atomic statement pair, so it needs no lock — and must not take one + // after a delete, where camping on the create lock would serialize submissions. + expect(requestRepository.closeOrphanedOpenRequests).toHaveBeenCalledExactlyOnceWith({}); + expect(dbLockService.withLockContext).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalled(); + }); + + it('stays quiet when the delete orphaned nothing', async () => { + requestRepository.closeOrphanedOpenRequests.mockResolvedValue([]); + + await service.afterWorkflowDeleted('wf-1'); + + expect(logger.info).not.toHaveBeenCalled(); + }); + + // The delete already committed, so there is nothing left to abort. + it('swallows repository errors, unlike the pre-delete hook', async () => { + requestRepository.closeOrphanedOpenRequests.mockRejectedValue(new Error('db down')); + + await expect(service.afterWorkflowDeleted('wf-1')).resolves.toBeUndefined(); + + expect(logger.error).toHaveBeenCalled(); + }); + }); + + it('a failed broadcast is only warned about, never thrown', async () => { + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + { request: openRequest(), workflowIds: ['wf-1'] }, + ]); + collaborationService.broadcastWorkflowReviewStateChanged.mockRejectedValue( + new Error('push down'), + ); + + await expect(service.afterWorkflowArchived('wf-1')).resolves.toBeUndefined(); + + // Let the fire-and-forget rejection settle before asserting. + await new Promise(process.nextTick); + expect(logger.warn).toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts index cb7e8ead3b0..e38a653cfc4 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts @@ -96,9 +96,11 @@ describe('WorkflowReviewInboxService.getDetail', () => { licenseState.isWorkflowReviewsLicensed.mockReturnValue(true); workflowReviewPolicyService.get.mockResolvedValue({ enabled: true }); requestRepository.findById.mockResolvedValue(reviewRequest()); - // By default the caller can still read every workflow the review covers + // By default the review covers one workflow the caller can still read workflowFinderService.findWorkflowForUser.mockResolvedValue(mock()); - workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); + workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([ + { workflowId, workflowName: 'My workflow', workflowVersionId: null }, + ]); reviewerRepository.findByRequestIds.mockResolvedValue([]); userRepository.findManyByIds.mockResolvedValue([]); publishedVersionRepository.getPublishedVersionId.mockResolvedValue(null); @@ -220,14 +222,26 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(detail.workflows[0]).toMatchObject({ workflowId, workflowName: 'My workflow' }); }); - // A covered workflow is removed along with the workflow itself, so a review can end up with none - it('returns no workflows when the review no longer covers any', async () => { + // A covered workflow is removed along with the workflow itself, so a closed + // review — history of a deleted workflow — can legitimately cover none + it('returns a closed review with no workflows when its workflow was deleted', async () => { + requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' })); + workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); + const detail = await service.getDetail(requester, requestId); expect(detail.workflows).toEqual([]); expect(detail.workflowName).toBeNull(); expect(detail.workflowVersionId).toBeNull(); }); + + // An open review with no covered workflow is a dead leftover (nothing can + // decide or update it), so it is hidden — from its requester too + it('reports an open review whose workflows were all deleted as not found', async () => { + workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); + + await expect(service.getDetail(requester, requestId)).rejects.toThrow(NotFoundError); + }); }); describe('viewer decision eligibility', () => { @@ -272,7 +286,10 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(detail.viewerDecisionIneligibilityReason).toBe('missing_publish_permission'); }); - it('passes no workflow id when the review no longer covers any workflow', async () => { + it('passes no workflow id when a closed review no longer covers any workflow', async () => { + requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' })); + workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); + await service.getDetail(requester, requestId); expect(decisionEligibilityService.resolveViewerEligibility).toHaveBeenCalledWith( diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts index 305ed58a78a..9abb5c2edba 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts @@ -15,6 +15,7 @@ import type { WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflow, WorkflowReviewRequestWorkflowRepository, + WorkflowRepository, Transaction, OperationContext, } from '@n8n/db'; @@ -70,6 +71,7 @@ describe('WorkflowReviewRequestService.decide', () => { workflowFinderService, workflowHistoryService, workflowHistoryRepository, + mock(), sharedWorkflowRepository, publishHistoryRepository, requestRepository, diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts index c664cb4d921..56c97d8ef3c 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts @@ -20,6 +20,7 @@ import type { WorkflowReviewRequestForWorkflowRow, WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflowRepository, + WorkflowRepository, Transaction, OperationContext, } from '@n8n/db'; @@ -62,6 +63,8 @@ describe('WorkflowReviewRequestService', () => { const workflowFinderService = mock(); const workflowHistoryService = mock(); const workflowHistoryRepository = mock(); + /** The `workflow_entity` repository. `workflowRepository` below is the review's link table. */ + const workflowEntityRepository = mock(); const sharedWorkflowRepository = mock(); const publishHistoryRepository = mock(); const requestRepository = mock(); @@ -85,6 +88,7 @@ describe('WorkflowReviewRequestService', () => { workflowFinderService, workflowHistoryService, workflowHistoryRepository, + workflowEntityRepository, sharedWorkflowRepository, publishHistoryRepository, requestRepository, @@ -116,6 +120,8 @@ describe('WorkflowReviewRequestService', () => { mock({ isArchived: false }), ); workflowHistoryService.findVersion.mockResolvedValue(mock()); + // The in-lock re-check reads archived state on the lock's own connection. + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false }); sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue( mock({ id: 'project-1' }), ); @@ -144,6 +150,7 @@ describe('WorkflowReviewRequestService', () => { mock({ isArchived: false }), ); workflowHistoryService.findVersion.mockResolvedValue(mock()); + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false }); sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue( mock({ id: 'project-1' }), ); @@ -228,6 +235,7 @@ describe('WorkflowReviewRequestService', () => { mock({ isArchived: false }), ); workflowHistoryService.findVersion.mockResolvedValue(mock()); + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false }); sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue( mock({ id: 'project-1' }), ); @@ -244,6 +252,52 @@ describe('WorkflowReviewRequestService', () => { expect(authorRepository.addAuthor).not.toHaveBeenCalled(); }); + it('rejects a workflow archived between the pre-lock check and the lock', async () => { + mockSuccessfulCreatePath(); + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: true }); + + await expect(service.create(user, dto)).rejects.toThrow(BadRequestError); + + expect(dbLockService.withLockContext).toHaveBeenCalled(); + expect(requestRepository.createRequest).not.toHaveBeenCalled(); + expect(workflowRepository.createWorkflowRow).not.toHaveBeenCalled(); + }); + + it('rejects a workflow deleted between the pre-lock check and the lock', async () => { + mockSuccessfulCreatePath(); + workflowEntityRepository.findArchivedState.mockResolvedValue(null); + + await expect(service.create(user, dto)).rejects.toThrow(NotFoundError); + + expect(requestRepository.createRequest).not.toHaveBeenCalled(); + expect(workflowRepository.createWorkflowRow).not.toHaveBeenCalled(); + }); + + // A read that checks out a second connection here deadlocks a single-connection pool. + it('runs both in-lock re-check reads on the lock transaction', async () => { + mockSuccessfulCreatePath(); + + await service.create(user, dto); + + expect(workflowEntityRepository.findArchivedState).toHaveBeenCalledWith('wf-1', ctx); + expect(sharedWorkflowRepository.getWorkflowOwningProject).toHaveBeenLastCalledWith( + 'wf-1', + ctx, + ); + }); + + it('rejects a workflow moved to another project between the pre-lock check and the lock', async () => { + mockSuccessfulCreatePath(); + sharedWorkflowRepository.getWorkflowOwningProject + .mockResolvedValueOnce(mock({ id: 'project-1' })) + .mockResolvedValueOnce(mock({ id: 'project-2' })); + + await expect(service.create(user, dto)).rejects.toThrow(ConflictError); + + expect(dbLockService.withLockContext).toHaveBeenCalled(); + expect(requestRepository.createRequest).not.toHaveBeenCalled(); + }); + describe('reviewer assignment', () => { const mockEligibleReviewers = (...ids: string[]) => { roleService.rolesWithScope.mockResolvedValue(['some-role']); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts index b19f9592c4e..e9e6a70707b 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts @@ -16,6 +16,7 @@ import type { WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflow, WorkflowReviewRequestWorkflowRepository, + WorkflowRepository, Transaction, OperationContext, } from '@n8n/db'; @@ -73,6 +74,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => { workflowFinderService, workflowHistoryService, workflowHistoryRepository, + mock(), sharedWorkflowRepository, publishHistoryRepository, requestRepository, diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts index 2482acf9c7d..b18f40f8fa3 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts @@ -2109,6 +2109,16 @@ describe('GET /workflow-review-requests', () => { }); }); +/** An open request only surfaces in the inbox while it covers a live workflow. */ +async function linkToNewWorkflow(workflowReviewRequestId: string, project = teamProject) { + const workflow = await createWorkflow({}, project); + await workflowRepository.createWorkflowRow( + { workflowReviewRequestId, workflowId: workflow.id }, + {}, + ); + return workflow; +} + async function seedInboxRequests() { const openRequest = await requestRepository.createRequest( { @@ -2119,6 +2129,8 @@ async function seedInboxRequests() { }, {}, ); + const openWorkflow = await linkToNewWorkflow(openRequest.id); + // No link row: a hard-deleted workflow leaves closed requests exactly like this const closedRequest = await requestRepository.createRequest( { projectId: teamProject.id, @@ -2128,7 +2140,7 @@ async function seedInboxRequests() { }, {}, ); - return { openRequest, closedRequest }; + return { openRequest, closedRequest, openWorkflow }; } describe('GET /workflow-review-requests/summary', () => { @@ -2157,7 +2169,7 @@ describe('GET /workflow-review-requests/summary', () => { }); test('counts a requester their own review regardless of project scope', async () => { - await requestRepository.createRequest( + const ownRequest = await requestRepository.createRequest( { projectId: teamProject.id, title: 'Review submitted by viewer', @@ -2166,12 +2178,38 @@ describe('GET /workflow-review-requests/summary', () => { }, {}, ); + await linkToNewWorkflow(ownRequest.id); const response = await viewerAgent.get('/workflow-review-requests/summary').expect(200); expect(response.body.data).toEqual({ open: 1, closed: 0 }); }); + test('does not count an open review orphaned by a workflow hard delete', async () => { + await seedInboxRequests(); + const orphan = await requestRepository.createRequest( + { + projectId: teamProject.id, + title: 'Orphaned review', + createdById: owner.id, + state: 'open', + }, + {}, + ); + const workflow = await linkToNewWorkflow(orphan.id); + // Bypasses the auto-close hook: the cascade removes the link row and + // leaves the request open — as the folder cascade or a create/delete race would + await workflowEntityRepository.delete({ id: workflow.id }); + + // Owner exercises the global scope, member the project-scoped filter. + // The closed seed request has no link rows and must stay counted. + const ownerResponse = await ownerAgent.get('/workflow-review-requests/summary').expect(200); + expect(ownerResponse.body.data).toEqual({ open: 1, closed: 1 }); + + const memberResponse = await memberAgent.get('/workflow-review-requests/summary').expect(200); + expect(memberResponse.body.data).toEqual({ open: 1, closed: 1 }); + }); + test('returns 403 when feature is disabled', async () => { await policyService.set(false); @@ -2181,7 +2219,7 @@ describe('GET /workflow-review-requests/summary', () => { describe('GET /workflow-review-requests/inbox', () => { test('returns reviews for instance owner', async () => { - const { openRequest } = await seedInboxRequests(); + const { openRequest, openWorkflow } = await seedInboxRequests(); const response = await ownerAgent .get('/workflow-review-requests/inbox') @@ -2193,7 +2231,7 @@ describe('GET /workflow-review-requests/inbox', () => { id: openRequest.id, title: 'Open review request', state: 'open', - workflowName: null, + workflowName: openWorkflow.name, workflowVersionId: null, }); expect(response.body.data.hasMore).toBe(false); @@ -2209,6 +2247,54 @@ describe('GET /workflow-review-requests/inbox', () => { expect(response.body.data.hasMore).toBe(false); }); + test('omits an open review orphaned by a workflow hard delete', async () => { + const { openRequest } = await seedInboxRequests(); + const orphan = await requestRepository.createRequest( + { + projectId: teamProject.id, + title: 'Orphaned review', + createdById: owner.id, + state: 'open', + }, + {}, + ); + const workflow = await linkToNewWorkflow(orphan.id); + // Bypasses the auto-close hook: the cascade removes the link row and + // leaves the request open — as the folder cascade or a create/delete race would + await workflowEntityRepository.delete({ id: workflow.id }); + + // Owner exercises the global scope, member the project-scoped filter + const ownerResponse = await ownerAgent + .get('/workflow-review-requests/inbox') + .query({ state: 'open', limit: 15 }) + .expect(200); + expect(ownerResponse.body.data.data.map((row: { id: string }) => row.id)).toEqual([ + openRequest.id, + ]); + + const memberResponse = await memberAgent + .get('/workflow-review-requests/inbox') + .query({ state: 'open', limit: 15 }) + .expect(200); + expect(memberResponse.body.data.data.map((row: { id: string }) => row.id)).toEqual([ + openRequest.id, + ]); + }); + + test('still lists a closed review whose workflow was hard-deleted', async () => { + const { closedRequest } = await seedInboxRequests(); + + const response = await ownerAgent + .get('/workflow-review-requests/inbox') + .query({ state: 'closed', limit: 15 }) + .expect(200); + + // The closed seed request has no link rows — deleted-workflow history stays visible + expect(response.body.data.data).toEqual([ + expect.objectContaining({ id: closedRequest.id, state: 'closed', workflowName: null }), + ]); + }); + test('returns 403 when license is disabled', async () => { testServer.license.disable('feat:workflowReviews'); @@ -2217,7 +2303,7 @@ describe('GET /workflow-review-requests/inbox', () => { test('returns cursor pagination metadata', async () => { await seedInboxRequests(); - await requestRepository.createRequest( + const secondRequest = await requestRepository.createRequest( { projectId: teamProject.id, title: 'Second open review', @@ -2226,6 +2312,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(secondRequest.id); const firstPage = await ownerAgent .get('/workflow-review-requests/inbox') @@ -2288,7 +2375,7 @@ describe('GET /workflow-review-requests/inbox', () => { test('hides reviews from projects the member cannot access', async () => { const otherProject = await createTeamProject('Other Reviews Project', owner); - await requestRepository.createRequest( + const privateRequest = await requestRepository.createRequest( { projectId: otherProject.id, title: 'Private other-project review', @@ -2297,6 +2384,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(privateRequest.id, otherProject); const memberResponse = await memberAgent.get('/workflow-review-requests/inbox').expect(200); expect(memberResponse.body.data.data).toEqual([]); @@ -2322,6 +2410,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(ownRequest.id, otherProject); const response = await memberAgent.get('/workflow-review-requests/inbox').expect(200); @@ -2332,7 +2421,7 @@ describe('GET /workflow-review-requests/inbox', () => { test('does not truncate pagination when the cursor row is deleted', async () => { await seedInboxRequests(); - await requestRepository.createRequest( + const secondRequest = await requestRepository.createRequest( { projectId: teamProject.id, title: 'Second open review', @@ -2341,6 +2430,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(secondRequest.id); const firstPage = await ownerAgent .get('/workflow-review-requests/inbox') @@ -2372,6 +2462,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(request.id); await reviewerRepository.addReviewers( { workflowReviewRequestId: request.id, @@ -2412,6 +2503,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(request.id); const response = await ownerAgent .get('/workflow-review-requests/inbox') @@ -2437,6 +2529,7 @@ describe('GET /workflow-review-requests/inbox', () => { }, {}, ); + await linkToNewWorkflow(request.id); await reviewerRepository.addReviewers( { workflowReviewRequestId: request.id, @@ -2591,12 +2684,42 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { }); }); - test('still opens the review after its workflow was deleted', async () => { + test('no longer opens an open review after its workflow was hard-deleted', async () => { const workflow = await createWorkflow({}, teamProject); await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' }); const request = await seedRequest(workflow.id, 'version-pinned', owner); - // Deleting the workflow removes the review's reference to it as well + // Bypasses the auto-close hook: the cascade removes the link row and + // leaves the request open — a dead leftover nothing can decide or update + await workflowEntityRepository.delete({ id: workflow.id }); + + // 404 even for the requester (owner) — their inbox no longer lists it either + await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(404); + }); + + test('still opens a closed review after its workflow was deleted', async () => { + const workflow = await createWorkflow({}, teamProject); + await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' }); + const request = await requestRepository.createRequest( + { + projectId: teamProject.id, + title: 'Please review', + createdById: owner.id, + state: 'closed', + decision: 'approved', + }, + {}, + ); + await workflowRepository.createWorkflowRow( + { + workflowReviewRequestId: request.id, + workflowId: workflow.id, + workflowVersionId: 'version-pinned', + }, + {}, + ); + + // Deleting the workflow removes the review's reference, not its history await workflowEntityRepository.delete({ id: workflow.id }); const response = await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(200); diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts new file mode 100644 index 00000000000..755a8eb3352 --- /dev/null +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts @@ -0,0 +1,128 @@ +import { Logger } from '@n8n/backend-common'; +import { DbLock, DbLockService, WorkflowReviewRequestRepository } from '@n8n/db'; +import { Service } from '@n8n/di'; + +import { CollaborationService } from '@/collaboration/collaboration.service'; +import type { WorkflowMutationHooks } from '@/workflows/workflow-mutation-hooks-proxy.service'; + +type AutoCloseReason = 'workflow-archived' | 'workflow-moved' | 'workflow-deleted'; + +/** + * Closes open review requests when their workflow stops being reviewable: + * archived, moved to another project, or deleted. + * + * Deliberately not feature-gated beyond module load: the instance policy + * toggle guards user actions, but a review left open while the policy is off + * would still block publishing once the policy is re-enabled, so cleanup must + * run regardless. + */ +@Service() +export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { + constructor( + private readonly logger: Logger, + private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository, + private readonly dbLockService: DbLockService, + private readonly collaborationService: CollaborationService, + ) {} + + async afterWorkflowArchived(workflowId: string): Promise { + await this.closeOpenRequestsForWorkflows([workflowId], 'workflow-archived'); + } + + async afterWorkflowsTransferred(workflowIds: string[]): Promise { + await this.closeOpenRequestsForWorkflows(workflowIds, 'workflow-moved'); + } + + async beforeWorkflowDeleted(workflowId: string): Promise { + // The delete hasn't happened yet, so aborting is recoverable — whereas swallowing + // here would let the link rows cascade away and strand an open request. + await this.closeOpenRequestsForWorkflows([workflowId], 'workflow-deleted', { + rethrow: true, + }); + } + + /** + * Closes reviews whose workflow is gone: the cascade took their link row, so they can + * only be found by "open with nothing linked", not by workflow id. Catches reviews + * opened after the pre-delete hook ran, and any left by a delete that skips the hooks. + */ + async afterWorkflowDeleted(workflowId: string): Promise { + try { + const closedRequestIds = await this.workflowReviewRequestRepository.closeOrphanedOpenRequests( + {}, + ); + + if (closedRequestIds.length === 0) return; + + this.logger.info('Closed workflow review request(s) left without a workflow', { + reason: 'workflow-deleted', + workflowId, + workflowReviewRequestIds: closedRequestIds, + }); + } catch (error) { + // The delete has already committed; failing it now would be worse than a + // request that stays open, which the inbox hides until the next sweep. + this.logger.error('Failed to close workflow review request(s) left without a workflow', { + workflowId, + error, + }); + } + } + + private async closeOpenRequestsForWorkflows( + workflowIds: string[], + reason: AutoCloseReason, + options: { rethrow?: boolean } = {}, + ): Promise { + try { + const affectedWorkflowIds = await this.dbLockService.withLockContext( + DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, + async (ctx) => { + // Fetched under the lock so the close can't race a concurrent + // decide/version sync on the same request. + const openRequests = + await this.workflowReviewRequestRepository.findOpenRequestsForWorkflows( + workflowIds, + ctx, + ); + + const affected = new Set(); + for (const { request, workflowIds: linkedWorkflowIds } of openRequests) { + request.state = 'closed'; + // A system close has no closing user; the decision stays as-is. + request.closedById = null; + await this.workflowReviewRequestRepository.saveRequest(request, ctx); + for (const linkedWorkflowId of linkedWorkflowIds) affected.add(linkedWorkflowId); + } + + return [...affected]; + }, + ); + + if (affectedWorkflowIds.length === 0) return; + + this.logger.info('Closed open workflow review request(s)', { + reason, + workflowIds: affectedWorkflowIds, + }); + + for (const workflowId of affectedWorkflowIds) { + // Fire-and-forget: viewers heal via focus/reconnect refetch. + this.collaborationService + .broadcastWorkflowReviewStateChanged(workflowId) + .catch((error) => + this.logger.warn('Failed to broadcast review state change', { workflowId, error }), + ); + } + } catch (error) { + this.logger.error('Failed to close open workflow review request(s)', { + reason, + workflowIds, + error, + }); + // Cleanup must never fail a mutation that already committed; only the + // pre-delete hook is still in a position to call its mutation off. + if (options.rethrow) throw error; + } + } +} diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts index 9a6f03e68d1..270a4058c77 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts @@ -136,6 +136,15 @@ export class WorkflowReviewInboxService { this.workflowReviewRequestReviewerRepository.findByRequestIds([request.id]), ]); + // An open request whose link rows all cascaded away with a workflow hard + // delete is a dead leftover: nothing can act on it and the inbox hides it, + // so hide it here too — for the requester as well, matching their inbox. + // A closed request keeps zero link rows legitimately (history of a deleted + // workflow) and stays readable. + if (request.state === 'open' && workflowRows.length === 0) { + throw new NotFoundError('Could not find review request'); + } + const readableRows = await this.filterReadableWorkflowRows(user, workflowRows); // Someone who reaches this review through its project has no reason to learn it // exists once they can read none of the workflows it covers. The requester already diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts index 6254118ee75..b62a8f038ef 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts @@ -23,6 +23,7 @@ import { WorkflowPublishHistoryRepository, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, + WorkflowRepository, WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflowRepository, type OperationContext, @@ -67,6 +68,7 @@ export class WorkflowReviewRequestService { private readonly workflowFinderService: WorkflowFinderService, private readonly workflowHistoryService: WorkflowHistoryService, private readonly workflowHistoryRepository: WorkflowHistoryRepository, + private readonly workflowRepository: WorkflowRepository, private readonly sharedWorkflowRepository: SharedWorkflowRepository, private readonly workflowPublishHistoryRepository: WorkflowPublishHistoryRepository, private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository, @@ -273,6 +275,41 @@ export class WorkflowReviewRequestService { } } + /** + * Re-asserts under the lock what `create`'s pre-lock checks established: the + * workflow still exists, is unarchived, and still belongs to the same project. + * Access is not re-checked — the races this guards against are archive and + * transfer, both of which these two reads cover. + * + * Every read is threaded with `ctx` so it runs on the lock transaction's own + * connection. A read that checks out a second connection here deadlocks against + * the transaction holding the first one (see the same note in `decide`). + */ + private async assertWorkflowStillReviewable( + workflowId: string, + expectedProjectId: string, + ctx: OperationContext, + ): Promise { + const workflow = await this.workflowRepository.findArchivedState(workflowId, ctx); + if (!workflow) { + throw new NotFoundError('Could not find workflow'); + } + + if (workflow.isArchived) { + throw new BadRequestError( + `The workflow '${workflowId}' is archived and cannot be submitted for review`, + ); + } + + const project = await this.sharedWorkflowRepository.getWorkflowOwningProject(workflowId, ctx); + if (project?.id !== expectedProjectId) { + throw new ConflictError( + `The workflow '${workflowId}' moved to another project and cannot be submitted for review here`, + 'Retry from the project that now owns the workflow', + ); + } + } + async create( user: User, dto: CreateWorkflowReviewRequestDto, @@ -330,6 +367,10 @@ export class WorkflowReviewRequestService { const request = await this.dbLockService.withLockContext( DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, async (ctx) => { + // without this, a create that lost the race opens a review on an archived + // or moved workflow. + await this.assertWorkflowStillReviewable(workflowId, project.id, ctx); + const existing = await this.workflowReviewRequestRepository.findOpenRequestForWorkflow( workflowId, ctx, diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-reviews.module.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-reviews.module.ts index df559f213cf..283de790b87 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-reviews.module.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-reviews.module.ts @@ -4,6 +4,7 @@ import { BackendModule } from '@n8n/decorators'; import { Container } from '@n8n/di'; import { isWorkflowReviewsEnvFeatureFlagEnabled } from '@/constants/workflow-reviews'; +import { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service'; @BackendModule({ name: 'workflow-reviews', licenseFlag: LICENSE_FEATURES.WORKFLOW_REVIEWS }) @@ -18,5 +19,12 @@ export class WorkflowReviewsModule implements ModuleInterface { Container.get(WorkflowPublishGuardProxy).registerProvider( Container.get(WorkflowReviewPublishGuard), ); + + const { WorkflowReviewAutoCloseService } = await import( + './workflow-review-auto-close.service.js' + ); + Container.get(WorkflowMutationHooksProxy).registerProvider( + Container.get(WorkflowReviewAutoCloseService), + ); } } diff --git a/packages/cli/src/services/ownership-transfer/ownership-transfer.manifest.json b/packages/cli/src/services/ownership-transfer/ownership-transfer.manifest.json index d1fef090970..f2cd0ea19df 100644 --- a/packages/cli/src/services/ownership-transfer/ownership-transfer.manifest.json +++ b/packages/cli/src/services/ownership-transfer/ownership-transfer.manifest.json @@ -67,7 +67,7 @@ { "name": "WorkflowReviewRequest", "path": "packages/@n8n/db/src/entities/workflow-review-request.ee.ts", - "reason": "Not transferred with the project. Open reviews should be closed when their workflows move to another project; that close-on-move behaviour is deferred." + "reason": "Not transferred with the project. Open reviews are closed when their workflow is archived, moved to another project, or deleted (workflow lifecycle hooks); rows are removed via the projectId FK cascade when the source project itself is deleted, as in ownership-transfer flows." } ] } diff --git a/packages/cli/src/workflows/__tests__/workflow-mutation-hooks-proxy.service.test.ts b/packages/cli/src/workflows/__tests__/workflow-mutation-hooks-proxy.service.test.ts new file mode 100644 index 00000000000..084b8fe1ce9 --- /dev/null +++ b/packages/cli/src/workflows/__tests__/workflow-mutation-hooks-proxy.service.test.ts @@ -0,0 +1,36 @@ +import { mock } from 'vitest-mock-extended'; + +import { + type WorkflowMutationHooks, + WorkflowMutationHooksProxy, +} from '../workflow-mutation-hooks-proxy.service'; + +describe('WorkflowMutationHooksProxy', () => { + test('every hook is a no-op when no provider is registered', async () => { + const proxy = new WorkflowMutationHooksProxy(); + + await expect(proxy.afterWorkflowArchived('workflow-1')).resolves.toBeUndefined(); + await expect(proxy.afterWorkflowsTransferred(['workflow-1'])).resolves.toBeUndefined(); + await expect(proxy.beforeWorkflowDeleted('workflow-1')).resolves.toBeUndefined(); + await expect(proxy.afterWorkflowDeleted('workflow-1')).resolves.toBeUndefined(); + }); + + test('forwards each hook to the registered provider', async () => { + const proxy = new WorkflowMutationHooksProxy(); + const provider = mock(); + proxy.registerProvider(provider); + + await proxy.afterWorkflowArchived('workflow-1'); + await proxy.afterWorkflowsTransferred(['workflow-1', 'workflow-2']); + await proxy.beforeWorkflowDeleted('workflow-3'); + await proxy.afterWorkflowDeleted('workflow-4'); + + expect(provider.afterWorkflowArchived).toHaveBeenCalledExactlyOnceWith('workflow-1'); + expect(provider.afterWorkflowsTransferred).toHaveBeenCalledExactlyOnceWith([ + 'workflow-1', + 'workflow-2', + ]); + expect(provider.beforeWorkflowDeleted).toHaveBeenCalledExactlyOnceWith('workflow-3'); + expect(provider.afterWorkflowDeleted).toHaveBeenCalledExactlyOnceWith('workflow-4'); + }); +}); diff --git a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts index f2d11926aab..12c6896fef9 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts @@ -1,14 +1,18 @@ import type { CredentialsEntity, + Project, + SharedWorkflow, + WorkflowEntity, WorkflowPublishHistoryRepository, WorkflowRepository, } from '@n8n/db'; -import type { UpdateResult } from '@n8n/typeorm'; +import type { EntityManager, UpdateResult } from '@n8n/typeorm'; import type { IWorkflowBase } from 'n8n-workflow'; import { WorkflowActivationError } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; import type { ActiveWorkflowManager } from '@/active-workflow-manager'; +import type { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee'; describe('EnterpriseWorkflowService', () => { @@ -16,6 +20,7 @@ describe('EnterpriseWorkflowService', () => { const workflowRepository = mock(); const activeWorkflowManager = mock(); const workflowPublishHistoryRepository = mock(); + const workflowMutationHooks = mock(); beforeEach(() => { vi.clearAllMocks(); @@ -33,6 +38,7 @@ describe('EnterpriseWorkflowService', () => { mock(), // workflowFinderService mock(), // folderRepository workflowPublishHistoryRepository, + workflowMutationHooks, ); }); @@ -162,4 +168,50 @@ describe('EnterpriseWorkflowService', () => { expect(workflowRepository.updateActiveState).not.toHaveBeenCalled(); }); }); + + describe('transferWorkflowOwnership', () => { + const destinationProject = mock({ id: 'proj-dest' }); + + const makeWorkflow = (id: string, ownerProjectId: string) => + mock({ + id, + shared: [ + mock({ + role: 'workflow:owner', + project: mock({ id: ownerProjectId }), + }), + ], + }); + + beforeEach(() => { + const entityManager = mock(); + entityManager.transaction.mockImplementation( + // @ts-expect-error transaction() has multiple overloads; tests use the single-callback one + async (cb: (trx: EntityManager) => Promise) => await cb(mock()), + ); + Object.defineProperty(workflowRepository, 'manager', { + value: entityManager, + configurable: true, + }); + }); + + it('notifies the mutation hook only for workflows whose owning project changed', async () => { + const moved = makeWorkflow('wf-moved', 'proj-source'); + const folderMoveOnly = makeWorkflow('wf-same-project', 'proj-dest'); + + await service['transferWorkflowOwnership']([moved, folderMoveOnly], destinationProject); + + expect(workflowMutationHooks.afterWorkflowsTransferred).toHaveBeenCalledExactlyOnceWith([ + 'wf-moved', + ]); + }); + + it('does not notify the mutation hook for a same-project folder move', async () => { + const folderMoveOnly = makeWorkflow('wf-same-project', 'proj-dest'); + + await service['transferWorkflowOwnership']([folderMoveOnly], destinationProject); + + expect(workflowMutationHooks.afterWorkflowsTransferred).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index d5a312a45f8..ed7a0bc2150 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -43,6 +43,7 @@ import * as WorkflowHelpers from '@/workflow-helpers'; import type { WorkflowHookContextService } from '@/workflow-hook-context.service'; import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import type { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; +import type { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import type { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service'; import type { WorkflowValidationService } from '@/workflows/workflow-validation.service'; import { WorkflowService } from '@/workflows/workflow.service'; @@ -110,6 +111,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository mock(), // workflowHookContextService mock(), // workflowPublishGuard + mock(), // workflowMutationHooks ); }); @@ -379,6 +381,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository workflowHookContextServiceMock, // workflowHookContextService mock(), // workflowPublishGuard + mock(), // workflowMutationHooks ); vi.clearAllMocks(); @@ -1119,6 +1122,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository workflowHookContextServiceMock, // workflowHookContextService workflowPublishGuardMock, // workflowPublishGuard + mock(), // workflowMutationHooks ); // Bypass validation internals @@ -1621,6 +1625,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository mock(), // workflowHookContextService mock(), // workflowPublishGuard + mock(), // workflowMutationHooks ); }); @@ -1679,6 +1684,7 @@ describe('WorkflowService', () => { let activeWorkflowManagerMock: MockProxy; let externalHooksMock: MockProxy; let workflowPublishedVersionRepositoryMock: MockProxy; + let workflowMutationHooksMock: MockProxy; const WORKFLOW_ID = 'workflow-1'; @@ -1699,6 +1705,7 @@ describe('WorkflowService', () => { executionPersistenceMock = mock(); activeWorkflowManagerMock = mock(); externalHooksMock = mock(); + workflowMutationHooksMock = mock(); workflowPublishedVersionRepositoryMock = mock(); workflowPublishedVersionRepositoryMock.getPublishedVersionId.mockResolvedValue(null); globalConfigMock = mock({ @@ -1736,6 +1743,7 @@ describe('WorkflowService', () => { workflowPublishedVersionRepositoryMock, // workflowPublishedVersionRepository mock(), // workflowHookContextService mock(), // workflowPublishGuard + workflowMutationHooksMock, // workflowMutationHooks ); }); @@ -1783,6 +1791,88 @@ describe('WorkflowService', () => { expect(workflowRepositoryMock.delete).toHaveBeenCalledWith(WORKFLOW_ID); }); + test('runs the beforeWorkflowDeleted lifecycle hook before the row delete', async () => { + const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + + await workflowService.delete(mock(), WORKFLOW_ID, true); + + expect(workflowMutationHooksMock.beforeWorkflowDeleted).toHaveBeenCalledExactlyOnceWith( + WORKFLOW_ID, + ); + expect( + workflowMutationHooksMock.beforeWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeLessThan(workflowRepositoryMock.delete.mock.invocationCallOrder[0]); + }); + + test('does not run the beforeWorkflowDeleted lifecycle hook when deletion is rejected', async () => { + const workflow = makeWorkflowEntity({ activeVersionId: 'v1' }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + + await expect(workflowService.delete(mock(), WORKFLOW_ID, true)).rejects.toBeInstanceOf( + ConflictError, + ); + + expect(workflowMutationHooksMock.beforeWorkflowDeleted).not.toHaveBeenCalled(); + }); + + // Deactivation is not rolled back when a later step fails, so an aborted delete + // must not have torn the triggers down. + test('aborts the deletion, leaving the workflow running, when the hook throws', async () => { + globalConfigMock.workflows.useWorkflowPublicationService = false; + const workflow = makeWorkflowEntity({ active: true, activeVersionId: 'v1' }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + workflowMutationHooksMock.beforeWorkflowDeleted.mockRejectedValue(new Error('db down')); + + await expect(workflowService.delete(mock(), WORKFLOW_ID, true)).rejects.toThrow( + 'db down', + ); + + expect(activeWorkflowManagerMock.remove).not.toHaveBeenCalled(); + }); + + // The hook may throw to abort the delete, so it has to run before the executions are purged, + // not just before the row. + test('aborts the deletion, leaving executions and the row intact, when the hook throws', async () => { + const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + workflowMutationHooksMock.beforeWorkflowDeleted.mockRejectedValue(new Error('db down')); + + await expect(workflowService.delete(mock(), WORKFLOW_ID, true)).rejects.toThrow( + 'db down', + ); + + expect(executionPersistenceMock.hardDeleteByWorkflowId).not.toHaveBeenCalled(); + expect(workflowRepositoryMock.delete).not.toHaveBeenCalled(); + }); + + // It cleans up rows the cascade orphaned, which cannot be found until the row is gone. + test('runs the afterWorkflowDeleted lifecycle hook once the row is deleted', async () => { + const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + + await workflowService.delete(mock(), WORKFLOW_ID, true); + + expect(workflowMutationHooksMock.afterWorkflowDeleted).toHaveBeenCalledExactlyOnceWith( + WORKFLOW_ID, + ); + expect( + workflowMutationHooksMock.afterWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeGreaterThan(workflowRepositoryMock.delete.mock.invocationCallOrder[0]); + }); + + test('does not run the afterWorkflowDeleted lifecycle hook when the deletion is aborted', async () => { + const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + workflowMutationHooksMock.beforeWorkflowDeleted.mockRejectedValue(new Error('db down')); + + await expect(workflowService.delete(mock(), WORKFLOW_ID, true)).rejects.toThrow( + 'db down', + ); + + expect(workflowMutationHooksMock.afterWorkflowDeleted).not.toHaveBeenCalled(); + }); + test('deletes the workflow executions before the workflow itself', async () => { const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); @@ -1881,6 +1971,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository mock(), // workflowHookContextService mock(), // workflowPublishGuard + mock(), // workflowMutationHooks ); }); @@ -1933,6 +2024,7 @@ describe('WorkflowService', () => { let workflowFinderServiceMock: MockProxy; let workflowRepositoryMock: MockProxy; let externalHooksMock: MockProxy; + let workflowMutationHooksMock: MockProxy; const WORKFLOW_ID = 'workflow-1'; @@ -1969,6 +2061,7 @@ describe('WorkflowService', () => { workflowFinderServiceMock = mock(); workflowRepositoryMock = mock(); externalHooksMock = mock(); + workflowMutationHooksMock = mock(); workflowService = new WorkflowService( mock(), // logger @@ -2001,6 +2094,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository mock(), // workflowHookContextService mock(), // workflowPublishGuard + workflowMutationHooksMock, // workflowMutationHooks ); }); @@ -2027,6 +2121,29 @@ describe('WorkflowService', () => { expectedActor, ]); }); + + test('runs the afterWorkflowArchived lifecycle hook on archive', async () => { + const workflow = makeWorkflowEntity({ isArchived: false, activeVersionId: null }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + + await workflowService.archive(makeActingUser(), WORKFLOW_ID); + + expect(workflowMutationHooksMock.afterWorkflowArchived).toHaveBeenCalledExactlyOnceWith( + WORKFLOW_ID, + ); + }); + + test('runs no lifecycle hook when archiving is skipped or on unarchive', async () => { + const archived = makeWorkflowEntity({ isArchived: true }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(archived); + + await workflowService.archive(makeActingUser(), WORKFLOW_ID, { skipArchived: true }); + await workflowService.unarchive(makeActingUser(), WORKFLOW_ID); + + expect(workflowMutationHooksMock.afterWorkflowArchived).not.toHaveBeenCalled(); + expect(workflowMutationHooksMock.beforeWorkflowDeleted).not.toHaveBeenCalled(); + expect(workflowMutationHooksMock.afterWorkflowsTransferred).not.toHaveBeenCalled(); + }); }); describe('updateWorkflowTags()', () => { @@ -2074,6 +2191,7 @@ describe('WorkflowService', () => { mock(), // workflowPublishedVersionRepository mock(), // workflowHookContextService mock(), // workflowPublishGuard + mock(), // workflowMutationHooks ); }); diff --git a/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts b/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts new file mode 100644 index 00000000000..99cd1bec23a --- /dev/null +++ b/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts @@ -0,0 +1,58 @@ +import { Service } from '@n8n/di'; + +/** + * Lets a module react to workflow mutations without core depending on the + * module: it registers an implementation on {@link WorkflowMutationHooksProxy}. + * With no provider registered, every hook is a no-op. + * + * Distinct from the `workflow.afterArchive` / `workflow.afterDelete` external + * hooks, which notify code outside n8n rather than modules inside it. + * + * The `after*` hooks observe an already-committed mutation and must not throw — + * there is nothing left to abort. `beforeWorkflowDeleted` is the exception: + * it runs while the delete can still be called off, so it may throw to stop it. + */ +export interface WorkflowMutationHooks { + afterWorkflowArchived(workflowId: string): Promise; + + /** Called only for workflows whose owning project actually changed. */ + afterWorkflowsTransferred(workflowIds: string[]): Promise; + + /** + * Called before anything about the workflow is destroyed, while rows + * referencing it still exist. Throwing here aborts the deletion. + */ + beforeWorkflowDeleted(workflowId: string): Promise; + + /** + * Called once the workflow row is gone, for cleanup that can only be done + * after the delete cascades — rows orphaned by it, which by definition cannot + * be found while the workflow still exists. + */ + afterWorkflowDeleted(workflowId: string): Promise; +} + +@Service() +export class WorkflowMutationHooksProxy implements WorkflowMutationHooks { + private provider: WorkflowMutationHooks | null = null; + + registerProvider(provider: WorkflowMutationHooks): void { + this.provider = provider; + } + + async afterWorkflowArchived(workflowId: string): Promise { + await this.provider?.afterWorkflowArchived(workflowId); + } + + async afterWorkflowsTransferred(workflowIds: string[]): Promise { + await this.provider?.afterWorkflowsTransferred(workflowIds); + } + + async beforeWorkflowDeleted(workflowId: string): Promise { + await this.provider?.beforeWorkflowDeleted(workflowId); + } + + async afterWorkflowDeleted(workflowId: string): Promise { + await this.provider?.afterWorkflowDeleted(workflowId); + } +} diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index 5ac5dac1b47..6c4f02283fb 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -43,6 +43,7 @@ import { OwnershipService } from '@/services/ownership.service'; import { ProjectService } from '@/services/project.service.ee'; import { WorkflowFinderService } from './workflow-finder.service'; +import { WorkflowMutationHooksProxy } from './workflow-mutation-hooks-proxy.service'; @Service() export class EnterpriseWorkflowService { @@ -60,6 +61,7 @@ export class EnterpriseWorkflowService { private readonly workflowFinderService: WorkflowFinderService, private readonly folderRepository: FolderRepository, private readonly workflowPublishHistoryRepository: WorkflowPublishHistoryRepository, + private readonly workflowMutationHooks: WorkflowMutationHooksProxy, ) {} async shareWithProjects( @@ -648,6 +650,16 @@ export class EnterpriseWorkflowService { workflows: WorkflowEntity[], destinationProject: Project, ) { + // Resolved before the transaction rewrites the sharings. Workflows already + // owned by the destination are folder moves, not project transfers. + const movedWorkflowIds = workflows + .filter( + (workflow) => + workflow.shared.find((s) => s.role === 'workflow:owner')?.project.id !== + destinationProject.id, + ) + .map((workflow) => workflow.id); + await this.workflowRepository.manager.transaction(async (trx) => { for (const workflow of workflows) { // Remove all sharings @@ -668,6 +680,10 @@ export class EnterpriseWorkflowService { for (const workflow of workflows) { await this.ownershipService.setWorkflowProjectCacheEntry(workflow.id, destinationProject); } + + if (movedWorkflowIds.length > 0) { + await this.workflowMutationHooks.afterWorkflowsTransferred(movedWorkflowIds); + } } private async shareCredentialsWithProject( diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index aef2da01fe1..525bf2469b6 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -31,6 +31,7 @@ import { WorkflowPublicationNotifier } from './publication/workflow-publication- import { getErrorDescription, getErrorNodeId, getRequiredRedactionScopes } from './utils'; import { WorkflowFinderService } from './workflow-finder.service'; import { WorkflowHistoryService } from './workflow-history/workflow-history.service'; +import { WorkflowMutationHooksProxy } from './workflow-mutation-hooks-proxy.service'; import { WorkflowPublishGuardProxy } from './workflow-publish-guard-proxy.service'; import { WorkflowValidationService } from './workflow-validation.service'; @@ -98,6 +99,7 @@ export class WorkflowService { private readonly workflowPublishedVersionRepository: WorkflowPublishedVersionRepository, private readonly workflowHookContextService: WorkflowHookContextService, private readonly workflowPublishGuard: WorkflowPublishGuardProxy, + private readonly workflowMutationHooks: WorkflowMutationHooksProxy, ) {} async getMany( @@ -1176,6 +1178,12 @@ export class WorkflowService { throw new BadRequestError('Workflow must be archived before it can be deleted.'); } + // Ahead of every destructive step, including the trigger teardown below: the + // hook may throw to abort the delete, and deactivation is not rolled back, so + // running it later would strand the workflow as active in the DB but no longer + // running. + await this.workflowMutationHooks.beforeWorkflowDeleted(workflowId); + if (workflow.active) { // deactivate before deleting await this.activeWorkflowManager.remove(workflowId); @@ -1188,6 +1196,10 @@ export class WorkflowService { await this.workflowRepository.delete(workflowId); + // After the cascade, so it can see the rows the delete orphaned. Observes a + // committed delete, so it must not throw — the module swallows its own errors. + await this.workflowMutationHooks.afterWorkflowDeleted(workflowId); + this.eventService.emit('workflow-deleted', { user, workflowId, publicApi: false }); await this.externalHooks.run('workflow.afterDelete', [ workflowId, @@ -1254,6 +1266,8 @@ export class WorkflowService { await this.workflowHistoryService.saveVersion(user, workflow, workflowId); + await this.workflowMutationHooks.afterWorkflowArchived(workflowId); + this.eventService.emit('workflow-archived', { user, workflowId, diff --git a/packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts b/packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts index f867f2df8ec..f9891538a21 100644 --- a/packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts +++ b/packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts @@ -1,10 +1,16 @@ import { + createTeamProject, createWorkflow, createWorkflowHistory, createWorkflowWithHistory, testDb, } from '@n8n/backend-test-utils'; -import { WorkflowHistoryRepository, WorkflowPublishedVersionRepository } from '@n8n/db'; +import { + WorkflowHistoryRepository, + WorkflowPublishedVersionRepository, + WorkflowReviewRequestRepository, + WorkflowReviewRequestWorkflowRepository, +} from '@n8n/db'; import { Container } from '@n8n/di'; import { RULES, type INode } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; @@ -27,10 +33,13 @@ describe('WorkflowHistoryRepository', () => { beforeEach(async () => { await testDb.truncate([ + 'WorkflowReviewRequestWorkflow', + 'WorkflowReviewRequest', 'WorkflowPublishedVersion', 'WorkflowPublishHistory', 'WorkflowHistory', 'WorkflowEntity', + 'Project', 'User', ]); }); @@ -338,6 +347,90 @@ describe('WorkflowHistoryRepository', () => { expect(remainingIds).toContain(vCurrent); // preserved: current version expect(remainingIds).not.toContain(vOther); // pruned: old and unreferenced }); + + // The open-review exclusion must hold even when named-version preservation + // is off (unlicensed), because review pins are named versions and would + // otherwise be pruned mid-review. A closed review no longer protects its pin. + it('should preserve versions pinned by an open review but not by a closed one', async () => { + const vCurrent = uuid(); + const vOpenPinned = uuid(); + const vClosedPinned = uuid(); + + const tenDaysAgo = new Date(); + tenDaysAgo.setDate(tenDaysAgo.getDate() - 10); + + const oneDayAgo = new Date(); + oneDayAgo.setDate(oneDayAgo.getDate() - 1); + + const workflow = await createWorkflow({ + versionId: vCurrent, + nodes: [{ ...testNode1, parameters: { a: 'current' } }], + }); + await createWorkflowHistory( + { + ...workflow, + versionId: vOpenPinned, + nodes: [{ ...testNode1, parameters: { a: 'open-pinned' } }], + }, + undefined, + undefined, + { createdAt: tenDaysAgo, name: 'Open review pin' }, + ); + await createWorkflowHistory( + { + ...workflow, + versionId: vClosedPinned, + nodes: [{ ...testNode1, parameters: { a: 'closed-pinned' } }], + }, + undefined, + undefined, + { createdAt: tenDaysAgo, name: 'Closed review pin' }, + ); + await createWorkflowHistory(workflow); + + const project = await createTeamProject('Reviews Project'); + const requestRepository = Container.get(WorkflowReviewRequestRepository); + const linkRepository = Container.get(WorkflowReviewRequestWorkflowRepository); + + const openRequest = await requestRepository.createRequest( + { projectId: project.id, title: 'Open review', createdById: null }, + {}, + ); + await linkRepository.createWorkflowRow( + { + workflowReviewRequestId: openRequest.id, + workflowId: workflow.id, + workflowVersionId: vOpenPinned, + }, + {}, + ); + + const closedRequest = await requestRepository.createRequest( + { projectId: project.id, title: 'Closed review', state: 'closed', createdById: null }, + {}, + ); + await linkRepository.createWorkflowRow( + { + workflowReviewRequestId: closedRequest.id, + workflowId: workflow.id, + workflowVersionId: vClosedPinned, + }, + {}, + ); + + const repository = Container.get(WorkflowHistoryRepository); + + // preserveNamedVersions = false: the name on the pins must not save them here + await repository.deleteEarlierThanExceptCurrentAndActive(oneDayAgo, false); + + const remainingIds = (await repository.find()).map((r) => r.versionId); + expect(remainingIds).toContain(vOpenPinned); // preserved: pinned by an open review + expect(remainingIds).not.toContain(vClosedPinned); // pruned: its review is closed + + // The pin was nulled by the FK, not left dangling + const closedLinkRows = await linkRepository.findByRequestId(closedRequest.id, {}); + expect(closedLinkRows[0]?.workflowVersionId).toBeNull(); + }); }); describe('getWorkflowIdsInRange', () => { diff --git a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts index f155d0a035f..0a7b4e04e64 100644 --- a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts @@ -39,6 +39,7 @@ describe('EnterpriseWorkflowService', () => { mock(), // workflowFinderService mock(), // folderRepository mock(), // workflowPublishHistoryRepository + mock(), // workflowMutationHooks ); }); diff --git a/packages/cli/test/integration/workflows/workflow.service.test.ts b/packages/cli/test/integration/workflows/workflow.service.test.ts index 9a3599cad42..60be3edba33 100644 --- a/packages/cli/test/integration/workflows/workflow.service.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.test.ts @@ -99,6 +99,7 @@ beforeAll(async () => { workflowPublishedVersionRepository, Container.get(WorkflowHookContextService), // workflowHookContextService workflowPublishGuard, + mock(), // workflowMutationHooks ); });