feat: bounded-memory file uploads for huge files (1GB-100GB+) - #20
Merged
Merged
Conversation
Add ChunkSpill mechanism that writes encrypted chunks to a temp directory during encryption, keeping only 32-byte addresses in memory. Chunks are read back one wave (64 chunks) at a time for upload, bounding peak RSS to ~256 MB regardless of file size. Key changes: - ChunkSpill struct: temp-dir-based buffer with Drop cleanup - Disk space pre-check before upload (fs2::available_space) - InsufficientDiskSpace error variant - file_prepare_upload refactored to use spill (with honest docs that PreparedUpload still holds all chunks in memory) - Wave-level progress logging during encryption and upload - Fixed direct indexing (prepared[0]) in batch_pay - Fixed inline paths and swallowed errors per review feedback - Integer arithmetic for disk space headroom (no float precision loss) E2E tests proving bounded memory: - 1 GB: 260 chunks, RSS increase 185 MB (limit 512 MB) - 4 GB: 1029 chunks, RSS increase 132 MB (limit 512 MB) - Both verified byte-for-byte after round-trip - Uses mach_task_basic_info (current RSS, not peak) on macOS Memory does NOT scale with file size — 4 GB uses less RSS increase than 1 GB, proving the wave-based spill mechanism works correctly.
…rity Fixes from multi-agent review (2 Claude Opus + 3 Codex gpt-5.4 xhigh): - Use unwrap_or_else(PoisonError::into_inner) on all 3 mutex lock sites in spawn_file_encryption instead of silently skipping on poison. A poisoned mutex previously caused read errors to be silently dropped, which could produce a truncated DataMap. - Move xor_name::XorName to top-level use import (was inline in file_download closure). - Revert merkle chunk limit guard: gas savings from merkle payments far outweigh the O(n) proof memory overhead (~86 KB per chunk from ML-DSA-65 signatures in candidate pools). For a 100 GB file (~25k chunks), proof memory is ~2 GB which is acceptable.
Fixes from Codex gpt-5.4 xhigh production review: - batch.rs: move evmlib::contract::payment_vault::get_market_price and evmlib::wallet::PayForQuotesError to top-level use imports (PROD-006) - file.rs: ChunkSpill::new() now uses create_dir instead of create_dir_all, so creation fails if the directory already exists. This prevents silent reuse of a stale/symlinked spill directory (PROD-007) Not addressed (architectural, separate PRs): - PROD-001/002/003: Resumable uploads after partial payment failure - PROD-004: CancellationToken threading through upload APIs
Fixes from Codex gpt-5.4 xhigh test review: - Replace repeating 256-byte pattern with seeded xorshift64 PRNG for incompressible test data. Repeating patterns compress to tiny chunks, making the wave buffer test pass trivially without stressing the ~256 MB wave buffer. Incompressible data produces full-size ~4 MB encrypted chunks. (TEST-003) - RSS measurement failure now panics on macOS/Linux instead of silently passing. This prevents CI from quietly skipping the bounded-memory assertion if the FFI probe ever drifts. (TEST-002) - Tighten 4 GB chunk count assertion from >= 800 to >= 1000 (actual expected: ~1026 chunks at MAX_CHUNK_SIZE). (TEST-004)
If std::fs::rename fails after the download temp file is fully written, the previous code used ? which exited without cleanup, leaking the temp file. Now explicitly handles the rename error, removes the temp file, and then returns the error. Found by Codex gpt-5.4 xhigh review round 3 (R3-001).
mickvandijke
left a comment
mickvandijke
left a comment
Member
There was a problem hiding this comment.
Review: Duplicate chunk addresses cause double-upload and inflated metrics
ChunkSpill::push() (file.rs:73-79) uses compute_address(content) as the filename. If two chunks have identical content (which self-encryption can produce for repeated data), the second fs::write silently overwrites the first file on disk, but both addresses are appended to self.addresses. Consequences:
total_bytesdouble-counts — inflatesavg_chunk_size(), which feeds into quoting metrics viapay_for_merkle_batch- Wave uploads read and re-upload the same chunk twice — wasted bandwidth and gas
The old code had the same latent issue (duplicate Bytes in the vec), but the new code makes it slightly worse because total_bytes is now used for the avg_chunk_size quoting metric.
Suggested fix: Deduplicate in push() — e.g. track seen addresses in a HashSet and skip the write + total_bytes increment if the address already exists (still preserve the entry in addresses if ordering matters for the DataMap, or skip it entirely if duplicates are redundant on the network).
Alternatively, document that this is acceptable because self-encryption rarely produces duplicates for real (incompressible) files.
ChunkSpill::push() now tracks seen addresses in a HashSet and skips the write, total_bytes increment, and address recording for duplicate content. This prevents: - Double-uploading the same chunk (wasted bandwidth and gas) - Inflated avg_chunk_size() feeding into quoting metrics - Inflated chunk count in progress logging Self-encryption rarely produces duplicate chunks for incompressible data, but repeated/patterned content could trigger this.
jacderida
added a commit
to jacderida/ant-client
that referenced
this pull request
May 22, 2026
…t DHT lookups in deferred retry Two failure modes surfaced on PROD-DL-04 (20-file droplet run on the home-download-fixes branch), both small robustness gaps: 1. Run WithAutonomi#13 (1.77 GB, failed at the first chunk): a single storer responded `ChunkGetResponse::Error("Chunk verification failed ...")` for one chunk. `chunk_get_from_peer` classified that as `Error::Protocol`, and `chunk_get_try_close_group`'s per-peer loop's catch-all `Err(e) => return Err(e)` propagated it out immediately — aborting the close-group sweep without trying the other 6 peers, despite the other peers possibly having a clean copy. From there the error escalated through `chunk_get_observed` into the batch consumer, which treats any `Err` (not `Ok(None)`) as fatal. Fix: add an `Err(Error::Protocol(_))` arm to the per-peer loop alongside `Timeout` and `Network`. Counted separately as `protocol_err` so the summary log still distinguishes "peer corrupted" from "peer unreachable". Continues the sweep. 2. Run WithAutonomi#20 (2.75 GB): 7 chunks got deferred (the existing deferred-retry mechanism in file.rs from 4ce1ca7). On the first deferred attempt, `chunk_get_observed` returned `Error::InsufficientPeers("DHT returned no peers for target address")` — `close_group_peers` came back empty because the DHT walk transiently failed to find any candidates. The deferred retry loop treated that as fatal and aborted the whole download instead of falling through to the next attempt's longer sleep. Fix: in the deferred retry loop, an `Err(e)` from `chunk_get_observed` logs and continues to the next attempt (with backoff), same as `Ok(None)`. Only a chunk that exhausts all DEFERRED_RETRY_ATTEMPTS attempts becomes fatal. Both fixes are in the same "single transient peer/lookup issue should not abort a multi-hundred-chunk download" theme as the deferred-retry mechanism itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
InsufficientDiskSpaceerror) usingfs2::available_spacewith integer arithmetic for the 10% headroom calculation.file_prepare_upload(external-signer path) to use spill for encryption, with honest docs thatPreparedUploadstill holds all chunks in memory (inherent to the two-phase protocol).prepared[0]direct indexing inbatch_pay— replaced with iterator.sum().evmlib::contract::...,xor_name::XorName) — moved to top-leveluseimports.spawn_file_encryption— 3 sites now useunwrap_or_else(PoisonError::into_inner)instead of silently skipping.datamap_tx.send()and temp file cleanup now log warnings.file_download.create_dir(notcreate_dir_all) to reject pre-existing directories.Proven test results
Memory does not scale with file size — 4 GB uses less RSS increase (132 MB) than 1 GB (185 MB), proving the wave-based spill mechanism works.
E2E tests (
e2e_huge_file.rs)test_huge_file_upload_download_1gb— upload, download, byte-verify 1 GB, assert RSS < 512 MBtest_huge_file_upload_download_4gb— same for 4 GB with tighter chunk count assertion (>= 1000)test_large_file_64mb_round_trip— fast CI-friendly 64 MB round-triptest_file_upload_disk_space_check— verifies pre-flight disk space checkmach_task_basic_info.resident_sizeon macOS (current RSS, not peak) — panics on measurement failure on supported platformsReview history
3 review rounds, 12 agents total (6 Claude Opus + 6 Codex gpt-5.4 xhigh). Final round verdict: zero remaining production code violations across all project rules (no unwrap/expect/panic, no direct indexing, no inline paths, no swallowed errors).
Test plan
cfd(cargo fmt + clippy) passes with zero warnings