Skip to content

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

Merged
waleedlatif1 merged 1 commit into
stagingfrom
fix/upload-replay-contract
Aug 12, 2026
Merged

fix(uploads): make execution attachment completion replay-safe#6601
waleedlatif1 merged 1 commit into
stagingfrom
fix/upload-replay-contract

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • 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 what the finalizeUploadPurpose TSDoc already states — so neither needs the marker. Dropped it from the execution finalizer so the twins agree.
  • loadCompletedUploadPurpose is now 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. 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; a concurrent double-submit is already a clean 409 from claimSession. Nothing is mutated on the failing path.

Not a parity regression — the whole upload-session plane is new on staging (uploadSession does 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

  • Bug fix

Testing

bun run type-check clean. 2053 tests pass across app/api/files, lib/uploads, lib/knowledge, lib/table — the last three being the other completeUploadSession consumers. bun run lint and bun run check:api-validation pass.

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. Verified they fail — restoring the marker turns the execution_attachment case 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-scoped WHERE predicate written 9×; the release-lease-on-failure catch 4× (two byte-identical); the terminal status-transition UPDATE 3×; deleteOwnedFinalObject is a strict prefix of discardIncompleteProviderState.
  • provider.ts: the 3-way provider branch re-opened in 7 exported functions (21 copies of create<P>Config), with GCS as a silent fallthrough in 5 of them.
  • application.ts: completeWorkspaceUploadSession writes the same reauthorize call 4× where its internal twin extracts one authorize closure.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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.
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
docsSkippedSkippedAug 12, 2026 4:52am

Request Review

@cursor

cursorBot commented Aug 12, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches upload-session completion marking and replay routing, including how completedFileId is preserved for abort/cleanup guards. Scope is small and latent (no current client retries), but the state-machine change is easy to get wrong.

Overview
Makes execution attachment completion replay-safe by aligning it with other idempotent, metadata-backed purposes.

finalizeExecutionAttachment no longer reports a completedFileId, so a replayed completion re-runs the finalizer instead of routing into loadCompletedUploadPurpose (which only reloads workspace_file and previously threw an unclassified error → generic 500).

loadCompletedUploadPurpose is now an exhaustive switch that throws a classified UploadSessionError('internal') for purposes without a durable loader. markUploadSessionCompleted only writes completedFileId when the finalizer supplies one, so it no longer clears a marker recorded earlier in registration.

Tests add a compile-time Record gate over InternalUploadPurpose that pins which purposes replay via loader vs idempotent finalizer.

Reviewed by Cursor Bugbot for commit cb57d0a. Configure here.

@greptile-apps

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes execution-attachment completion replay through its idempotent finalizer, classifies invalid loader routing, and preserves completion markers written during registration.

  • Removes the loader marker from execution-attachment finalization.
  • Makes completed-purpose loading exhaustive and returns a classified internal error for unsupported loader paths.
  • Avoids clearing an existing marker when completion supplies none.
  • Adds compile-time replay routing and behavior tests.

Confidence Score: 5/5

The 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.

Important Files Changed

FilenameOverview
apps/sim/app/api/files/uploads/finalizers.tsAligns execution attachments with metadata-backed idempotent replay and exhaustively classifies unsupported durable-loader paths.
apps/sim/lib/uploads/upload-session/service.tsPreserves transactionally registered file markers when the finalizer does not return a replacement marker.
apps/sim/app/api/files/uploads/finalizers.test.tsAdds 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

@waleedlatif1
waleedlatif1 merged commit 0d640aa into stagingAug 12, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/upload-replay-contract branch August 12, 2026 05:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@waleedlatif1