From fed347f1d653a6f73366084bf49fd407180fa92b Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 21 Jul 2026 22:30:26 -0400 Subject: [PATCH 1/2] fix(policies): delete detached PDF objects when regenerating a draft Regenerating an uploaded draft cleared its PDF references but left the objects in S3, orphaning one per regeneration. The detached keys (policy + current version, deduplicated) are now removed best-effort after the database update commits; failures are logged with their keys instead of failing the regeneration. --- .../policies/update-policy-helpers.spec.ts | 88 +++++++++++++++++++ .../trigger/policies/update-policy-helpers.ts | 46 +++++++++- 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/apps/api/src/trigger/policies/update-policy-helpers.spec.ts b/apps/api/src/trigger/policies/update-policy-helpers.spec.ts index f7ddc36ae0..2e433a67d4 100644 --- a/apps/api/src/trigger/policies/update-policy-helpers.spec.ts +++ b/apps/api/src/trigger/policies/update-policy-helpers.spec.ts @@ -31,6 +31,18 @@ jest.mock('@trigger.dev/sdk', () => ({ logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, })); +// Detached-PDF cleanup imports the S3 SDK dynamically; the mock intercepts it +// so tests can assert deletions without touching AWS. +const s3Send = jest.fn(); +jest.mock('@aws-sdk/client-s3', () => ({ + S3Client: class { + send = s3Send; + }, + DeleteObjectCommand: class { + constructor(public readonly input: { Bucket: string; Key: string }) {} + }, +})); + jest.mock('ai', () => ({ generateObject: jest.fn(), NoObjectGeneratedError: { isInstance: jest.fn(() => false) }, @@ -254,6 +266,7 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { currentVersionId: 'pv_1', displayFormat: 'PDF', pdfUrl: 'org_1/policies/pol_1/uploaded.pdf', + currentVersion: { pdfUrl: 'org_1/policies/pol_1/v1.pdf' }, content: [ { type: 'paragraph', content: [{ type: 'text', text: 'Stale draft' }] }, ], @@ -327,4 +340,79 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { const versionUpdate = txVersionUpdate.mock.calls[0][0]; expect(versionUpdate.data.pdfUrl).toBeNull(); }); + + describe('detached-PDF S3 cleanup', () => { + const ORIGINAL_BUCKET = process.env.APP_AWS_BUCKET_NAME; + + afterEach(() => { + if (ORIGINAL_BUCKET === undefined) { + delete process.env.APP_AWS_BUCKET_NAME; + } else { + process.env.APP_AWS_BUCKET_NAME = ORIGINAL_BUCKET; + } + }); + + it('deletes the detached PDF objects (deduplicated) after the update commits', async () => { + process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + + await updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'); + + const deletedKeys = s3Send.mock.calls.map( + (call) => (call[0] as { input: { Bucket: string; Key: string } }).input, + ); + expect(deletedKeys).toEqual([ + { Bucket: 'test-bucket', Key: 'org_1/policies/pol_1/uploaded.pdf' }, + { Bucket: 'test-bucket', Key: 'org_1/policies/pol_1/v1.pdf' }, + ]); + }); + + it('deduplicates when the policy and its version share one PDF key', async () => { + process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + (db.policy.findUnique as jest.Mock).mockResolvedValue({ + id: 'pol_1', + status: 'draft', + currentVersionId: 'pv_1', + pdfUrl: 'org_1/policies/pol_1/shared.pdf', + currentVersion: { pdfUrl: 'org_1/policies/pol_1/shared.pdf' }, + }); + + await updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'); + + expect(s3Send).toHaveBeenCalledTimes(1); + }); + + it('never fails the regeneration when an S3 delete fails (logged for cleanup)', async () => { + process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + s3Send.mockRejectedValueOnce(new Error('AccessDenied')); + + await expect( + updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'), + ).resolves.toBeUndefined(); + // Both keys are still attempted; the failure is logged, not thrown. + expect(s3Send).toHaveBeenCalledTimes(2); + }); + + it('skips deletion (with a warning) when no bucket is configured', async () => { + delete process.env.APP_AWS_BUCKET_NAME; + + await updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'); + + expect(s3Send).not.toHaveBeenCalled(); + }); + + it('does not touch S3 for an editor-mode draft with no PDFs', async () => { + process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + (db.policy.findUnique as jest.Mock).mockResolvedValue({ + id: 'pol_1', + status: 'draft', + currentVersionId: 'pv_1', + pdfUrl: null, + currentVersion: { pdfUrl: null }, + }); + + await updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'); + + expect(s3Send).not.toHaveBeenCalled(); + }); + }); }); diff --git a/apps/api/src/trigger/policies/update-policy-helpers.ts b/apps/api/src/trigger/policies/update-policy-helpers.ts index 3cff8397c4..0ffa284f1b 100644 --- a/apps/api/src/trigger/policies/update-policy-helpers.ts +++ b/apps/api/src/trigger/policies/update-policy-helpers.ts @@ -1,3 +1,4 @@ +import { DeleteObjectCommand, S3Client } from '@aws-sdk/client-s3'; import { db, Prisma, PolicyStatus } from '@db'; import type { FrameworkEditorFramework, @@ -71,6 +72,32 @@ export async function fetchOrganizationAndPolicy( // on the [policyId, version] key. const POLICY_VERSION_CREATE_RETRIES = 3; +/** + * Best-effort removal of the PDF objects a draft regeneration detached + * (pdfUrl stores the raw S3 key — same contract as the policies controller). + * Runs AFTER the database transaction commits so a failed delete can never + * roll back the content update; failures are logged with their keys so the + * objects can be cleaned up later instead of orphaning silently. + */ +async function deleteDetachedPdfObjects(keys: string[]): Promise { + if (keys.length === 0) return; + const bucketName = process.env.APP_AWS_BUCKET_NAME; + if (!bucketName) { + logger.warn( + `APP_AWS_BUCKET_NAME not configured; skipped deleting detached policy PDFs: ${keys.join(', ')}`, + ); + return; + } + const s3 = new S3Client({ region: process.env.AWS_REGION || 'us-east-1' }); + for (const key of keys) { + try { + await s3.send(new DeleteObjectCommand({ Bucket: bucketName, Key: key })); + } catch (error) { + logger.warn(`Failed to delete detached policy PDF ${key}: ${error}`); + } + } +} + export async function updatePolicyInDatabase( policyId: string, content: Record[], @@ -79,7 +106,13 @@ export async function updatePolicyInDatabase( try { const policy = await db.policy.findUnique({ where: { id: policyId }, - select: { id: true, status: true, currentVersionId: true }, + select: { + id: true, + status: true, + currentVersionId: true, + pdfUrl: true, + currentVersion: { select: { pdfUrl: true } }, + }, }); if (!policy) throw new Error(`Policy not found: ${policyId}`); @@ -103,6 +136,16 @@ export async function updatePolicyInDatabase( // currentVersion.pdfUrl ?? policy.pdfUrl) keep serving the old uploaded // document instead of the regenerated content (CS-766). if (policy.status === PolicyStatus.draft) { + // The uploaded-PDF keys being detached below. Captured before the + // transaction, deleted from S3 only after it commits — otherwise every + // regeneration of a PDF-mode draft orphans its old object. + const detachedPdfKeys = [ + ...new Set( + [policy.pdfUrl, policy.currentVersion?.pdfUrl].filter( + (key): key is string => !!key, + ), + ), + ]; await db.$transaction(async (tx) => { if (policy.currentVersionId) { await tx.policyVersion.update({ @@ -124,6 +167,7 @@ export async function updatePolicyInDatabase( }, }); }); + await deleteDetachedPdfObjects(detachedPdfKeys); return; } From efbc86013e79f14e9acd6e800ee44522cda533fb Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 21 Jul 2026 22:44:32 -0400 Subject: [PATCH 2/2] fix(policies): capture detached PDF keys under the row lock and use APP_AWS config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cleanup client now mirrors the other trigger tasks' APP_AWS_* configuration (region, credentials, optional endpoint) instead of the default chain, and the detached keys are captured inside the transaction after a SELECT ... FOR UPDATE on the policy row — a concurrently uploaded PDF can no longer slip past the cleanup as an untracked orphan. --- .../policies/update-policy-helpers.spec.ts | 59 ++++++++++++------- .../trigger/policies/update-policy-helpers.ts | 55 ++++++++++------- 2 files changed, 73 insertions(+), 41 deletions(-) diff --git a/apps/api/src/trigger/policies/update-policy-helpers.spec.ts b/apps/api/src/trigger/policies/update-policy-helpers.spec.ts index 2e433a67d4..e2068c34ea 100644 --- a/apps/api/src/trigger/policies/update-policy-helpers.spec.ts +++ b/apps/api/src/trigger/policies/update-policy-helpers.spec.ts @@ -251,6 +251,7 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { ]; let txPolicyUpdate: jest.Mock; + let txPolicyFind: jest.Mock; let txVersionUpdate: jest.Mock; let txVersionCreate: jest.Mock; let txVersionDeleteMany: jest.Mock; @@ -277,6 +278,11 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { }); txPolicyUpdate = jest.fn(); + // The in-transaction (row-locked) re-read that captures the detached keys. + txPolicyFind = jest.fn().mockResolvedValue({ + pdfUrl: 'org_1/policies/pol_1/uploaded.pdf', + currentVersion: { pdfUrl: 'org_1/policies/pol_1/v1.pdf' }, + }); txVersionUpdate = jest.fn(); txVersionCreate = jest.fn(() => ({ id: 'pv_2' })); txVersionDeleteMany = jest.fn(); @@ -284,7 +290,8 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { (db.$transaction as jest.Mock).mockImplementation( async (cb: (tx: unknown) => Promise) => cb({ - policy: { update: txPolicyUpdate }, + $executeRaw: jest.fn(), + policy: { update: txPolicyUpdate, findUniqueOrThrow: txPolicyFind }, policyVersion: { update: txVersionUpdate, create: txVersionCreate, @@ -342,21 +349,38 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { }); describe('detached-PDF S3 cleanup', () => { - const ORIGINAL_BUCKET = process.env.APP_AWS_BUCKET_NAME; + const ENV_KEYS = [ + 'APP_AWS_BUCKET_NAME', + 'APP_AWS_ACCESS_KEY_ID', + 'APP_AWS_SECRET_ACCESS_KEY', + ] as const; + const ORIGINAL_ENV = Object.fromEntries( + ENV_KEYS.map((key) => [key, process.env[key]]), + ); + + const configureS3Env = () => { + process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + process.env.APP_AWS_ACCESS_KEY_ID = 'test-key'; + process.env.APP_AWS_SECRET_ACCESS_KEY = 'test-secret'; + }; afterEach(() => { - if (ORIGINAL_BUCKET === undefined) { - delete process.env.APP_AWS_BUCKET_NAME; - } else { - process.env.APP_AWS_BUCKET_NAME = ORIGINAL_BUCKET; + for (const key of ENV_KEYS) { + const original = ORIGINAL_ENV[key]; + if (original === undefined) delete process.env[key]; + else process.env[key] = original; } }); - it('deletes the detached PDF objects (deduplicated) after the update commits', async () => { - process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + it('deletes the PDF keys captured inside the locked transaction', async () => { + configureS3Env(); await updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'); + // The keys come from the in-transaction re-read (not the outer snapshot), + // so a PDF uploaded concurrently before the row lock is still covered. + expect(txPolicyFind).toHaveBeenCalledTimes(1); + const deletedKeys = s3Send.mock.calls.map( (call) => (call[0] as { input: { Bucket: string; Key: string } }).input, ); @@ -367,11 +391,8 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { }); it('deduplicates when the policy and its version share one PDF key', async () => { - process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; - (db.policy.findUnique as jest.Mock).mockResolvedValue({ - id: 'pol_1', - status: 'draft', - currentVersionId: 'pv_1', + configureS3Env(); + txPolicyFind.mockResolvedValue({ pdfUrl: 'org_1/policies/pol_1/shared.pdf', currentVersion: { pdfUrl: 'org_1/policies/pol_1/shared.pdf' }, }); @@ -382,7 +403,7 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { }); it('never fails the regeneration when an S3 delete fails (logged for cleanup)', async () => { - process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; + configureS3Env(); s3Send.mockRejectedValueOnce(new Error('AccessDenied')); await expect( @@ -392,7 +413,8 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { expect(s3Send).toHaveBeenCalledTimes(2); }); - it('skips deletion (with a warning) when no bucket is configured', async () => { + it('skips deletion (with a warning) when the S3 configuration is missing', async () => { + configureS3Env(); delete process.env.APP_AWS_BUCKET_NAME; await updatePolicyInDatabase('pol_1', REGEN_CONTENT, 'mem_regen'); @@ -401,11 +423,8 @@ describe('updatePolicyInDatabase (draft policy regeneration)', () => { }); it('does not touch S3 for an editor-mode draft with no PDFs', async () => { - process.env.APP_AWS_BUCKET_NAME = 'test-bucket'; - (db.policy.findUnique as jest.Mock).mockResolvedValue({ - id: 'pol_1', - status: 'draft', - currentVersionId: 'pv_1', + configureS3Env(); + txPolicyFind.mockResolvedValue({ pdfUrl: null, currentVersion: { pdfUrl: null }, }); diff --git a/apps/api/src/trigger/policies/update-policy-helpers.ts b/apps/api/src/trigger/policies/update-policy-helpers.ts index 0ffa284f1b..fb42f31380 100644 --- a/apps/api/src/trigger/policies/update-policy-helpers.ts +++ b/apps/api/src/trigger/policies/update-policy-helpers.ts @@ -81,14 +81,25 @@ const POLICY_VERSION_CREATE_RETRIES = 3; */ async function deleteDetachedPdfObjects(keys: string[]): Promise { if (keys.length === 0) return; + // Same APP_AWS_* configuration as the other trigger tasks (evidence export) + // — but non-throwing: cleanup is best-effort and must never fail the + // regeneration, so missing configuration is logged and skipped. const bucketName = process.env.APP_AWS_BUCKET_NAME; - if (!bucketName) { + const accessKeyId = process.env.APP_AWS_ACCESS_KEY_ID; + const secretAccessKey = process.env.APP_AWS_SECRET_ACCESS_KEY; + if (!bucketName || !accessKeyId || !secretAccessKey) { logger.warn( - `APP_AWS_BUCKET_NAME not configured; skipped deleting detached policy PDFs: ${keys.join(', ')}`, + `APP_AWS_* S3 configuration missing; skipped deleting detached policy PDFs: ${keys.join(', ')}`, ); return; } - const s3 = new S3Client({ region: process.env.AWS_REGION || 'us-east-1' }); + const s3 = new S3Client({ + region: process.env.APP_AWS_REGION || 'us-east-1', + credentials: { accessKeyId, secretAccessKey }, + ...(process.env.APP_AWS_ENDPOINT + ? { endpoint: process.env.APP_AWS_ENDPOINT, forcePathStyle: true } + : {}), + }); for (const key of keys) { try { await s3.send(new DeleteObjectCommand({ Bucket: bucketName, Key: key })); @@ -106,13 +117,7 @@ export async function updatePolicyInDatabase( try { const policy = await db.policy.findUnique({ where: { id: policyId }, - select: { - id: true, - status: true, - currentVersionId: true, - pdfUrl: true, - currentVersion: { select: { pdfUrl: true } }, - }, + select: { id: true, status: true, currentVersionId: true }, }); if (!policy) throw new Error(`Policy not found: ${policyId}`); @@ -136,17 +141,18 @@ export async function updatePolicyInDatabase( // currentVersion.pdfUrl ?? policy.pdfUrl) keep serving the old uploaded // document instead of the regenerated content (CS-766). if (policy.status === PolicyStatus.draft) { - // The uploaded-PDF keys being detached below. Captured before the - // transaction, deleted from S3 only after it commits — otherwise every - // regeneration of a PDF-mode draft orphans its old object. - const detachedPdfKeys = [ - ...new Set( - [policy.pdfUrl, policy.currentVersion?.pdfUrl].filter( - (key): key is string => !!key, - ), - ), - ]; - await db.$transaction(async (tx) => { + // The uploaded-PDF keys this regeneration detaches, captured INSIDE the + // transaction under a row lock: a concurrent PDF upload (a plain UPDATE + // on the policy row) blocks on the lock, so the captured keys are + // exactly the values the updates below clear — nothing committed in + // between can slip an untracked orphan past the cleanup. Deleted from + // S3 only after the transaction commits. + const detachedPdfKeys = await db.$transaction(async (tx) => { + await tx.$executeRaw`SELECT id FROM "Policy" WHERE id = ${policyId} FOR UPDATE`; + const current = await tx.policy.findUniqueOrThrow({ + where: { id: policyId }, + select: { pdfUrl: true, currentVersion: { select: { pdfUrl: true } } }, + }); if (policy.currentVersionId) { await tx.policyVersion.update({ where: { id: policy.currentVersionId }, @@ -166,6 +172,13 @@ export async function updatePolicyInDatabase( displayFormat: 'EDITOR', }, }); + return [ + ...new Set( + [current.pdfUrl, current.currentVersion?.pdfUrl].filter( + (key): key is string => !!key, + ), + ), + ]; }); await deleteDetachedPdfObjects(detachedPdfKeys); return;