test(core): Pin accepted review/publish race outcomes (no-changelog) (#36982)

packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts

@@ -25,6 +25,8 @@ import { v4 as uuid } from 'uuid';
 import { ActiveWorkflowManager } from '@/active-workflow-manager';
 import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service';
 import { WorkflowValidationService } from '@/workflows/workflow-validation.service';
+import { WorkflowService } from '@/workflows/workflow.service';
+import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee';
 import { createAdmin, createMember, createOwner, createUser } from '@test-integration/db/users';
 import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history';
 import type { SuperAgentTest } from '@test-integration/types';
@@ -932,6 +934,147 @@ describe('publishing a workflow under review', () => {
 	});
 });
 
+/**
+ * The approval commits under the review lock, but auto-publish runs after it is
+ * released. Workflow mutations are deliberately not serialized behind that lock,
+ * so they can land in the gap. These pin the accepted outcomes: the approval
+ * stands and the publish that lost the race is reported as a failure.
+ */
+describe('a workflow mutation racing the auto-publish of an approval', () => {
+	/** Run `raceAction` in the gap between the committed approval and auto-publish. */
+	function raceBeforeAutoPublish(raceAction: () => Promise<unknown>) {
+		const workflowService = Container.get(WorkflowService);
+		const activate = workflowService.activateWorkflow.bind(workflowService);
+		vi.spyOn(workflowService, 'activateWorkflow').mockImplementationOnce(async (...args) => {
+			await raceAction();
+			return await activate(...args);
+		});
+	}
+
+	test('an archive that lands in the gap leaves an approved review and a failed auto-publish', async () => {
+		const { workflow, versionId } = await createReviewableWorkflow();
+		const request = await createOpenReview(workflow.id, versionId);
+
+		raceBeforeAutoPublish(
+			async () => await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200),
+		);
+
+		const response = await ownerAgent
+			.post(`/workflow-review-requests/${request.id}/decision`)
+			.send({ decision: 'approved' })
+			.expect(200);
+
+		expect(response.body.data).toMatchObject({
+			state: 'closed',
+			decision: 'approved',
+			autoPublish: { status: 'failed', message: 'Cannot activate an archived workflow.' },
+		});
+
+		// The archive found the review already closed, so it left the approval alone.
+		const closed = await requestRepository.findById(request.id, {});
+		expect(closed).toMatchObject({ state: 'closed', decision: 'approved' });
+
+		const archived = await workflowEntityRepository.findOneByOrFail({ id: workflow.id });
+		expect(archived.isArchived).toBe(true);
+		expect(archived.activeVersionId).toBeNull();
+
+		// Publishing stays blocked by the archival itself, not by the closed review.
+		await ownerAgent.post(`/workflows/${workflow.id}/activate`).send({ versionId }).expect(400);
+	});
+
+	test('a transfer that lands in the gap leaves an approved review and a failed auto-publish', async () => {
+		const versionId = uuid();
+		const workflow = await createWorkflow({}, teamProject);
+		await createWorkflowHistoryItem(workflow.id, { versionId });
+
+		// The requester publishes on approval, so it must be someone who loses access
+		// when the workflow moves — the deciding owner never does.
+		const request = await requestRepository.createRequest(
+			{ projectId: teamProject.id, title: 'Review before publishing', createdById: member.id },
+			{},
+		);
+		await workflowRepository.createWorkflowRow(
+			{
+				workflowReviewRequestId: request.id,
+				workflowId: workflow.id,
+				workflowVersionId: versionId,
+			},
+			{},
+		);
+		await authorRepository.addAuthor(
+			{ workflowReviewRequestId: request.id, userId: member.id },
+			{},
+		);
+
+		const destination = await createTeamProject('Elsewhere', await createMember());
+		raceBeforeAutoPublish(
+			async () =>
+				await Container.get(EnterpriseWorkflowService).transferWorkflow(
+					owner,
+					workflow.id,
+					destination.id,
+				),
+		);
+
+		const response = await ownerAgent
+			.post(`/workflow-review-requests/${request.id}/decision`)
+			.send({ decision: 'approved' })
+			.expect(200);
+
+		expect(response.body.data).toMatchObject({
+			state: 'closed',
+			decision: 'approved',
+			autoPublish: {
+				status: 'failed',
+				message:
+					'You do not have permission to activate this workflow. Ask the owner to share it with you.',
+			},
+		});
+
+		expect(await requestRepository.findById(request.id, {})).toMatchObject({
+			state: 'closed',
+			decision: 'approved',
+		});
+		expect(
+			(await workflowEntityRepository.findOneByOrFail({ id: workflow.id })).activeVersionId,
+		).toBeNull();
+	});
+
+	test('a review created in the gap blocks the auto-publish without failing the decision', async () => {
+		const { workflow, versionId } = await createReviewableWorkflow();
+		const request = await createOpenReview(workflow.id, versionId);
+
+		let racingRequestId = '';
+		raceBeforeAutoPublish(async () => {
+			racingRequestId = (await createOpenReview(workflow.id, versionId)).id;
+		});
+
+		const response = await ownerAgent
+			.post(`/workflow-review-requests/${request.id}/decision`)
+			.send({ decision: 'approved' })
+			.expect(200);
+
+		expect(response.body.data).toMatchObject({
+			state: 'closed',
+			decision: 'approved',
+			autoPublish: {
+				status: 'failed',
+				message:
+					"Workflow can't be published while its review is open. Submit this version to the review, or wait for the review to close.",
+			},
+		});
+
+		// The new review is untouched and still guards the workflow.
+		expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toMatchObject({
+			id: racingRequestId,
+			state: 'open',
+		});
+		expect(
+			(await workflowEntityRepository.findOneByOrFail({ id: workflow.id })).activeVersionId,
+		).toBeNull();
+	});
+});
+
 describe('POST /workflow-review-requests/:workflowReviewRequestId/update-version', () => {
 	/** Seed an open review request pinned to `versionId`, authored by `author`. */
 	async function seedOpenRequest(

packages/cli/src/workflows/workflow.service.ts

@@ -886,6 +886,13 @@ export class WorkflowService {
 
 		// re-applying the already-published version (e.g. a settings-only update)
 		// publishes no new version, so the review gate must not block it.
+		//
+		// This check is deliberately not serialized with review mutations: putting
+		// publishing behind the review feature's global lock would slow down a core
+		// workflow operation for every instance. A review opened just after this
+		// passes, or just before an approval's auto-publish reaches it, therefore
+		// races — accepted, because both outcomes degrade gracefully (the approval
+		// stands and auto-publish reports `failed`).`.
 		if (versionIdToActivate !== previousActiveVersionId) {
 			await this.workflowPublishGuard.assertCanPublish(workflowId);
 		}