Skip to content

Optimistic presigned urls #1187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Brendonovich merged 9 commits into from
Oct 15, 2025
Merged

Optimistic presigned urls #1187

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4e0e9f3
initial pass
oscartbeaumont Oct 13, 2025
f4af36c
improved
oscartbeaumont Oct 13, 2025
06301f7
Merge branch 'main' into optimistic-presigned-urls
oscartbeaumont Oct 14, 2025
880e480
fix
oscartbeaumont Oct 14, 2025
f22a07e
Merge branch 'main' into optimistic-presigned-urls
oscartbeaumont Oct 15, 2025
bc40b2c
cleanup
oscartbeaumont Oct 15, 2025
292dc97
cleanup code a bit
oscartbeaumont Oct 15, 2025
2f97e1a
thanks CodeRabbit
oscartbeaumont Oct 15, 2025
dea6aec
fix comment
oscartbeaumont Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions apps/desktop/src-tauri/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ pub async fn upload_multipart_presign_part(
video_id: &str,
upload_id: &str,
part_number: u32,
md5_sum: &str,
) -> Result<String, AuthedApiError> {
#[derive(Deserialize)]
#[serde(rename_all = "camelCase")]
Expand All @@ -68,7 +67,6 @@ pub async fn upload_multipart_presign_part(
"videoId": video_id,
"uploadId": upload_id,
"partNumber": part_number,
"md5Sum": md5_sum
}))
})
.await
Expand Down
53 changes: 50 additions & 3 deletions apps/desktop/src-tauri/src/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,16 +630,45 @@ fn multipart_uploader(
let mut stream = pin!(stream);
let mut prev_part_number = None;

let mut optimistic_presigned_url_task: Option<tokio::task::JoinHandle<Result<String, AuthedApiError>>> = Some(
tokio::spawn({
let app = app.clone();
let video_id = video_id.clone();
let upload_id = upload_id.clone();

async move {
api::upload_multipart_presign_part(&app, &video_id, &upload_id, 1)
.await
}
})
);
let mut expected_part_number = 1u32;

while let Some(item) = stream.next().await {
let Chunk { total_size, part_number, chunk } = item.map_err(|err| format!("uploader/part/{:?}/fs: {err:?}", prev_part_number.map(|p| p + 1)))?;
trace!("Uploading chunk {part_number} ({} bytes) for video {video_id:?}", chunk.len());
prev_part_number = Some(part_number);
let md5_sum = base64::encode(md5::compute(&chunk).0);
let size = chunk.len();

let presigned_url =
api::upload_multipart_presign_part(&app, &video_id, &upload_id, part_number, &md5_sum)
.await?;
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 {
// 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?
};

trace!("Uploading part {part_number}");

Expand Down Expand Up @@ -671,6 +700,24 @@ fn multipart_uploader(
size,
total_size
};

// We generate the presigned URL ahead of time for the next expected part.
// This means if the filesystem takes a while for the recording to reach previous total + CHUNK_SIZE, we aren't just doing nothing.
expected_part_number = part_number + 1;
optimistic_presigned_url_task = Some(tokio::spawn({
let app = app.clone();
let video_id = video_id.clone();
let upload_id = upload_id.clone();

async move {
api::upload_multipart_presign_part(&app, &video_id, &upload_id, expected_part_number)
.await
}
}));
}

if let Some(task) = optimistic_presigned_url_task.take() {
task.abort();
}

debug!("Completed multipart upload for {video_id:?} in {:?}", start.elapsed());
Expand Down
6 changes: 3 additions & 3 deletions apps/web/app/api/upload/[...route]/multipart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,18 +141,19 @@ app.post(
.object({
uploadId: z.string(),
partNumber: z.number(),
md5Sum: z.string(),
})
.and(
z.union([
z.object({ videoId: z.string() }),
// deprecated
z.object({ fileKey: z.string() }),
// deprecated
// z.object({ md5Sum: z.string() }),
]),
),
Comment on lines +150 to 153
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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

),
async (c) => {
const { uploadId, partNumber, md5Sum, ...body } = c.req.valid("json");
const { uploadId, partNumber, ...body } = c.req.valid("json");
const user = c.get("user");

const fileKey = parseVideoIdOrFileKey(user.id, {
Expand All @@ -174,7 +175,6 @@ app.post(
fileKey,
uploadId,
partNumber,
{ ContentMD5: md5Sum },
);

return presignedUrl;
Expand Down
Loading