From a2dd7f8438201373644c2ed2adbecb50c2dd0bf0 Mon Sep 17 00:00:00 2001 From: Kai Date: Mon, 10 Aug 2026 09:21:31 +0200 Subject: [PATCH] refactor(core): Pass the whole deleted batch to the post-delete workflow hook (no-changelog) (#35884) Co-authored-by: Claude Fable 5 --- .../source-control-import.service.ee.test.ts | 23 +++++++++++-------- .../source-control-import.service.ee.ts | 9 ++++---- ...workflow-review-auto-close.service.test.ts | 23 ++++++++++++++----- .../workflow-review-auto-close.service.ts | 13 ++++++----- ...kflow-mutation-hooks-proxy.service.test.ts | 9 +++++--- .../__tests__/workflow.service.test.ts | 12 +++++----- .../workflow-mutation-hooks-proxy.service.ts | 11 +++++---- .../cli/src/workflows/workflow.service.ts | 2 +- 8 files changed, 60 insertions(+), 42 deletions(-) diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts index 9ac44ccaee4..7f8c75401b5 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts @@ -2980,7 +2980,7 @@ describe('SourceControlImportService', () => { expect(folderRepository.delete).not.toHaveBeenCalled(); }); - it('should fire the afterWorkflowDeleted sweep once, after the folder row delete', async () => { + it('should fire the afterWorkflowsDeleted sweep once, after the folder row delete', async () => { const candidates = [mock({ id: 'folder1' })]; const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: false }); folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); @@ -2989,11 +2989,11 @@ describe('SourceControlImportService', () => { await service.deleteFoldersNotInWorkfolder(candidates as any); - expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledTimes(1); - expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledWith('wf-1'); + expect(workflowMutationHooks.afterWorkflowsDeleted).toHaveBeenCalledTimes(1); + expect(workflowMutationHooks.afterWorkflowsDeleted).toHaveBeenCalledWith(['wf-1']); // Only the row delete cascades the workflows away, so the sweep must run after it expect( - workflowMutationHooks.afterWorkflowDeleted.mock.invocationCallOrder[0], + workflowMutationHooks.afterWorkflowsDeleted.mock.invocationCallOrder[0], ).toBeGreaterThan(folderRepository.delete.mock.invocationCallOrder[0]); }); @@ -3004,7 +3004,7 @@ describe('SourceControlImportService', () => { await service.deleteFoldersNotInWorkfolder(candidates as any); - expect(workflowMutationHooks.afterWorkflowDeleted).not.toHaveBeenCalled(); + expect(workflowMutationHooks.afterWorkflowsDeleted).not.toHaveBeenCalled(); }); }); }); @@ -3640,7 +3640,7 @@ describe('SourceControlImportService', () => { expect(projectRepository.delete).not.toHaveBeenCalled(); }); - it('should fire the afterWorkflowDeleted sweep once, after the project row delete', async () => { + it('should fire the afterWorkflowsDeleted sweep once for the whole batch, after the project row delete', async () => { const candidates = [mock({ id: 'project-1' })]; sharedWorkflowRepository.find.mockResolvedValueOnce([ { workflowId: 'wf-active' }, @@ -3657,10 +3657,13 @@ describe('SourceControlImportService', () => { await service.deleteTeamProjectsNotInWorkfolder(candidates); // The sweep searches globally for orphaned requests, so one call covers the batch - expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledTimes(1); - expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledWith('wf-active'); + expect(workflowMutationHooks.afterWorkflowsDeleted).toHaveBeenCalledTimes(1); + expect(workflowMutationHooks.afterWorkflowsDeleted).toHaveBeenCalledWith([ + 'wf-active', + 'wf-inactive', + ]); expect( - workflowMutationHooks.afterWorkflowDeleted.mock.invocationCallOrder[0], + workflowMutationHooks.afterWorkflowsDeleted.mock.invocationCallOrder[0], ).toBeGreaterThan(projectRepository.delete.mock.invocationCallOrder[0]); }); @@ -3670,7 +3673,7 @@ describe('SourceControlImportService', () => { await service.deleteTeamProjectsNotInWorkfolder(candidates); - expect(workflowMutationHooks.afterWorkflowDeleted).not.toHaveBeenCalled(); + expect(workflowMutationHooks.afterWorkflowsDeleted).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts index af6c6a695d4..668a54962a7 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts @@ -1858,7 +1858,7 @@ export class SourceControlImportService { * regardless — so we do the physical cleanup directly beforehand. The * `beforeWorkflowDeleted` mutation hook fires here so modules can run their * pre-delete side effects (e.g. closing open review requests), and the - * caller fires `afterWorkflowDeleted` once the cascade has run (see + * caller fires `afterWorkflowsDeleted` once the cascade has run (see * {@link sweepAfterWorkflowCascade}) — but external hooks * (`workflow.delete`/`workflow.afterDelete`) and the `workflow-deleted` * event still don't fire, a pre-existing gap for any cascade-deleted @@ -1898,15 +1898,14 @@ export class SourceControlImportService { } /** - * Fire `afterWorkflowDeleted` once the folder/project row delete has + * Fire `afterWorkflowsDeleted` once the folder/project row delete has * cascaded the given workflows away, mirroring `WorkflowService.delete`: * the sweep behind the hook closes review requests opened after the - * pre-delete hooks ran and now left without a workflow. It searches - * globally for such orphans, so one call covers the whole batch. + * pre-delete hooks ran and now left without a workflow. */ private async sweepAfterWorkflowCascade(cascadedWorkflowIds: string[]) { if (cascadedWorkflowIds.length === 0) return; - await this.workflowMutationHooks.afterWorkflowDeleted(cascadedWorkflowIds[0]); + await this.workflowMutationHooks.afterWorkflowsDeleted(cascadedWorkflowIds); } /** Contextual error for a failed deletion during pull, so the operator learns which resource to look at. */ 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 index 81cfbf6e051..543067cc2ee 100644 --- 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 @@ -132,23 +132,31 @@ describe('WorkflowReviewAutoCloseService', () => { expect(logger.error).toHaveBeenCalled(); }); - describe('afterWorkflowDeleted', () => { + describe('afterWorkflowsDeleted', () => { it('closes the requests the delete orphaned, outside any lock', async () => { requestRepository.closeOrphanedOpenRequests.mockResolvedValue(['req-9']); - await service.afterWorkflowDeleted('wf-1'); + await service.afterWorkflowsDeleted(['wf-1', 'wf-2']); // 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. + // The sweep is global, so the batch never reaches the query; it is log context only. expect(requestRepository.closeOrphanedOpenRequests).toHaveBeenCalledExactlyOnceWith({}); expect(dbLockService.withLockContext).not.toHaveBeenCalled(); - expect(logger.info).toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledExactlyOnceWith( + expect.any(String), + expect.objectContaining({ + reason: 'workflow-deleted', + workflowIds: ['wf-1', 'wf-2'], + workflowReviewRequestIds: ['req-9'], + }), + ); }); it('stays quiet when the delete orphaned nothing', async () => { requestRepository.closeOrphanedOpenRequests.mockResolvedValue([]); - await service.afterWorkflowDeleted('wf-1'); + await service.afterWorkflowsDeleted(['wf-1']); expect(logger.info).not.toHaveBeenCalled(); }); @@ -157,9 +165,12 @@ describe('WorkflowReviewAutoCloseService', () => { 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(); + await expect(service.afterWorkflowsDeleted(['wf-1', 'wf-2'])).resolves.toBeUndefined(); - expect(logger.error).toHaveBeenCalled(); + expect(logger.error).toHaveBeenCalledExactlyOnceWith( + expect.any(String), + expect.objectContaining({ workflowIds: ['wf-1', 'wf-2'] }), + ); }); }); 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 index c3b748c4849..86448b32c0d 100644 --- 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 @@ -42,11 +42,12 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { } /** - * 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. + * Closes reviews whose workflow is gone: the cascade took their link rows, so they + * can only be found by "open with nothing linked", not by workflow id — the sweep is + * global and one call covers the whole batch. Catches reviews opened after the + * pre-delete hooks ran, and any left by a delete that skips the hooks. */ - async afterWorkflowDeleted(workflowId: string): Promise { + async afterWorkflowsDeleted(workflowIds: string[]): Promise { try { const closedRequestIds = await this.workflowReviewRequestRepository.closeOrphanedOpenRequests( {}, @@ -56,14 +57,14 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { this.logger.info('Closed workflow review request(s) left without a workflow', { reason: 'workflow-deleted', - workflowId, + workflowIds, workflowReviewRequestIds: closedRequestIds, }); } catch (error) { // The delete has already committed; failing it now would be worse than a // request that stays open until the next sweep closes it. this.logger.error('Failed to close workflow review request(s) left without a workflow', { - workflowId, + workflowIds, error, }); } 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 index 084b8fe1ce9..1af377d138d 100644 --- 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 @@ -12,7 +12,7 @@ describe('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(); + await expect(proxy.afterWorkflowsDeleted(['workflow-1'])).resolves.toBeUndefined(); }); test('forwards each hook to the registered provider', async () => { @@ -23,7 +23,7 @@ describe('WorkflowMutationHooksProxy', () => { await proxy.afterWorkflowArchived('workflow-1'); await proxy.afterWorkflowsTransferred(['workflow-1', 'workflow-2']); await proxy.beforeWorkflowDeleted('workflow-3'); - await proxy.afterWorkflowDeleted('workflow-4'); + await proxy.afterWorkflowsDeleted(['workflow-4', 'workflow-5']); expect(provider.afterWorkflowArchived).toHaveBeenCalledExactlyOnceWith('workflow-1'); expect(provider.afterWorkflowsTransferred).toHaveBeenCalledExactlyOnceWith([ @@ -31,6 +31,9 @@ describe('WorkflowMutationHooksProxy', () => { 'workflow-2', ]); expect(provider.beforeWorkflowDeleted).toHaveBeenCalledExactlyOnceWith('workflow-3'); - expect(provider.afterWorkflowDeleted).toHaveBeenCalledExactlyOnceWith('workflow-4'); + expect(provider.afterWorkflowsDeleted).toHaveBeenCalledExactlyOnceWith([ + 'workflow-4', + 'workflow-5', + ]); }); }); diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index a3578c47224..45f8601d24e 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -1847,21 +1847,21 @@ describe('WorkflowService', () => { }); // 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 () => { + test('runs the afterWorkflowsDeleted 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( + expect(workflowMutationHooksMock.afterWorkflowsDeleted).toHaveBeenCalledExactlyOnceWith([ WORKFLOW_ID, - ); + ]); expect( - workflowMutationHooksMock.afterWorkflowDeleted.mock.invocationCallOrder[0], + workflowMutationHooksMock.afterWorkflowsDeleted.mock.invocationCallOrder[0], ).toBeGreaterThan(workflowRepositoryMock.delete.mock.invocationCallOrder[0]); }); - test('does not run the afterWorkflowDeleted lifecycle hook when the deletion is aborted', async () => { + test('does not run the afterWorkflowsDeleted 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')); @@ -1870,7 +1870,7 @@ describe('WorkflowService', () => { 'db down', ); - expect(workflowMutationHooksMock.afterWorkflowDeleted).not.toHaveBeenCalled(); + expect(workflowMutationHooksMock.afterWorkflowsDeleted).not.toHaveBeenCalled(); }); test('deletes the workflow executions before the workflow itself', async () => { diff --git a/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts b/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts index 99cd1bec23a..669c524baaf 100644 --- a/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts +++ b/packages/cli/src/workflows/workflow-mutation-hooks-proxy.service.ts @@ -25,11 +25,12 @@ export interface WorkflowMutationHooks { beforeWorkflowDeleted(workflowId: string): Promise; /** - * Called once the workflow row is gone, for cleanup that can only be done + * Called once the workflow rows are 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. + * be found while the workflows still exist. One call covers a whole batch + * (e.g. a folder cascade); the ids identify what triggered the cleanup. */ - afterWorkflowDeleted(workflowId: string): Promise; + afterWorkflowsDeleted(workflowIds: string[]): Promise; } @Service() @@ -52,7 +53,7 @@ export class WorkflowMutationHooksProxy implements WorkflowMutationHooks { await this.provider?.beforeWorkflowDeleted(workflowId); } - async afterWorkflowDeleted(workflowId: string): Promise { - await this.provider?.afterWorkflowDeleted(workflowId); + async afterWorkflowsDeleted(workflowIds: string[]): Promise { + await this.provider?.afterWorkflowsDeleted(workflowIds); } } diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index 525bf2469b6..43bcbd697ab 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -1198,7 +1198,7 @@ export class WorkflowService { // 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); + await this.workflowMutationHooks.afterWorkflowsDeleted([workflowId]); this.eventService.emit('workflow-deleted', { user, workflowId, publicApi: false }); await this.externalHooks.run('workflow.afterDelete', [