fix(conda): avoid temp file collisions during parallel package downloads#9293
fix(conda): avoid temp file collisions during parallel package downloads#9293
Conversation
Greptile SummaryThis PR fixes a temp-file collision race in Confidence Score: 5/5Safe to merge — the fix is correct and both previously flagged issues have been resolved. All findings are P2 or lower. The atomic counter correctly ensures unique temp paths within a process; Ordering::Relaxed is appropriate here since only uniqueness (not ordering) is required. The race-tolerant finalize cleanly handles the cross-device and concurrent-rename edge cases. The Ok(()) and temp-file cleanup that were previously missing are now both present. The unit test validates path uniqueness end-to-end. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant A as Installer A
participant B as Installer B
participant FS as Filesystem
par Parallel downloads
A->>FS: download_file → libgcc.tmp.1234.0
B->>FS: download_file → libgcc.tmp.1234.1
end
A->>FS: verify_checksum(tmp.1234.0) ✓
B->>FS: verify_checksum(tmp.1234.1) ✓
A->>FS: rename(tmp.1234.0 → libgcc.conda) ✓
A-->>A: return Ok(())
B->>FS: rename(tmp.1234.1 → libgcc.conda) ✗ (EEXIST/EXDEV)
B->>FS: remove_all(tmp.1234.1)
B->>FS: dest.exists() && verify_checksum(dest) ✓
B-->>B: return Ok(()) [race-tolerant fallback]
Reviews (3): Last reviewed commit: "fix(conda): avoid temp file collisions" | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces unique temporary download paths for Conda packages by incorporating the process ID and an atomic counter. It also enhances the file finalization logic to handle race conditions between concurrent installers by verifying the checksum of an existing destination file if a rename operation fails. Additionally, a unit test has been included to verify the uniqueness of the generated temporary paths. I have no feedback to provide.
63a8cb8 to
b56ec1d
Compare
during parallel package downloads The old code derived the temporary filename from the destination path and the PID. When multiple conda tools are installed concurrently in the same process (as in GH CI with `mise-action`), and the tools share a common transitive dependency (e.g. `libgcc`, `_openmp_mutex`, `libgomp`), all parallel installers produce the exact same `temp` path for the same shared package, because they all have the same PID and the same `dest`. This leads to multiple concurrent tasks all downloading to the same `temp` file and then competing to rename/delete it, which results in `No such file or directory` errors during `file::rename(&temp, dest)`. Besides adding an atomic counter to the `temp` path to make it unique, we add a second line of defense with a race-tolerant finalize to cover edge cases where `file::rename()` fails for any reason, i.e. another concurrent installer won the race (cross-device rename (`EXDEV`) or possible filesystem/permission errors unrelated to concurrency). Co-authored-by: salim-b <20040931+salim-b@users.noreply.github.com>
b56ec1d to
cff9486
Compare
issue has been fixed upstream in mise and released with v2026.4.19, cf. jdx/mise#9293
The old code derived the temporary filename from the destination path and the PID. When multiple conda tools are installed concurrently in the same process (as in GH CI with
mise-action), and the tools share a common transitive dependency (e.g.libgcc,_openmp_mutex,libgomp), all parallel installers produce the exact sametemppath for the same shared package, because they all have the same PID and the samedest. This leads to multiple concurrent tasks all downloading to the sametempfile and then competing to rename/delete it, which results inNo such file or directoryerrors duringfile::rename(&temp, dest).Besides adding an atomic counter to the
temppath to make it unique, we add a second line of defense with a race-tolerant finalize to cover edge cases wherefile::rename()fails for any reason, i.e. another concurrent installer won the race (cross-device rename (EXDEV) or possible filesystem/permission errors unrelated to concurrency).