fix(snapshots): Chunk image uploads to avoid fd exhaustion and 413 errors#3249
Conversation
|
|
||
| const IMAGE_EXTENSIONS: &[&str] = &["png", "jpg", "jpeg"]; | ||
| const MAX_PIXELS_PER_IMAGE: u64 = 40_000_000; | ||
| const UPLOAD_BATCH_SIZE: usize = 100; |
There was a problem hiding this comment.
open to your guys' thoughts, but through some testing, seems to be a good number
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
NicoHinderling
force-pushed
the
fix/chunk-snapshot-uploads
branch
from
3b2f5fa to
938a884
Compare
…rors Uploading hundreds of images fails in two ways: all file handles are opened upfront causing EMFILE (too many open files), and the objectstore client batches everything into one HTTP request causing 413 Payload Too Large from the server. Split upload_images() into two phases: pre-process all images without opening files (collision detection, hashing, metadata), then upload in chunks of 100. This bounds both open file descriptors and HTTP request payload size regardless of total image count. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
NicoHinderling
force-pushed
the
fix/chunk-snapshot-uploads
branch
from
938a884 to
835e271
Compare
NicoHinderling
changed the base branch from
graphite-base/3249
to
03-26-perf_snapshots_parallelize_image_hashing_with_rayon
|
synced offline with @lcian , putting in draft mode since while fd limit is real, we on client side apparently shouldn't be handling the batching |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| let mut error_count = 0; | ||
| for error in errors { | ||
| let error = anyhow::Error::new(error); | ||
| for error in &errors { |
There was a problem hiding this comment.
Upload error messages lose cause chain detail
Low Severity
The old code wrapped each upload error in anyhow::Error::new(error) before formatting with {error:#}, which displays the full error cause chain (e.g., "request failed: 413 Payload Too Large"). The new code removed this wrapping, so {error:#} on the raw error type likely only shows the top-level message since most non-anyhow types ignore the alternate flag. This loses valuable diagnostic context, especially for the 413 errors this PR aims to fix. Computing error_count via .len() before the loop would allow iterating by value and preserving the anyhow wrapping.
1 participant
Uploading hundreds of images via
build snapshotsfails in two ways:Both failures were confirmed with a real 753-image upload. 734 out of 753 images failed with 413 errors.
This PR splits
upload_images()into two phases:session.many()builder, and send. This bounds both open file descriptors and HTTP request payload size.Also includes minor code quality improvements: simplified scope extraction, iterator-based collision formatting, and removal of redundant bindings.