Resume interrupted Clips uploads - #2436
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — generation failedThe visual recap could not be generated for this pull request. This is informational only and does not block the PR. Diagnostic: No plan URL: create-visual-recap failed 422 Unprocessable Entity: {"error":"plan.mdx: Expected a closing tag for Agent output: Repaired both malformed |
|
@shomix can you give this a review when you have a sec |
There was a problem hiding this comment.
Builder reviewed your changes and found 6 potential issues 🔴
Review Details
Incremental Code Review Summary
The latest remote head addresses the five prior cleanup/compatibility comments, so I resolved those existing threads before this review. The new changes improve generation-scoped abort/reaper cleanup and legacy namespace isolation, but the new asynchronous state transitions still have several race and failure-coercion gaps.
New findings
- 🔴 HIGH: A delayed interruption callback can overwrite a replacement generation's
uploadingapplication state after its SQL claim has already become stale. - 🔴 HIGH: Reset can race with abort between the row CAS and provider-session persistence, leaving a provider session attached to an already-cancelled generation.
- 🔴 HIGH: Seven-digit chunk indexes can be accepted while exact scratch-key filters only recognize six-digit indexes, allowing accepted bytes to be omitted from sums/finalization and left behind by cleanup.
- 🟡 MEDIUM: Resume and interruption convert unreadable application-state reads into empty state and overwrite metadata instead of surfacing the state-store failure.
- 🟡 MEDIUM: Resume can publish
uploadingapplication state after an interrupt wins concurrently, leaving SQL and polling state inconsistent.
The generation CAS structure is substantially improved, but these remaining issues affect correctness under delayed callbacks and partial state-store/provider failures. Risk remains high because this PR coordinates durable upload state with external storage sessions.
🧪 Browser testing: Will run after this review (PR touches UI code).
0dfbb69 to
bbb0d44
Compare
Problem
When a Clips desktop upload failed, retry could replay the entire local recording even when the storage provider had already accepted most of it. That made retries take minutes, and some failed recordings could remain in a loading state because interruption cleanup discarded the resumable session before a retry could reclaim it.
Approach
Treat the server and storage provider as the authority for retry progress. Preserve recoverable upload state, ask the server for the last committed byte, and resume only when the local file is the exact same byte stream. Otherwise, take an explicit full-restart path.
The behavior is protected by the default-off
uploadRetryResumefeature flag so it can be enabled for one production account before broader rollout.What changed
upload_attempt_idandupload_generation_idcolumns through additive startup migrations.Safety and operations
processingorready; stale writers can clean only their own generation.Verification
Review focus
Follow-ups