Skip to content

download: prevent conflict when 2 processes download the same file - #28803

Closed
ngxson wants to merge 2 commits into
masterfrom
xsn/download_flock
Closed

ngxson wants to merge 2 commits into
masterfrom
xsn/download_flock

Conversation

@ngxson

@ngxson ngxson commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator

Overview

Ref discussion: #28555 (comment)

Requirements

@ngxson
ngxson requested a review from a team as a code owner September 12, 2026 10:02
@ngxson
ngxson requested review from ServeurpersoCom and angt and removed request for a team September 12, 2026 10:02
@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Great, I'm running my repros on it + quick windows build/test.

@ServeurpersoCom ServeurpersoCom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Built and tested on Windows, no regression, and on Linux I confirmed this fixes real corruption on top of the CI annoyance: on master two servers sharing one LLAMA_CACHE produce a blob exactly twice the right size, while with this PR the sha256 matches its filename again and the full server suite passes with 4 xdist workers.

Nits, none blocking: remove(path_progress) comes after the early return -1 so a failed download leaves the lock file behind, and the std::rename just below still fails on Windows when the destination exists, same for the etag rewrite in write_file. I have the cross-platform fix ready, std::filesystem::rename on both sites, it went away with the revert in #28555, happy to push it here or as its own PR, whichever you prefer.

@angt

angt commented Sep 12, 2026

Copy link
Copy Markdown
Member

For cached downloads, I think the cache "module" should define the locking scheme, we should match the .locks layout for Hugging Face, the appropriate one for Docker or a future ModelScope cache (#24716)

@angt

angt commented Sep 12, 2026

Copy link
Copy Markdown
Member

That feels like a lot of complexity (and potentially error-prone) just to share download progress. Couldn't we simply wait to acquire the appropriate lock (generic or provided by the cache backend), then check whether the download completed?

@ServeurpersoCom

ServeurpersoCom commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Agreed the lock belongs to the cache module with the HF .locks layout. Progress is not optional for us though, a server download can run for tens of minutes and the WebUI has to show something.
But I think there may be a simpler way to get it: the in-progress file already grows as bytes arrive, so its size is the received count, and the waiter knows the total from its own HEAD, which makes it a stat in the wait loop instead of a shared record format.

@ngxson

ngxson commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

Hmm ok right, this case is rare in general so probably progress reporting is an unnecessary complexity part.

will remove that logic and simply replace with a log message "file is downloaded by another process, waiting..."

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

I retested by starting two llama-server at the same time with the same LLAMA_CACHE and the same -hf model. On master one of them dies with unable to rename and the blob that survives is exactly twice the right size, which is easy to check since HF names blobs by their hash: the sha256sum of the file should equal its filename, and on master it does not. With your PR it matches again and one process waits for the other. Two different models never contend, they lock different paths.

Two things left:

Across six runs I still lose a server twice, on get_repo_commit: error: failed to write file: .../refs/main. That is safe_write_file in hf-cache.cpp going through a shared path + ".tmp", the same bug one level down, and a per-process temp name is enough since there is nothing to resume and both write the same content.

And the std::rename just below still fails on Windows when the destination exists, same for the etag rewrite in write_file, where std::filesystem::rename has the POSIX semantics everywhere.

@ngxson

ngxson commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

@angt can you have a look?

@ngxson

ngxson commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator Author

per discussion via DM, let's push your changes directly here @angt . I'll temporary move this PR to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants