Repository navigation
Publish downloaded vocab files atomically - #157
Open
ishapira-positron wants to merge 3 commits into
Open
ishapira-positron wants to merge 3 commits into
ishapira-positron wants to merge 3 commits into
Conversation
download_or_find_cached_file caches vocab files in one directory shared
by every process on the host ($TMPDIR/tiktoken-rs-cache by default), and
load_remote_file streamed the download straight into the final cache
path. Processes that start with a cold cache at the same time (a test
suite running its binaries in parallel, several servers starting
together) download the same file at once, so one could read another's
partial file ("invalid tiktoken vocab file: failed to parse rank at line
...") or find it gone after a peer deleted it as a hash mismatch.
The download now goes to a temporary file unique to the process and call,
in the cache directory, and is renamed over the cache path only once it
is complete, synced to disk and matches the expected hash. On any failure
the temporary file is removed and the cache path is left as it was, so a
reader sees either no file or a complete, verified one.
write_file_atomically takes any Read so the tests can drive it without a
network: a download that fails part way or has the wrong hash leaves the
published file untouched and no temporary file behind, and a reader
polling the path while four writers republish it only ever finds no file
or the whole of it. Restoring the in-place write fails both tests.
|
Note Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
write_file_atomically_never_exposes_a_partial_file cleared the reader's polling flag only after every writer joined cleanly. A writer error panicked the join, the unwind skipped the store, and thread::scope then waited forever on the reader, so the error paths the test guards (a temporary name that is not unique per call, a cross-device rename, any I/O error) hung with no diagnostic. A guard now clears the flag on unwind; with a non-unique temporary name the test fails in seconds with the writer's panic instead of running until killed. The reader's count of complete reads was returned and discarded, so a reader first scheduled after the writers finished passed the test with no evidence. A barrier now starts the reader before any writer and the test asserts it saw at least one published file; with the reader held back until the writers finish, the test now fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A temporary download used a predictable name, so a writer whose name was already taken assumed the owner dead and removed the entry. The owner can be live: a process with the same pid in another pid namespace, or one on another host sharing the cache directory. Removing its file lets it rename our half-written download into place, the race this change exists to stop. The temporary name now comes from a per-process candidate list tried with create_new, a taken name is skipped rather than removed, and temporaries no writer has touched for an hour are swept before each download. The cache file is no longer removed on a hash mismatch either: the atomic rename replaces it, and a removal could take a peer's complete file instead of the partial one. A reader that finds the file gone between the existence check and the open, which only an unpatched writer can cause, downloads instead of failing.
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
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.
download_or_find_cached_file caches vocab files in one directory shared
by every process on the host ($TMPDIR/tiktoken-rs-cache by default), and
load_remote_file streamed the download straight into the final cache
path. Processes that start with a cold cache at the same time (a test
suite running its binaries in parallel, several servers starting
together) download the same file at once, so one could read another's
partial file ("invalid tiktoken vocab file: failed to parse rank at line
...") or find it gone after a peer deleted it as a hash mismatch.
The download now goes to a temporary file unique to the process and call,
in the cache directory, and is renamed over the cache path only once it
is complete, synced to disk and matches the expected hash. On any failure
the temporary file is removed and the cache path is left as it was, so a
reader sees either no file or a complete, verified one.
write_file_atomically takes any Read so the tests can drive it without a
network: a download that fails part way or has the wrong hash leaves the
published file untouched and no temporary file behind, and a reader
polling the path while four writers republish it only ever finds no file
or the whole of it. Restoring the in-place write fails both tests.