Repository navigation
fix: resilient processed-video upload (resumable + retry) - #86
Merged
Merged
Conversation
The finalize-stage processed-video upload failed for larger clips (verified same
footage: a 2s clip uploads fine, the 50s/~16MB version fails 2/2), marking the
whole match failed with num_frames/fps left NULL. Root cause: the single-shot
standard /object upload is bounded by storage3's 20s-per-request timeout — a
~16MB upload over a slow/contended Modal->Supabase egress exceeds it — and, unlike
the download path, the upload had no retry.
- Upload processed.mp4 via Supabase's resumable (TUS) endpoint in 6MB chunks, so
no single request approaches the 20s timeout.
- Add bounded exponential-backoff retry to the video and heatmap uploads; skip
retry on permanent 4xx (e.g. 413).
- Add an ffmpeg subprocess timeout in make_streamable_mp4 so a hung encode fails
fast instead of running to the Modal wall-clock ("stopped during finalize").
Verified against Supabase: 16MB and 48MB upload via resumable; 60MB fails fast
with a single 413 (files >~50MB additionally need the project's global upload
size limit raised in Supabase settings).
fix: upload processed video via resumable endpoint with retry
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain around encode deadlines, upload retry budgets, and heatmap 4xx classification.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens processed-video delivery with resumable uploads, retries, and ffmpeg timeout handling.
Changes:
- Adds 6 MiB TUS uploads with exponential retry handling.
- Retries video and heatmap uploads.
- Adds ffmpeg timeout behavior and upload tests.
File summaries
| File | Summary | Review findings |
|---|---|---|
backend/pipeline/storage.py |
Implements resilient uploads and encoding timeouts. | Moderate (3 votes): enforce an overall ffmpeg encode deadline. Moderate (1 vote): bound retries by an overall upload time budget. Moderate (3 votes): classify heatmap SDK 4xx exceptions as permanent failures. |
backend/tests/test_storage_upload.py |
Tests transient retries and permanent upload failures. | No findings. |
Review details
Suppressed comments (1)
backend/pipeline/storage.py:153
- The retry is bounded by attempt count, but not by the Modal job's runtime. A 48 MiB video needs eight 6 MiB chunks; if each request reaches the 120-second timeout, one resumable attempt can take about 16 minutes, and four full attempts can exceed 64 minutes before backoff, while
process_videois capped at 30 minutes. Add an overall upload deadline/budget so a degraded endpoint cannot guarantee a container timeout and stuck processing row.
with httpx.Client(timeout=httpx.Timeout(_STORAGE_TIMEOUT_SEC)) as client:
created = client.post(
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses PR #86 review comments: - make_streamable_mp4 shares one encode deadline across both codec attempts, so a hung nvenc followed by a hung libx264 can no longer each burn the full budget (previously up to 2x900s = the whole 1800s job). - Upload retries are bounded by an overall 600s per-artifact budget: the deadline gates both the retry loop and each TUS chunk's request timeout, so a degraded endpoint cannot run out the 1800s process_video wall-clock. - _permanent_status classifies storage3 StorageApiError 4xx (e.g. a heatmap 413) as permanent, not just httpx.HTTPStatusError, so SDK-path 4xx aren't retried. Adds tests for SDK 4xx classification and the time-budget cutoff (7 pass).
…mments fix: bound encode/upload deadlines and classify SDK 4xx as permanent
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate findings remain in backend/pipeline/storage.py.
Review details
Suppressed comments (3)
backend/pipeline/storage.py:78
- Catching
TimeoutExpiredhere turns a hung encode into the normalFalsepath; with the shared 900s deadline exhausted, thelibx264attempt is skipped andmake_streamable_mp4returns the MJPEG.aviinput.run_pipelinethen uploads those AVI bytes asprocessed.mp4(run.py:2189,run.py:2549-2557), so the match can be marked done with a non-playable artifact instead of failing. Propagate this timeout (or otherwise make the pipeline fail) rather than falling back to the original file for this case.
except subprocess.TimeoutExpired:
print(f"[Storage] ffmpeg ({codec}) hit the {_FFMPEG_BUDGET_SEC}s encode budget")
return False
backend/pipeline/storage.py:230
- Each retry invokes
_upload_resumablefrom the beginning, so a transient failure after several successful TUS PATCHes discards all progress and creates a new session. On a slow connection, repeated 6 MB uploads can consume the 600-second deadline and fail even though the existing TUS session could resume; retain the session URL/offset and retry the failed chunk (or use a TUS client that resumes) across attempts.
_upload_with_retry(
lambda: _upload_resumable(local_path, bucket, remote_path, "video/mp4", deadline),
label=f"processed video {match_id}",
deadline=deadline,
backend/pipeline/storage.py:257
- The new 600-second retry budget for optional heatmaps can delay a mandatory video failure:
upload_results_parallelexits itsThreadPoolExecutorcontext before reading futures, so it waits for these retries even afterupload_processed_videohas failed (for example, on a 413). This defeats fail-fast behavior and can consume a large part of the 1800-second Modal run; short-circuit/cancel optional uploads once the video future fails or collect the video result before waiting on heatmaps.
_upload_with_retry(_do, label=f"{filename} {match_id}", deadline=time.monotonic() + _UPLOAD_BUDGET_SEC)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
AnikMajumdar
added a commit
that referenced
this pull request
Sep 12, 2026
Addresses PR #86 review round 2: - make_streamable_mp4 now raises on an ffmpeg timeout / exhausted encode budget instead of returning the unencoded original, which run_pipeline would otherwise upload as a non-playable processed.mp4 and mark the match done. - _upload_resumable resumes a TUS upload from the server's offset (via HEAD) after a transient chunk failure instead of restarting the session; the create step is retried and the whole upload stays within the 600s budget. - upload_results_parallel blocks on the mandatory video first and shuts the pool down without waiting, so a video failure (e.g. 413) is not delayed by optional heatmap retries burning their budget. Adds tests for the encode-timeout raise and the video fail-fast behavior (9 pass).
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the processed-video upload fix from
aniktomain.Problem
Finalize-stage upload of
processed.mp4to Supabase Storage failed for largerclips, marking the whole match failed (verified: same footage — a 2s clip
uploads fine, the 50s/~16MB version fails 2/2; only 5 failures in table history).
Root cause: the single-shot standard
/objectupload is bounded by storage3's20s-per-request timeout — a ~16MB upload over a slow/contended Modal→Supabase
egress exceeds it — and, unlike the download path, the upload had no retry.
Fix (backend/pipeline/storage.py)
processed.mp4via Supabase's resumable (TUS) endpoint in 6MB chunks,so no single request approaches the 20s timeout.
on permanent 4xx (e.g. 413).
make_streamable_mp4so a hung encodefails fast instead of running to the Modal wall-clock.
Verification (against real Supabase)
test_storage_upload.py— 5 tests pass ✅Follow-ups (not code)
tennis-modalto thecourtcheckModal workspace for the fix to reach prod.processed.mp4>~50MB) also need the Supabaseproject's global upload size limit raised (dashboard setting).