From 96dfb64ae17ec33b3d375ea007f58291c6979a0a Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 7 Jul 2026 16:35:19 -0400 Subject: [PATCH] fix(isms): serialize approve against ALL register/content edits (central lock) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the approve-race fix: cubic correctly noted the advisory lock only protected paths that also take it, so a register UPDATE/DELETE (or narrative / control / regenerate) could still interleave while approve() loads and freezes the version, leaving an approved version that omits the edit — or an edit that never invalidated the approval. Enforced centrally instead of per-call-site: invalidateApprovalIfNeeded now takes the per-document advisory lock as its first action. Every content-mutation path already calls it (register create/update/delete, narrative save, control add/remove, regenerate), so all edits now serialize against approve() (which takes the same lock). The lock is transaction-scoped and re-entrant, so create paths that already acquire it for position allocation are unaffected. Adds utils/approval.spec.ts (lock-before-status ordering + revert/no-op behavior) and $executeRaw to the two tx mocks that exercise the real invalidate. Tests: 324 api ISMS tests passing; typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp --- .../isms-document-control.service.spec.ts | 2 + .../src/isms/isms-narrative.service.spec.ts | 2 + apps/api/src/isms/utils/approval.spec.ts | 62 +++++++++++++++++++ apps/api/src/isms/utils/approval.ts | 12 ++++ 4 files changed, 78 insertions(+) create mode 100644 apps/api/src/isms/utils/approval.spec.ts diff --git a/apps/api/src/isms/isms-document-control.service.spec.ts b/apps/api/src/isms/isms-document-control.service.spec.ts index c91cb7c11d..b209192048 100644 --- a/apps/api/src/isms/isms-document-control.service.spec.ts +++ b/apps/api/src/isms/isms-document-control.service.spec.ts @@ -14,6 +14,8 @@ jest.mock('@db', () => { createMany: jest.fn(), deleteMany: jest.fn(), }, + // Advisory lock taken by invalidateApprovalIfNeeded (serializes vs approve). + $executeRaw: jest.fn(), // Run the callback with the same mock as the transaction client. $transaction: jest.fn((cb: (tx: unknown) => unknown) => cb(db)), }; diff --git a/apps/api/src/isms/isms-narrative.service.spec.ts b/apps/api/src/isms/isms-narrative.service.spec.ts index 2770acd489..c1deacca14 100644 --- a/apps/api/src/isms/isms-narrative.service.spec.ts +++ b/apps/api/src/isms/isms-narrative.service.spec.ts @@ -9,6 +9,8 @@ jest.mock('@db', () => { findUnique: jest.fn(), update: jest.fn(), }, + // Advisory lock taken by invalidateApprovalIfNeeded (serializes vs approve). + $executeRaw: jest.fn(), // Run the callback with the same mock as the transaction client. $transaction: jest.fn((cb: (tx: unknown) => unknown) => cb(db)), }; diff --git a/apps/api/src/isms/utils/approval.spec.ts b/apps/api/src/isms/utils/approval.spec.ts new file mode 100644 index 0000000000..9ca4a07801 --- /dev/null +++ b/apps/api/src/isms/utils/approval.spec.ts @@ -0,0 +1,62 @@ +import { invalidateApprovalIfNeeded } from './approval'; + +/** + * invalidateApprovalIfNeeded is the central serialization point: every ISMS + * content-mutation path calls it, and it must take the per-document advisory lock + * (so edits serialize against approve()) BEFORE reading/altering status. + */ +function makeTx() { + return { + $executeRaw: jest.fn().mockResolvedValue(0), + ismsDocument: { + findUnique: jest.fn(), + update: jest.fn().mockResolvedValue({}), + }, + }; +} + +describe('invalidateApprovalIfNeeded', () => { + it('acquires the per-document advisory lock before reading status', async () => { + const tx = makeTx(); + tx.ismsDocument.findUnique.mockResolvedValue({ status: 'draft' }); + + await invalidateApprovalIfNeeded({ + tx: tx as never, + documentId: 'doc_1', + }); + + expect(tx.$executeRaw).toHaveBeenCalledTimes(1); + // Lock is taken before the status read (serializes vs approve()). + expect(tx.$executeRaw.mock.invocationCallOrder[0]).toBeLessThan( + tx.ismsDocument.findUnique.mock.invocationCallOrder[0], + ); + }); + + it('reverts an approved document to draft (clearing approver + approvedAt)', async () => { + const tx = makeTx(); + tx.ismsDocument.findUnique.mockResolvedValue({ status: 'approved' }); + + await invalidateApprovalIfNeeded({ + tx: tx as never, + documentId: 'doc_1', + }); + + expect(tx.ismsDocument.update).toHaveBeenCalledWith({ + where: { id: 'doc_1' }, + data: { status: 'draft', approvedAt: null, approverId: null }, + }); + }); + + it('is a no-op for a non-approved document (but still locked)', async () => { + const tx = makeTx(); + tx.ismsDocument.findUnique.mockResolvedValue({ status: 'needs_review' }); + + await invalidateApprovalIfNeeded({ + tx: tx as never, + documentId: 'doc_1', + }); + + expect(tx.$executeRaw).toHaveBeenCalledTimes(1); + expect(tx.ismsDocument.update).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/isms/utils/approval.ts b/apps/api/src/isms/utils/approval.ts index 186bfcae04..6b0db62544 100644 --- a/apps/api/src/isms/utils/approval.ts +++ b/apps/api/src/isms/utils/approval.ts @@ -1,4 +1,5 @@ import type { Prisma } from '@db'; +import { lockDocument } from './document-lock'; /** * Editing an approved ISMS document invalidates its sign-off: revert it to draft @@ -6,6 +7,15 @@ import type { Prisma } from '@db'; * touches the document when it is currently `approved`; a no-op otherwise. Must * run inside the same transaction as the content write so a failed write does not * leave the document reverted to draft without the new content. + * + * Every content-mutation path (register create/update/delete, narrative save, + * control link add/remove, regenerate) calls this, so acquiring the per-document + * advisory lock here centrally serializes ALL edits against approve() — which + * takes the same lock. Without it, an edit could interleave while approve() is + * loading + freezing the version under READ COMMITTED, leaving an approved version + * that omits the edit (or an edit that never invalidated the approval). The lock + * is transaction-scoped and re-entrant, so create paths that already take it for + * position allocation are unaffected. */ export async function invalidateApprovalIfNeeded({ tx, @@ -14,6 +24,8 @@ export async function invalidateApprovalIfNeeded({ tx: Prisma.TransactionClient; documentId: string; }): Promise { + await lockDocument(tx, documentId); + const document = await tx.ismsDocument.findUnique({ where: { id: documentId }, select: { status: true },