Harden resumable chunk uploads (ALO-121)#282
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🔍 Remote MCP GitHub CopilotRelevant review context from public code:
I did not find exact matches for the PR’s specific 📝 WalkthroughSummary by CodeRabbit
WalkthroughChunked video uploads now resume from localStorage and server status, while the worker adds upload status and cancel endpoints, shared KV key helpers, and corrected retry byte accounting. ChangesFrontend resume persistence and server sync
Backend upload status/cancel endpoints and retry accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Comment |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
spooool | 35dd1b0 | Jul 09 2026, 06:31 AM |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates chunked upload state persistence from sessionStorage to localStorage and introduces new endpoints to retrieve upload status and cancel active upload sessions. It also refines byte-counting logic during final chunk retries to prevent double-counting. The review feedback highlights several key improvements: forwarding custom authentication headers to the upload status check, handling potential JSON parsing exceptions for KV data to avoid internal server errors, and decoupling database cleanup from R2 abort actions in the cancellation endpoint to ensure robust resource cleanup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async function fetchUploadStatus(endpoint: string, uploadId: string): Promise<UploadStatus | null> { | ||
| const res = await fetch(statusEndpoint(endpoint, uploadId), { method: 'GET' }); | ||
| if (res.status === 404) return null; | ||
| if (!res.ok) throw new Error(`resume status failed: ${res.status}`); | ||
| return (await res.json()) as UploadStatus; | ||
| } |
There was a problem hiding this comment.
The fetchUploadStatus function does not accept or forward custom headers (such as authentication tokens or session headers) that might be provided in UploadOptions.headers. Since the status endpoint /api/videos/upload/:uploadId/status requires authentication, any client relying on custom headers for authentication will fail to resume uploads.
We should update fetchUploadStatus to accept and forward these headers.
async function fetchUploadStatus(
endpoint: string,
uploadId: string,
headers?: Record<string, string>,
): Promise<UploadStatus | null> {
const res = await fetch(statusEndpoint(endpoint, uploadId), { method: 'GET', headers });
if (res.status === 404) return null;
if (!res.ok) throw new Error(`resume status failed: ${res.status}`);
return (await res.json()) as UploadStatus;
}| uploadId = stored.uploadId; | ||
| startChunk = stored.nextChunk; | ||
| try { | ||
| const status = await fetchUploadStatus(endpoint, stored.uploadId); |
There was a problem hiding this comment.
Pass the headers option to fetchUploadStatus so that the status check is properly authenticated.
| const status = await fetchUploadStatus(endpoint, stored.uploadId); | |
| const status = await fetchUploadStatus(endpoint, stored.uploadId, headers); |
| function parseUploadedChunks(partsJson: string | null): number[] { | ||
| if (!partsJson) return []; | ||
| const uploadedPartsMap = JSON.parse(partsJson) as Record<string, { etag: string; size: number }>; | ||
| return Object.keys(uploadedPartsMap) | ||
| .map((partNumber) => Number(partNumber) - 1) | ||
| .filter((chunkIndex) => Number.isInteger(chunkIndex) && chunkIndex >= 0) | ||
| .sort((a, b) => a - b); | ||
| } |
There was a problem hiding this comment.
JSON.parse(partsJson) can throw an unhandled exception if partsJson is malformed or corrupted in KV, which would cause a 500 Internal Server Error on the status endpoint. It is safer to wrap the parsing in a try-catch block and return an empty array on failure.
function parseUploadedChunks(partsJson: string | null): number[] {
if (!partsJson) return [];
try {
const uploadedPartsMap = JSON.parse(partsJson) as Record<string, { etag: string; size: number }>;
return Object.keys(uploadedPartsMap)
.map((partNumber) => Number(partNumber) - 1)
.filter((chunkIndex) => Number.isInteger(chunkIndex) && chunkIndex >= 0)
.sort((a, b) => a - b);
} catch {
return [];
}
}| if (multipartUploadId && uploadMetaJson) { | ||
| const uploadMeta = uploadMetaPersistedSchema.parse(JSON.parse(uploadMetaJson)); | ||
| await c.env.VIDEOS.resumeMultipartUpload(uploadMeta.r2Key, multipartUploadId).abort().catch(() => {}); | ||
| await c.env.DB.prepare( | ||
| `DELETE FROM videos WHERE id = ? AND user_id = ? AND status = 'uploading'`, | ||
| ) | ||
| .bind(uploadMeta.videoId, user.id) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
If multipartUploadId is missing or expired from KV, but uploadMetaJson is still present, the temporary database row with status = 'uploading' will not be deleted because the entire block is skipped when multipartUploadId is falsy. We should decouple the database row deletion from the R2 multipart abort check so that the database row is always cleaned up when the session metadata is available.
if (uploadMetaJson) {
try {
const uploadMeta = uploadMetaPersistedSchema.parse(JSON.parse(uploadMetaJson));
if (multipartUploadId) {
await c.env.VIDEOS.resumeMultipartUpload(uploadMeta.r2Key, multipartUploadId).abort().catch(() => {});
}
await c.env.DB.prepare(
`DELETE FROM videos WHERE id = ? AND user_id = ? AND status = 'uploading'`,
)
.bind(uploadMeta.videoId, user.id)
.run();
} catch { /* ignore parsing/validation errors */ }
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfad2e3e9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uploadId = stored.uploadId; | ||
| startChunk = stored.nextChunk; | ||
| try { | ||
| const status = await fetchUploadStatus(endpoint, stored.uploadId); |
There was a problem hiding this comment.
When uploadInChunks is used for recorder uploads, this new unconditional status probe calls /api/videos/upload/:uploadId/status, but recorder multipart state is stored under the separate upload:rec:${user.id}:... namespace in handleRecorderUpload and the new status route only reads video upload keys. After a reload/disconnect of a multi-chunk recorder upload, the status call returns 404, removeProgress runs, and the helper restarts at chunk 0 instead of resuming the existing recorder multipart session, re-uploading data and leaving the old multipart upload around. Skip the server-status verification for target === 'recorder' or add a recorder-aware status endpoint before deleting stored progress.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/workers/videos.test.ts (1)
682-711: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify the double-counting fix.
The seeded old part size (512, key
'2') equals the retried chunk's size (512 bytes), so the assertionexpect(res1.status).toBe(201)would pass identically whether the pre-completion check subtracts the old size or naively double-counts it — the resulting total (either ~1024 or ~1536 bytes) is nowhere near the 10GB quota mock, so no code path this test exercises can actually fail from the bug being fixed.To meaningfully guard the fix, either:
- use a retried size that differs from the previously recorded size and assert the persisted/completed total equals the new size (not old+new), or
- make the previously-recorded "old" size large enough that double-counting would trip a size/quota check, and assert the request still succeeds.
♻️ Example: make the test size-sensitive
const partsKey = `upload:${USER_ID}:${uploadId}:parts`; kv[partsKey] = JSON.stringify({ '1': { etag: 'etag-1', size: MP4_MAGIC.byteLength }, - '2': { etag: 'etag-old', size: 512 }, + '2': { etag: 'etag-old', size: 999_999 }, }); const fd1 = new FormData(); fd1.set('title', 'vid'); fd1.set('description', ''); fd1.set('file', new Blob([new Uint8Array(512)], { type: 'video/mp4' }), 'v.mp4'); fd1.set('chunkIndex', '1'); fd1.set('chunkCount', '2'); fd1.set('uploadId', uploadId); const res1 = await fetcher('/api/videos/upload', { method: 'POST', body: fd1 }); expect(res1.status).toBe(201); + // Assert against whatever exposes the final computed byte total + // (e.g. dbUpdates bound params) to confirm it reflects 512, not 999_999 + 512.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workers/videos.test.ts` around lines 682 - 711, The test in videos.test.ts does not currently prove the double-counting fix because the seeded prior part size matches the retried chunk size and never exercises the failing path. Update the scenario around the upload retry in the `does not double-count bytes...` test so the previously stored part size differs from the retried chunk, or make the old size large enough that double-counting would break the size/quota logic. Then assert on the resulting persisted/completed total from the upload flow, not just the 201 response from `/api/videos/upload`, so the test specifically verifies the behavior in `multiChunkEnv`, `partsKey`, and the retry handling around `uploadId`.src/frontend/lib/chunked-upload.test.ts (1)
193-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the transient-failure test assert storage retention.
This upload succeeds, so final cleanup would mask a regression that clears local progress inside the status-failure catch. Add a rejecting upload path and assert the stored resume entry remains.
Example coverage
+ it('retains localStorage progress when status lookup and upload retry fail transiently', async () => { + const file = makeFile(30 * 1024 * 1024, 'status-err-fail.mp4', 'video/mp4'); + const key = `chunk-upload:status-err-fail.mp4:${file.size}:${file.lastModified}`; + const stored = JSON.stringify({ uploadId: 'stored-id', nextChunk: 1, chunkCount: 3 }); + ssMap.set(key, stored); + + mockFetch(async (_url, init) => { + if (!init?.body) return new Response('temporary', { status: 503 }); + return new Response('server down', { status: 503 }); + }); + + await expect(uploadInChunks({ + file, endpoint: '/api/upload', target: 'video', fields: {}, onProgress: () => {}, + _sleep: noSleep, + })).rejects.toThrow(/chunk 1 failed: 503/); + expect(ssMap.get(key)).toBe(stored); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/lib/chunked-upload.test.ts` around lines 193 - 213, The transient resume-status failure test currently only covers a successful upload, so it can miss regressions where the stored progress is cleared after a status check error. Update the test around uploadInChunks to use a rejecting upload path for the status-failure case and assert that the ssMap resume entry for the upload key is still present afterward, while keeping the existing chunkIndex behavior coverage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/lib/chunked-upload.ts`:
- Around line 129-130: The status lookup in fetchUploadStatus is missing the
same request headers used by the chunk POST path, which can break
header-authenticated uploads and cause stale resume behavior. Update
fetchUploadStatus (and the related status-check call site around the chunk
upload flow) to accept and forward the upload headers into the fetch call,
keeping the status verification request consistent with the main upload
requests.
- Around line 168-174: The resume logic in chunked-upload.ts does not handle an
`uploading` status when the server’s `chunkCount` differs from the stored one,
so it may incorrectly reuse the existing `uploadId` and `startChunk`. Update the
branch around `firstMissingChunk`, `saveProgress`, and the `status.chunkCount
=== chunkCount` check to detect this incompatible state and restart the upload
cleanly instead of resuming. Make sure the `saveProgress`/resume path only
applies when the server session matches the current chunk layout.
In `@src/workers/videos.ts`:
- Around line 577-592: Do not clear multipart upload session state when the R2
abort in the upload cleanup path fails. In the cleanup logic around
resumeMultipartUpload(...).abort() and the subsequent SESSIONS deletions, catch
the abort failure and return a 5xx instead of continuing; only delete mpidKey,
metaKey, partsKey, and doneKey after a successful abort. Keep the existing
uploadMetaPersistedSchema parse and DB cleanup flow, but preserve retry state
when abort cannot complete.
- Around line 773-774: The upload metadata in videos.ts is trusting raw form
input for fileSize, which can become NaN/null and break upload status parsing
later. Update the logic around the fileSize assignment in the upload metadata
flow to validate that formData.get('fileSize') is a nonnegative integer before
persisting it, and fall back to omitting the field or using the rawFile.size
value only when the input is invalid. Use the surrounding upload metadata
construction and uploadMetaPersistedSchema.parse path to locate the fix.
---
Nitpick comments:
In `@src/frontend/lib/chunked-upload.test.ts`:
- Around line 193-213: The transient resume-status failure test currently only
covers a successful upload, so it can miss regressions where the stored progress
is cleared after a status check error. Update the test around uploadInChunks to
use a rejecting upload path for the status-failure case and assert that the
ssMap resume entry for the upload key is still present afterward, while keeping
the existing chunkIndex behavior coverage intact.
In `@src/workers/videos.test.ts`:
- Around line 682-711: The test in videos.test.ts does not currently prove the
double-counting fix because the seeded prior part size matches the retried chunk
size and never exercises the failing path. Update the scenario around the upload
retry in the `does not double-count bytes...` test so the previously stored part
size differs from the retried chunk, or make the old size large enough that
double-counting would break the size/quota logic. Then assert on the resulting
persisted/completed total from the upload flow, not just the 201 response from
`/api/videos/upload`, so the test specifically verifies the behavior in
`multiChunkEnv`, `partsKey`, and the retry handling around `uploadId`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21c39ef8-35b3-434d-aad4-60ae870060b1
📒 Files selected for processing (4)
src/frontend/lib/chunked-upload.test.tssrc/frontend/lib/chunked-upload.tssrc/workers/videos.test.tssrc/workers/videos.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Workers Builds: spooool
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,html,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Strand stack utilities: require the base
stackclass for flex layout; size classes likestack-sm/stack-xlonly control gap
Files:
src/frontend/lib/chunked-upload.tssrc/workers/videos.test.tssrc/frontend/lib/chunked-upload.test.tssrc/workers/videos.ts
🔍 Remote MCP
Relevant context for review
- Public multipart-upload implementations commonly expose a dedicated
.../statusroute for resumable sessions, includingGET /chunks/:token/:uploadId/status,GET /:eventId/chunked-upload/:uploadId/status, androuter.get("/:uploadId/status"). citeturn0search0 - In those examples, status handling is tied to upload-session state, including explicit
getUploadStatus(uploadId)-style lookups. citeturn0search0 - Duplicate chunk handling is typically idempotent: several chunk upload services check
uploadedChunks.includes(chunkIndex)and return early / skipped instead of reprocessing the same part. citeturn0search0 - Abort is a distinct cleanup path in multipart systems, with explicit
AbortMultipartUpload/abortMultipartUploadcalls used to cancel in-flight uploads. citeturn0search0
🔇 Additional comments (6)
src/workers/videos.test.ts (4)
485-488: LGTM!Also applies to: 500-500, 534-534, 555-555
633-660: LGTM!
662-680: LGTM!
643-645: 🎯 Functional CorrectnessNo duplicate declaration here The snippet contains a single
const { uploadId, videoId } = ...destructuring statement, so there’s no duplicateconstin this block.> Likely an incorrect or invalid review comment.src/frontend/lib/chunked-upload.ts (1)
88-108: LGTM!Also applies to: 112-127, 196-196
src/frontend/lib/chunked-upload.test.ts (1)
7-13: LGTM!Also applies to: 114-146, 165-191
1 participant
Closes ALO-121.
Summary
GET /api/videos/upload/:uploadId/statusso clients can resume from server-confirmed uploaded chunks after reloads or disconnects.DELETE /api/videos/upload/:uploadIdto abort in-flight R2 multipart uploads, clear KV session state, and remove the temporary uploading row.localStorage, verifies it against the server before resuming, preserves it on transient status lookup failures, and sends totalfileSizemetadata.Verification
npx vitest run src/frontend/lib/chunked-upload.test.tsnpx vitest run src/workers/videos.test.tsnpm run type-checknpm run lint(passes with existing warning-only lint output)npm run buildFull suite note
npm testcurrently fails on existing full-suite environment drift in Node 26:window.localStorageis undefined inApp.shell.dom.test.tsxandanalytics.test.ts. The run still reported 98 passing files / 1041 passing tests before those unrelated failures.