Optimistic presigned urls #1187
Conversation
WalkthroughRemoved md5/ContentMD5 from presign-part requests/handlers. Desktop uploader now prefetches the next part’s presigned URL concurrently, conditionally reuses or refetches it based on part numbers, and stops prefetching when the stream ends. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Desktop Uploader
participant B as Backend /presign-part
participant S as Storage
rect rgb(245,250,240)
note right of U: Start upload, prefetch presign for part=1
U->>B: POST /presign-part {fileKey, uploadId, partNumber=1}
B-->>U: {url, headers}
end
loop For each part N
alt Prefetched URL matches N
U-->>U: use cached presign URL(N)
else Prefetch mismatch
U->>B: POST /presign-part {partNumber=N}
B-->>U: {url, headers}
end
U->>S: PUT part N using returned URL (include computed Content-MD5 header locally)
U-->>U: expected_part_number = N+1
U->>B: POST /presign-part {partNumber=N+1} [async]
end
opt Completion
U-->>U: cancel/cleanup outstanding presign task
end
sequenceDiagram
autonumber
participant C as Client (desktop/web)
participant B as Backend /presign-part
participant S as Storage
note over C,B: md5Sum removed from request/response
C->>B: POST /presign-part {fileKey, uploadId, partNumber}
B-->>C: 200 {url, headers} %% no md5Sum/ContentMD5 in payload
C->>S: PUT part using returned URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/web/app/api/upload/[...route]/multipart.ts (1)
156-156: Tighten input validation for partNumberUse integer and valid range to avoid bad inputs.
- const { uploadId, partNumber, ...body } = c.req.valid("json"); + const { uploadId, partNumber, ...body } = c.req.valid("json");And update the schema above:
- partNumber: z.number(), + partNumber: z.number().int().min(1).max(10000),apps/desktop/src-tauri/src/upload.rs (4)
633-646: Good addition: optimistic presign prefetch initializationSeeding a background presign for part 1 and tracking expected_part_number is sound. Consider documenting the invariant that the first yielded chunk is usually part 1, but may re-emit part 1 at the end (your later dedupe handles this).
654-671: Avoid blocking on unfinished prefetch to preserve the “optimistic” benefitCurrently you always await the prefetch when expected_part_number == part_number. If the task isn’t finished, this blocks and negates the optimization. Prefer using the prefetched URL only when the task has already completed; otherwise abort it and fetch inline, or just fetch inline and keep/replace the task deterministically.
Apply a non-blocking variant like:
- let presigned_url = if expected_part_number == part_number { - if let Some(task) = optimistic_presigned_url_task.take() { - task.await - .map_err(|e| format!("uploader/part/{part_number}/task_join: {e:?}"))? - .map_err(|e| format!("uploader/part/{part_number}/presign: {e}"))? - } else { - api::upload_multipart_presign_part(&app, &video_id, &upload_id, part_number) - .await? - } - } else { + let presigned_url = if expected_part_number == part_number { + if let Some(ref handle) = optimistic_presigned_url_task { + if handle.is_finished() { + // Use prefetched result + optimistic_presigned_url_task + .take() + .unwrap() + .await + .map_err(|e| format!("uploader/part/{part_number}/task_join: {e:?}"))? + .map_err(|e| format!("uploader/part/{part_number}/presign: {e}"))? + } else { + // Prefer not to wait; abort to avoid stale work and fetch inline + handle.abort(); + optimistic_presigned_url_task = None; + api::upload_multipart_presign_part(&app, &video_id, &upload_id, part_number) + .await? + } + } else { + api::upload_multipart_presign_part(&app, &video_id, &upload_id, part_number) + .await? + } + } else { // The optimistic presigned URL doesn't match, abort it and generate a new correct one if let Some(task) = optimistic_presigned_url_task.take() { task.abort(); } debug!("Throwing out optimistic presigned URL for part {expected_part_number} as part {part_number} was requested!"); api::upload_multipart_presign_part(&app, &video_id, &upload_id, part_number) .await? };Also consider keeping the aborted task rare by adjusting prefetch only when the stream advances sequentially.
704-717: Prefetch for next part: solid; minor ergonomic nitSpawning for expected_part_number is correct and captures by value. Optional: bound retries/timeouts in upload_multipart_presign_part to avoid hanging tasks if the server is slow/unavailable.
680-681: Reconcile Content-MD5 header with server-side presign changeServer no longer signs/consumes md5Sum for presign. Sending Content-MD5 is optional; if it wasn’t part of the signed headers, it’s usually ignored by SigV4 verification, but S3 will still validate the MD5 if present. If you hit SignatureDoesNotMatch or intermittent 403s, drop this header or gate it behind a feature flag.
- .header("Content-MD5", &md5_sum) + // Consider dropping Content-MD5 now that presign doesn’t include it + // .header("Content-MD5", &md5_sum)If keeping it, please confirm presigned URLs are generated without requiring Content-MD5 and uploads succeed consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/api.rs(0 hunks)apps/desktop/src-tauri/src/upload.rs(2 hunks)apps/web/app/api/upload/[...route]/multipart.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src-tauri/src/api.rs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/api/upload/[...route]/multipart.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/upload/[...route]/multipart.ts
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/upload.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/api.rs (1)
upload_multipart_presign_part(50-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
719-721: Cleanup OKAborting any remaining optimistic task at the end avoids leaks.
Ensure aborted tasks don’t log panics in your tracing setup.
| // deprecated | ||
| // z.object({ md5Sum: z.string() }), | ||
| ]), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
md5Sum removal acknowledged; please verify no remaining callers depend on it
Dropping md5Sum from the schema is fine and aligns with the API change. Ensure no clients still send md5Sum or expect ContentMD5 to be signed.
Run to confirm repo-wide consistency:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for md5Sum payload usage..."
rg -nC2 -iP '\bmd5\s*Sum\b|"md5Sum"\s*:|md5_sum' --glob '!**/node_modules/**'
echo "Searching for Content-MD5 header usage..."
rg -nC2 -iP 'Content-MD5' --glob '!**/node_modules/**'Length of output: 3968
Remove md5Sum payload and Content-MD5 header in desktop clients
apps/desktop/src-tauri/src/upload.rs (lines 651–652, 680–681) and upload_legacy.rs (lines 914–916, 975–976) still send these; drop them to match the updated API schema.
🤖 Prompt for AI Agents
In apps/web/app/api/upload/[...route]/multipart.ts around lines 150-153, remove
the deprecated md5Sum field from the request schema and stop accepting or
extracting a Content-MD5 header: delete the z.object({ md5Sum: z.string() })
entry and any related parsing/validation logic, and ensure incoming requests
that include Content-MD5 are ignored (or stripped) rather than causing
validation or processing to rely on it; update any docs/tests referring to
md5Sum accordingly and coordinate with the desktop clients
(apps/desktop/**/upload.rs) to stop sending that header/payload.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/upload.rs (2)
17-17: Avoidstreamname collision (macro vs module).
use async_stream::{stream, ...};(macro) anduse futures::{..., stream};(module) share the same identifier. Legal (separate namespaces) but confusing. Consider aliasing the module to improve readability.Example:
-use futures::{Stream, StreamExt, TryStreamExt, future::join, stream}; +use futures::{Stream, StreamExt, TryStreamExt, future::join, stream as fstream};Then call
fstream::once(...)at usage sites.
634-644: Prefetch join cannot be canceled; considerselect+ abort for mismatches.
futures::joinwaits for both futures. If the arriving chunk’s part_number differs (e.g., header re-emit of part 1), you still await the now-useless prefetch before issuing the correct presign. Prefer spawning the prefetch and usingtokio::select!so you can abort it when it becomes irrelevant.Would you like a small refactor that:
- Spawns a prefetch task for
expected_part_number- On chunk arrival, cancels prefetch on mismatch
- Reuses completed prefetch on match
- Cleans up any outstanding task when the stream ends?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/upload.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/upload.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/api.rs (1)
upload_multipart_presign_part(50-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
689-690: LGTM: update next expected part number.Incrementing
expected_part_numberafter a successful upload is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/upload.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/upload.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/api.rs (1)
upload_multipart_presign_part(50-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
17-17: LGTM: Import added for concurrent prefetch optimization.The
future::joinimport is correctly added to support the concurrent prefetching of presigned URLs introduced in themultipart_uploaderfunction.
2 participants
Replaces #1181 which I merged by mistake
Summary by CodeRabbit
New Features
Refactor