diff --git a/packages/cli/src/executions/__tests__/execution-persistence.test.ts b/packages/cli/src/executions/__tests__/execution-persistence.test.ts index 98890941921..9536621459d 100644 --- a/packages/cli/src/executions/__tests__/execution-persistence.test.ts +++ b/packages/cli/src/executions/__tests__/execution-persistence.test.ts @@ -12,7 +12,7 @@ import { QueryFailedError } from '@n8n/typeorm'; import { mock } from 'jest-mock-extended'; import type { BinaryDataService, StorageConfig } from 'n8n-core'; import type { IWorkflowBase } from 'n8n-workflow'; -import { createEmptyRunExecutionData } from 'n8n-workflow'; +import { createEmptyRunExecutionData, UnexpectedError } from 'n8n-workflow'; import { DuplicateExecutionError } from '@/errors/duplicate-execution.error'; import type { DbStore } from '@/executions/execution-data/db-store'; @@ -328,6 +328,22 @@ describe('ExecutionPersistence', () => { } as unknown as Awaited>); }; + describe('condition guards', () => { + it('throws when requireStatus and requireNotCanceled are combined (both constrain status)', async () => { + const executionPersistence = createPersistenceService('db'); + + await expect( + executionPersistence.updateExistingExecution( + executionId, + { status: 'running' }, + { requireStatus: 'waiting', requireNotCanceled: true }, + ), + ).rejects.toThrow(UnexpectedError); + + expect(executionRepository.update).not.toHaveBeenCalled(); + }); + }); + describe('metadata-only updates', () => { it('should update the entity directly without touching either data store', async () => { const executionPersistence = createPersistenceService('fs'); diff --git a/packages/cli/src/executions/execution-persistence.ts b/packages/cli/src/executions/execution-persistence.ts index c2390c51433..b6667fe5032 100644 --- a/packages/cli/src/executions/execution-persistence.ts +++ b/packages/cli/src/executions/execution-persistence.ts @@ -12,6 +12,7 @@ import { ExecutionEntity, ExecutionRepository, Not } from '@n8n/db'; import { Service } from '@n8n/di'; import { stringify } from 'flatted'; import { BinaryDataService, StorageConfig } from 'n8n-core'; +import { UnexpectedError } from 'n8n-workflow'; import { DbStore } from './execution-data/db-store'; import { FsStore } from './execution-data/fs-store'; @@ -246,16 +247,16 @@ export class ExecutionPersistence { executionId: string, conditions?: UpdateExecutionConditions, ): FindOptionsWhere { + if (conditions?.requireStatus && conditions?.requireNotCanceled) { + throw new UnexpectedError('`requireStatus` and `requireNotCanceled` cannot be combined'); + } + const where: FindOptionsWhere = { id: executionId }; if (conditions?.requireStatus) where.status = conditions.requireStatus; // TODO(CAT-3214): `ExecutionEntity.finished` is deprecated and we should rely on statuses // only, but for now we still use it to filter out finished executions for parity with // ExecutionRepository. if (conditions?.requireNotFinished) where.finished = false; - // TODO(CAT-3215): `requireStatus` and `requireNotCanceled` both write to `where.status`, - // so if both are supplied the `Not('canceled')` clause silently overwrites the specific - // status check. In practice callers never combine them, so once we drop strict parity with - // ExecutionRepository we should assert their mutual exclusivity (or combine them somehow). if (conditions?.requireNotCanceled) where.status = Not('canceled'); return where; }