fix(uploads): make execution attachment completion replay-safe (#6601)

waleedlatif1 · web-flow · commit 0d640aabbcd9 · 2026-08-11T22:01:19.000-07:00
finalizeExecutionAttachment reported a completedFileId. That marker is what
routes a replayed completion into loadCompletedUploadPurpose, which handled
only workspace_file and threw a bare Error otherwise -- unclassified, so the
route rendered a generic 500. Its structurally identical twin,
finalizeMothershipAttachment, correctly reports nothing.

Both are metadata-backed and idempotent by storage key, exactly as the
finalizeUploadPurpose TSDoc already states, so neither needs the marker: their
replays are correct through the finalizer itself. Drop it from the execution
finalizer so the two twins agree.

loadCompletedUploadPurpose becomes an exhaustive switch, matching the sibling
finalizeUploadPurpose switch, so adding a purpose is a compile error until its
replay behavior is decided rather than a runtime 500. The residual arm throws a
classified UploadSessionError('internal') instead of a bare Error.

markUploadSessionCompleted no longer clears a marker it was not given. A
finalizer that records one inside its own registration transaction --
markUploadSessionFileRegistered does this for workspace_file -- would otherwise
have it overwritten with null, and both the abort guard and the expiry sweep
key on it: cleanupExpiredUploadSessions only treats a finalizing session as
disposable when completedFileId is null. This is a no-op for every current
path, since markUploadSessionCompleted moves the session to completed, which is
neither abortable nor a cleanup candidate.

Latent only. No shipped client replays a completion: the sole producer,
uploadWorkflowAttachments, mints a fresh session per file and never retries,
requestJson does not retry, and a concurrent double-submit is already a clean
409 from claimSession.

Tests pin the invariant rather than the symptom: a Record over the purpose union
is a compile-time gate on which route each purpose replays through, and the
cases assert that idempotent purposes report no marker and reject cleanly if
they ever reach the loader.
diff --git a/apps/sim/app/api/files/uploads/finalizers.test.ts b/apps/sim/app/api/files/uploads/finalizers.test.ts
@@ -69,7 +69,11 @@ vi.mock('@/lib/users/queries', () => ({
   requireResolvedUserEmail: (emails: Map<string, string>, userId: string) => emails.get(userId)!,
 }))
 
-import { finalizeUploadPurpose } from '@/app/api/files/uploads/finalizers'
+import {
+  finalizeUploadPurpose,
+  loadCompletedUploadPurpose,
+} from '@/app/api/files/uploads/finalizers'
+import type { InternalUploadPurpose } from '@/app/api/files/uploads/purposes'
 
 const now = new Date('2026-08-04T12:00:00.000Z')
 const actor = { id: 'user-1', name: 'Ada', email: 'ada@example.com' }
@@ -138,6 +142,70 @@ const workspaceFile = {
   updatedAt: now,
 }
 
+/**
+ * How each purpose replays a completion. `Record` over the union is a
+ * compile-time completeness gate: adding a purpose fails to build until its
+ * replay behavior is declared here.
+ */
+const REPLAY_ROUTE: Record<InternalUploadPurpose, 'loader' | 'idempotent-finalizer'> = {
+  workspace_file: 'loader',
+  profile_picture: 'idempotent-finalizer',
+  workspace_logo: 'idempotent-finalizer',
+  mothership_attachment: 'idempotent-finalizer',
+  execution_attachment: 'idempotent-finalizer',
+}
+
+const purposesReplayedBy = (route: 'loader' | 'idempotent-finalizer') =>
+  (Object.keys(REPLAY_ROUTE) as InternalUploadPurpose[]).filter((p) => REPLAY_ROUTE[p] === route)
+
+describe('completion replay contract', () => {
+  const REPLAY_VIA_IDEMPOTENT_FINALIZER = purposesReplayedBy('idempotent-finalizer')
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it.each(REPLAY_VIA_IDEMPOTENT_FINALIZER)(
+    'does not mark %s as loader-backed, so its replay re-runs the idempotent finalizer',
+    async (purpose) => {
+      mockSelectLimit.mockResolvedValue([])
+      mockInsertReturning.mockResolvedValue([metadataRow])
+      const request = new NextRequest('http://localhost/api/files/uploads/upload-1/complete')
+
+      const finalized = await finalizeUploadPurpose({
+        session: { ...uploadSession, purpose },
+        actor,
+        principal,
+        request,
+      })
+
+      expect(finalized.completedFileId).toBeUndefined()
+    }
+  )
+
+  it.each(REPLAY_VIA_IDEMPOTENT_FINALIZER)(
+    'reports a classified error rather than an unhandled crash if %s ever reaches the loader',
+    async (purpose) => {
+      await expect(loadCompletedUploadPurpose({ ...uploadSession, purpose })).rejects.toMatchObject(
+        { code: 'internal' }
+      )
+    }
+  )
+
+  it('loads workspace_file from its durable record on replay', async () => {
+    mockGetWorkspaceFile.mockResolvedValueOnce(workspaceFile)
+
+    const loaded = await loadCompletedUploadPurpose({
+      ...uploadSession,
+      purpose: purposesReplayedBy('loader')[0],
+      completedFileId: workspaceFile.id,
+    })
+
+    expect(loaded).toMatchObject({ id: workspaceFile.id })
+    expect(mockGetWorkspaceFile).toHaveBeenCalledTimes(1)
+  })
+})
+
 describe('upload purpose finalizers', () => {
   beforeEach(() => {
     vi.clearAllMocks()
diff --git a/apps/sim/app/api/files/uploads/finalizers.ts b/apps/sim/app/api/files/uploads/finalizers.ts
@@ -59,6 +59,14 @@ interface FinalizeUploadPurposeParams {
 
 interface FinalizedUploadPurpose {
   value: UploadPurposeResult
+  /**
+   * Recorded on the session so a replayed completion returns the original
+   * result instead of re-running a finalizer with one-time side effects.
+   *
+   * Set only for a purpose {@link loadCompletedUploadPurpose} can reload; an
+   * already-idempotent finalizer must leave it undefined so replays flow back
+   * through the finalizer itself.
+   */
   completedFileId?: string
 }
 
@@ -111,13 +119,30 @@ export async function finalizeUploadPurpose({
   }
 }
 
+/**
+ * Reloads the durable result of an already-completed session, for the purposes
+ * that report a {@link FinalizedUploadPurpose.completedFileId}.
+ *
+ * The switch is exhaustive so that adding a purpose is a compile error until
+ * its replay behavior is decided here.
+ */
 export async function loadCompletedUploadPurpose(
   session: UploadSessionRecord
 ): Promise<UploadPurposeResult> {
-  if (session.purpose !== 'workspace_file') {
-    throw new Error(`Upload purpose ${session.purpose} has no durable file result`)
+  switch (session.purpose) {
+    case 'workspace_file':
+      return toV2File(await loadCompletedWorkspaceFileUpload(session))
+    case 'profile_picture':
+    case 'workspace_logo':
+    case 'mothership_attachment':
+    case 'execution_attachment':
+    case 'table_import':
+    case 'knowledge_document':
+      throw new UploadSessionError(
+        'internal',
+        `Upload purpose ${session.purpose} recorded a completed file but has no durable loader`
+      )
   }
-  return toV2File(await loadCompletedWorkspaceFileUpload(session))
 }
 
 async function finalizeInternalWorkspaceFile(
@@ -314,7 +339,6 @@ async function finalizeExecutionAttachment(
       key: session.storageKey,
       context: 'execution',
     },
-    completedFileId: finalized.file.id,
   }
 }
 
diff --git a/apps/sim/lib/uploads/upload-session/service.ts b/apps/sim/lib/uploads/upload-session/service.ts
@@ -701,6 +701,13 @@ export async function completeUploadSession<T>(params: {
   }
 }
 
+/**
+ * The completion marker is only written when the finalizer reports one. A
+ * finalizer that instead records it inside its own registration transaction —
+ * see `markUploadSessionFileRegistered` — keeps that value: clearing it would
+ * let the abort guard and the expiry sweep treat a session whose durable
+ * resource already exists as disposable.
+ */
 async function markUploadSessionCompleted(
   session: UploadSessionRecord,
   leaseId: string,
@@ -711,7 +718,7 @@ async function markUploadSessionCompleted(
     .update(uploadSession)
     .set({
       status: 'completed',
-      completedFileId,
+      ...(completedFileId !== null ? { completedFileId } : {}),
       completedAt,
       processingLeaseId: null,
       processingLeaseExpiresAt: null,
