From bb9b5bb1a3c09a089589a96baac31ec10d251fb6 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 13 May 2026 00:07:08 +0200 Subject: [PATCH] fix(audit-wave-1): orphan-blob window in handleDocumentCompleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes Wave 1.3 (CRITICAL). The previous storage.put → files.insert → documents.update sequence had two real failure modes: 1. **Orphan blob.** If storage.put succeeded but the files.insert or documents.update failed, the blob lived forever in MinIO with no DB pointer. Re-runs re-uploaded a new blob without cleaning up the previous one. 2. **Zombie completed state.** The catch block at the end ran `documents.update({status: 'completed'})` with NO signedFileId on any failure path. The idempotency early-return at the top requires BOTH status='completed' AND signedFileId, so retries *did* still re-attempt — but reps saw a "completed" document with no signed file, hiding the failure. Fix: - Track `putStoragePath` outside the try. After storage.put lands, the variable holds the path; cleared once the DB commit succeeds. - files.insert + documents.update + reservation contract mirror all run in a single `db.transaction(...)`. Atomic commit-or-rollback. - Catch block: compensating `storage.delete(putStoragePath)` if the DB commit didn't land. Logs at error level on compensating-delete failure so a human can clean up. - Catch block no longer sets `status='completed'`. The doc stays in its prior state; Documenso's retry (or our poll-worker) re- attempts the full sequence safely thanks to the unchanged idempotency gate. Verified: tsc clean, documents-completion-auto-deposit tests all pass (5/5). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/services/documents.service.ts | 119 ++++++++++++++++++-------- 1 file changed, 81 insertions(+), 38 deletions(-) diff --git a/src/lib/services/documents.service.ts b/src/lib/services/documents.service.ts index 947a133e..0a2cc056 100644 --- a/src/lib/services/documents.service.ts +++ b/src/lib/services/documents.service.ts @@ -1116,6 +1116,12 @@ export async function handleDocumentCompleted(eventData: { documentId: string; p return; } + // Tracked outside the try so the catch can attempt a compensating + // storage.delete if we put a blob but failed to commit the DB rows. + // Without this, partial failures left an orphan blob in storage with + // no DB pointer (the audit's orphan-blob CRITICAL). + let putStoragePath: string | null = null; + try { const signedPdfBuffer = await downloadSignedPdf(eventData.documentId); @@ -1137,6 +1143,7 @@ export async function handleDocumentCompleted(eventData: { documentId: string; p contentType: 'application/pdf', sizeBytes: signedPdfBuffer.length, }); + putStoragePath = storagePath; // Resolve owner via the Owner-wins chain. The signed PDF lands in // this owner's auto-created entity subfolder (or at root if no owner). @@ -1163,40 +1170,55 @@ export async function handleDocumentCompleted(eventData: { documentId: string; p } } - const [fileRecord] = await db - .insert(files) - .values({ - portId: doc.portId, - clientId: owner?.entityType === 'client' ? owner.entityId : (doc.clientId ?? null), - companyId: owner?.entityType === 'company' ? owner.entityId : (doc.companyId ?? null), - yachtId: owner?.entityType === 'yacht' ? owner.entityId : (doc.yachtId ?? null), - folderId: entityFolderId, - filename: `signed-${doc.id}.pdf`, - originalName: `signed-${doc.id}.pdf`, - mimeType: 'application/pdf', - sizeBytes: String(signedPdfBuffer.length), - storagePath, - storageBucket: env.MINIO_BUCKET, - category: 'eoi', - uploadedBy: 'system', - }) - .returning(); + // Atomic: the files row + the documents.signedFileId pointer + the + // reservation contract mirror commit together. If any throws, the + // outer catch fires storage.delete on the orphan blob. + const fileRecord = await db.transaction(async (tx) => { + const [inserted] = await tx + .insert(files) + .values({ + portId: doc.portId, + clientId: owner?.entityType === 'client' ? owner.entityId : (doc.clientId ?? null), + companyId: owner?.entityType === 'company' ? owner.entityId : (doc.companyId ?? null), + yachtId: owner?.entityType === 'yacht' ? owner.entityId : (doc.yachtId ?? null), + folderId: entityFolderId, + filename: `signed-${doc.id}.pdf`, + originalName: `signed-${doc.id}.pdf`, + mimeType: 'application/pdf', + sizeBytes: String(signedPdfBuffer.length), + storagePath, + storageBucket: env.MINIO_BUCKET, + category: 'eoi', + uploadedBy: 'system', + }) + .returning(); - await db - .update(documents) - .set({ status: 'completed', signedFileId: fileRecord!.id, updatedAt: new Date() }) - .where(eq(documents.id, doc.id)); + if (!inserted) { + throw new Error('files.insert returned no row'); + } - // Reservation agreements mirror their signed PDF onto - // berth_reservations.contractFileId so the portal "My Reservations" view - // can resolve the contract without joining through documents. - if (doc.documentType === 'reservation_agreement' && doc.reservationId) { - const { berthReservations } = await import('@/lib/db/schema/reservations'); - await db - .update(berthReservations) - .set({ contractFileId: fileRecord!.id, updatedAt: new Date() }) - .where(eq(berthReservations.id, doc.reservationId)); - } + await tx + .update(documents) + .set({ status: 'completed', signedFileId: inserted.id, updatedAt: new Date() }) + .where(eq(documents.id, doc.id)); + + // Reservation agreements mirror their signed PDF onto + // berth_reservations.contractFileId so the portal "My Reservations" view + // can resolve the contract without joining through documents. + if (doc.documentType === 'reservation_agreement' && doc.reservationId) { + const { berthReservations } = await import('@/lib/db/schema/reservations'); + await tx + .update(berthReservations) + .set({ contractFileId: inserted.id, updatedAt: new Date() }) + .where(eq(berthReservations.id, doc.reservationId)); + } + + return inserted; + }); + + // Mark as durably committed BEFORE side effects so the catch block + // doesn't try to undo a blob whose DB pointer just landed. + putStoragePath = null; // Audit log: the webhook just minted a new signed-PDF file row owned by // 'system'. Without this entry the file appears in the aggregated view @@ -1206,9 +1228,9 @@ export async function handleDocumentCompleted(eventData: { documentId: string; p portId: doc.portId, action: 'create', entityType: 'file', - entityId: fileRecord!.id, + entityId: fileRecord.id, newValue: { - filename: fileRecord!.filename, + filename: fileRecord.filename, mimeType: 'application/pdf', size: signedPdfBuffer.length, documentId: doc.id, @@ -1246,10 +1268,31 @@ export async function handleDocumentCompleted(eventData: { documentId: string; p { err, documentId: doc.id, portId: doc.portId }, 'Failed to download/store signed PDF', ); - await db - .update(documents) - .set({ status: 'completed', updatedAt: new Date() }) - .where(eq(documents.id, doc.id)); + + // Compensating delete: storage.put landed but the DB commit didn't. + // Without this the blob lives forever with no row pointing at it. + if (putStoragePath) { + try { + await (await getStorageBackend()).delete(putStoragePath); + logger.info( + { documentId: doc.id, storagePath: putStoragePath }, + 'Compensating storage.delete after failed signed-PDF commit', + ); + } catch (compErr) { + // We tried — log so a human can clean up the orphan if needed. + logger.error( + { compErr, documentId: doc.id, storagePath: putStoragePath }, + 'Compensating storage.delete also failed — manual cleanup required', + ); + } + } + + // Critical: do NOT set documents.status = 'completed' on failure. + // The previous catch block did — which created the "completed-with- + // no-signedFileId" zombie state the audit flagged. Let the next + // Documenso retry (or our poll-worker reconciliation) re-attempt; + // the early-return idempotency gate at the top requires BOTH + // status='completed' AND signedFileId so re-runs are safe. } // Update interest if eoi type