From cb57d0ab40649075ca1af7374fb6951f51f91ab3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 21:52:01 -0700 Subject: [PATCH] fix(uploads): make execution attachment completion replay-safe 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. --- .../app/api/files/uploads/finalizers.test.ts | 70 ++++++++++++++++++- apps/sim/app/api/files/uploads/finalizers.ts | 32 +++++++-- .../sim/lib/uploads/upload-session/service.ts | 9 ++- 3 files changed, 105 insertions(+), 6 deletions(-) diff --git a/apps/sim/app/api/files/uploads/finalizers.test.ts b/apps/sim/app/api/files/uploads/finalizers.test.ts index 37434dbac84..4483401fd18 100644 --- 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, 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 = { + 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 index a59eec3beb0..13d16c1df5b 100644 --- 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 { - 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 index 240bbdd7981..a4386de8753 100644 --- 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(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,