fix(uploads): make execution attachment completion replay-safe - #6601
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Tests add a compile-time Reviewed by Cursor Bugbot for commit cb57d0a. Configure here. |
Greptile SummaryThe PR makes execution-attachment completion replay through its idempotent finalizer, classifies invalid loader routing, and preserves completion markers written during registration.
Confidence Score: 5/5The PR appears safe to merge, with the changed replay and marker-preservation paths consistent with current finalizer behavior. Execution-attachment replay reuses a storage-key-idempotent metadata finalizer, unsupported loader paths now return classified errors, and existing workspace-file markers remain available for recovery.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/files/uploads/finalizers.ts | Aligns execution attachments with metadata-backed idempotent replay and exhaustively classifies unsupported durable-loader paths. |
| apps/sim/lib/uploads/upload-session/service.ts | Preserves transactionally registered file markers when the finalizer does not return a replacement marker. |
| apps/sim/app/api/files/uploads/finalizers.test.ts | Adds a typed replay-routing gate and verifies loader-backed versus idempotent-finalizer behavior for every internal purpose. |
Reviews (1): Last reviewed commit: "fix(uploads): make execution attachment ..." | Re-trigger Greptile
Summary
finalizeExecutionAttachmentreported acompletedFileId. That marker is what routes a replayed completion intoloadCompletedUploadPurpose, which handled onlyworkspace_fileand threw a bareErrorotherwise — unclassified, so the route rendered a generic 500. Its structurally identical twinfinalizeMothershipAttachmentcorrectly reports nothing.finalizeUploadPurposeTSDoc already states — so neither needs the marker. Dropped it from the execution finalizer so the twins agree.loadCompletedUploadPurposeis now an exhaustive switch, matching the siblingfinalizeUploadPurposeswitch, so adding a purpose is a compile error until its replay behavior is decided rather than a runtime 500. The residual arm throws a classifiedUploadSessionError('internal')instead of a bareError.markUploadSessionCompletedno longer clears a marker it was not given. A finalizer that records one inside its own registration transaction (markUploadSessionFileRegistereddoes this forworkspace_file) would otherwise have it overwritten withnull— and both the abort guard and the expiry sweep key on it:cleanupExpiredUploadSessionsonly treats afinalizingsession as disposable whencompletedFileIdis null. No-op for every current path, sincemarkUploadSessionCompletedmoves the session tocompleted, 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;requestJsondoes not retry; a concurrent double-submit is already a clean 409 fromclaimSession. Nothing is mutated on the failing path.Not a parity regression — the whole upload-session plane is new on staging (
uploadSessiondoes not exist in main's schema). This is new code not meeting its own documented contract: the plane's README states that sessions in domain finalization are retained "for an idempotent completion retry".Type of Change
Testing
bun run type-checkclean. 2053 tests pass acrossapp/api/files,lib/uploads,lib/knowledge,lib/table— the last three being the othercompleteUploadSessionconsumers.bun run lintandbun run check:api-validationpass.Tests pin the invariant rather than the symptom: a
Recordover 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. Verified they fail — restoring the marker turns theexecution_attachmentcase red; dropping a purpose from the table fails the gate.Follow-ups (deliberately not in this PR)
A DRY pass over the wider plane found real, pre-existing duplication that this change does not touch and should not be bundled with a latent-bug fix:
service.ts: the lease-scopedWHEREpredicate written 9×; the release-lease-on-failure catch 4× (two byte-identical); the terminal status-transition UPDATE 3×;deleteOwnedFinalObjectis a strict prefix ofdiscardIncompleteProviderState.provider.ts: the 3-way provider branch re-opened in 7 exported functions (21 copies ofcreate<P>Config), with GCS as a silent fallthrough in 5 of them.application.ts:completeWorkspaceUploadSessionwrites the same reauthorize call 4× where its internal twin extracts oneauthorizeclosure.Checklist