Conversation
sveitser
commented
Feb 28, 2026
sveitser
commented
- Retry
- Concurrent download de-duplication
- Nix flake
- Version bump
Use a .part file as both temp file for atomic rename and flock target to deduplicate concurrent downloads. Retries with exponential backoff on transient network errors and zero-byte responses.
feat: retry downloads
alxiong
left a comment
alxiong
left a comment
There was a problem hiding this comment.
ACK the robustness improvement and dedup-concurrent tests look good.
only minor nits.
| Ok(T::deserialize_uncompressed_unchecked(&bytes[..])?) | ||
| } | ||
|
|
||
| pub(crate) fn download_url_to_file( |
There was a problem hiding this comment.
I don't want to force nix usage. this package is simple enough to have any setup or lib dependency.
most systems should have openssl installed, pure cargo toolchain should be sufficient?
There was a problem hiding this comment.
Pull request overview
This PR adds retry and concurrency-safe de-duplication to SRS downloads, introduces Nix flake-based dev environment support, and bumps the crate version.
Changes:
- Add
download_url_to_filewith retry/backoff and file-lock-based de-duplication for concurrent downloads. - Add unit tests covering download success/failure, retries, and concurrent de-dup behavior.
- Add
flake.nix/flake.lockand bump crate version + dependencies (fs2,mockito).
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/load.rs | Implements retrying download + concurrent de-dup via .part file locking; adds tests. |
| flake.nix | Adds Nix flake dev shell for consistent Rust tooling + deps. |
| flake.lock | Pins Nix inputs for reproducible dev environments. |
| Cargo.toml | Bumps version; adds fs2 and test dependency mockito. |
| Cargo.lock | Updates lockfile for new dependencies and version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,21 +1,18 @@ | |||
| //! Utils for persisting serialized data to files and loading them into memroy. | |||
There was a problem hiding this comment.
Spelling: module doc comment says "memroy"; should be "memory".
| //! Utils for persisting serialized data to files and loading them into memroy. | |
| //! Utils for persisting serialized data to files and loading them into memory. |
| create_dir_all(dest.parent().context("no parent dir")?) | ||
| .context("Unable to create directory")?; | ||
|
|
||
| // .part file serves double duty: temp file for atomic rename and flock | ||
| // target to deduplicate concurrent downloads. Not truncated on open so a | ||
| // second opener doesn't clobber an in-progress write. Left on disk after | ||
| // failure -- harmless, the next caller overwrites it. | ||
| let part_path = { | ||
| let mut p = dest.as_os_str().to_owned(); | ||
| p.push(".part"); | ||
| PathBuf::from(p) | ||
| }; | ||
| let part_file = OpenOptions::new() | ||
| .write(true) | ||
| .create(true) | ||
| .truncate(false) | ||
| .open(&part_path)?; | ||
| part_file.lock_exclusive()?; | ||
|
|
||
| // Another thread may have completed the download while we blocked on the lock. | ||
| if dest.exists() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
download_url_to_file creates/opens the .part file before checking dest.exists(). If dest already exists and .part does not, this leaves behind an empty .part file even though no download occurs. Consider adding a fast-path if dest.exists() { return Ok(()) } before opening/creating .part (and keep the post-lock recheck for races), or explicitly remove the .part file on the early-return path.
| let mut buf: Vec<u8> = Vec::new(); | ||
| match ureq::get(url).call() { | ||
| Ok(resp) => match resp.into_reader().read_to_end(&mut buf) { | ||
| Ok(_) if buf.is_empty() => { | ||
| last_err = Some(anyhow!("zero-byte response")); | ||
| }, | ||
| Ok(_) => { | ||
| part_file.set_len(0)?; | ||
| (&part_file).write_all(&buf)?; | ||
| fs::rename(&part_path, dest)?; | ||
| return Ok(()); |
There was a problem hiding this comment.
The download path buffers the entire response into a Vec<u8> before writing to disk. For large SRS assets this can cause very high peak memory usage (and repeats the allocation on each retry). Prefer streaming the response reader directly into the .part file (and optionally hashing/size-checking as you stream) to keep memory bounded.
| for attempt in 0..=max_retries { | ||
| let mut buf: Vec<u8> = Vec::new(); | ||
| match ureq::get(url).call() { | ||
| Ok(resp) => match resp.into_reader().read_to_end(&mut buf) { | ||
| Ok(_) if buf.is_empty() => { | ||
| last_err = Some(anyhow!("zero-byte response")); | ||
| }, | ||
| Ok(_) => { | ||
| part_file.set_len(0)?; | ||
| (&part_file).write_all(&buf)?; | ||
| fs::rename(&part_path, dest)?; | ||
| return Ok(()); | ||
| }, | ||
| Err(e) => last_err = Some(anyhow::Error::from(e)), | ||
| }, | ||
| Err(e) => last_err = Some(anyhow::Error::from(e)), | ||
| } | ||
|
|
||
| if attempt < max_retries { | ||
| let backoff = base_backoff * 2u32.saturating_pow(attempt as u32); | ||
| thread::sleep(backoff); | ||
| } | ||
| } |
There was a problem hiding this comment.
The function holds an exclusive file lock across the entire network request and any backoff sleeps. Since ureq::get(url).call() has no explicit connect/read timeout here, a hung download can block all concurrent callers indefinitely. Consider using a configured ureq::Agent with reasonable timeouts (and/or a maximum total elapsed time) so lock-holding failures resolve predictably.
