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 — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: dark: page.screenshot: Timeout 30000ms exceeded. Call log: - taking page screenshot - waiting for fonts to load... |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Incremental Code Review Summary
The latest commit adds an upload-generation fence and propagates it through resumable sessions, buffered scratch, finalization, interruption, and background verification. It also adds status-and-generation CAS protection, generation-scoped cleanup, and native retry generation handling. I verified that all six issues from the previous review are addressed and resolved their existing threads before this review.
New findings
- 🔴 HIGH: The generation fence is enforced by
chunk.post.ts, but the plain browser recorder does not senduploadGenerationId. WhenuploadRetryResumeis enabled, browser reset/compression/retry receives a new generation and all subsequent browser chunks are rejected as stale. This makes the flag unsafe for the browser client unless rollout is desktop-only or the browser recorder forwards the returned generation. - 🟡 MEDIUM: Non-retry native reset callers pass no attempt or generation, but reset now creates a generation whenever recovery is enabled. Their subsequent native upload requests still omit that generation and are rejected by the chunk route.
The generation model is otherwise substantially stronger than the previous revision, with generation-scoped cleanup and finalization claims. The PR remains high risk because these paths coordinate client compatibility with server-side fencing.
🧪 Browser testing: Could not verify — browser automation tools were unavailable; the dev server was healthy and all planned cases reported couldnt_verify/env_issue.
| // The first renewal admits the request. Later renewals immediately before | ||
| // every durable write/finalization boundary close the body-read/provider | ||
| // gap where /abort can otherwise commit while this request is in flight. | ||
| if ((existing.uploadGenerationId ?? null) !== uploadGenerationId) { |
There was a problem hiding this comment.
🔴 Generation fencing breaks plain-browser retries
The chunk route now rejects any request whose uploadGenerationId differs from the row, but the plain browser recorder's resetUploadedChunks and subsequent uploadBlobInSlices/uploadBlobInStreamingChunks calls never send this field. When uploadRetryResume is enabled, reset creates and persists a generation, so every following browser retry/compression chunk is rejected with A newer upload generation is already active. Either propagate the returned generation through the browser upload path or ensure this flag cannot be enabled for browser clients.
Additional Info
Found by browser-test-planner source analysis and confirmed in app/components/recorder/recorder-engine.ts: reset response and chunk requests omit uploadGenerationId.
| // Fence this reset before deleting any provider or buffered state. A | ||
| // retry that lost the token race must not tear down the winner's session. | ||
| const now = new Date().toISOString(); | ||
| const nextGenerationId = recoveryEnabled ? randomUUID() : null; |
There was a problem hiding this comment.
🟡 Generation assignment breaks unfenced native uploads
This assigns a new generation for every recovery-enabled reset, including existing native live-upload callers that pass neither an attempt nor a generation and do not receive/propagate the returned generation. Their subsequent chunk/finalization requests omit it and fail the generation equality check in chunk.post.ts, so multi-segment/live native uploads can no longer complete when the flag is enabled. Only rotate a generation for a claimed retry, or return and propagate it through every unfenced caller.
Additional Info
Found by 1 of 4 incremental review agents and confirmed against native reset_upload_chunks call sites that pass None for both identifiers.
|
@shomix can you give this a review when you have a sec |
2 participants
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