Fix the raciness of the filesystem store (in progress) - #2715
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Some of the pull request description still needs filling in:
Edit the description and this check re-runs on its own. The sections exist because they are the parts a reviewer cannot get from the diff: why the change is needed, how you know it works, and what breaks if it is wrong. |
|
Thanks @jiri-szkandera-cdn77 we will review. Also great to hear you are using it in production! We would love to do a case study. Can you email me on sumon at tracemachina dot com to say hi |
3029fe0 to
b27cbfd
Compare
b27cbfd to
44c8da1
Compare
44c8da1 to
e648af7
Compare
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
There is a question of how to handle the migration. I've implement it for digests, but I'm not sure what the STR folder is used for exactly, and whether it's okay to consider '-' as a special character. Do you have any preference?
|
I've implemented the POC fix, and I'm currently testing it with our builds. So far, it looks like it has solved our problems. |
| // Sometimes we get files to emplace that are identical to the existing files | ||
| // Due to the evict/remove/replace cycle taking some amount of time, we actually | ||
| // want to drop these | ||
| if check_duplicate_files(&evicting_map, &key, &entry).await? { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| evicting_map | ||
| .insert(key.borrow().into_owned().into(), entry.clone()) | ||
| .await; | ||
|
|
||
| // The insert might have resulted in an eviction/unref so we need to check | ||
| // it still exists in there. But first, get the lock... | ||
| let mut encoded_file_path = entry.get_encoded_file_path().write().await; | ||
| // Then check it's still in there... | ||
| if evicting_map.get(&key).await.is_none() { | ||
| info!(%key, "Got eviction while emplacing, dropping"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let final_path = get_file_path_raw( | ||
| &PathType::Content, | ||
| encoded_file_path.shared_context.as_ref(), | ||
| &key, | ||
| encoded_file_path.generation, | ||
| ); | ||
|
|
||
| let from_path = encoded_file_path.get_file_path(); |
There was a problem hiding this comment.
This part can now be simplified.
@Jirixek Thank you for your contribution. This will need signing before we can accept this code. |
@palfrey I've signed the CLA and clicked the recheck buttom about 2-3 times, but the |
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
I would say this PR fixes a real race, but at the cost of startup time. The directory traversal and metadata work for d/ is approximately doubled on every startup—not only during the first migration. For a cache containing millions of files, that could materially increase restart time and metadata I/O.
The feedback addresses some of the correctness concerns I have about the change. Overall, great contribution and let me know how I can help you get it over the line.
| Ok(()) | ||
| } | ||
|
|
||
| async fn move_old_cache_2( |
There was a problem hiding this comment.
This migration only handles d/, but previous releases stored string entries as s/ without a generation. Startup subsequently parses every s/ filename with key_and_generation_from_file. Keys without - fail and are deleted, while a legacy key such as foo-1 is silently loaded as key foo, generation 1. String keys are arbitrary and are used by BEP and scheduler storage, so - cannot safely distinguish old and new layouts. Please introduce an unambiguous format/version marker and add upgrade tests covering string keys with no hyphen, hyphens, and numeric suffixes.
| let path_root = format!("{}/{folder}", shared_context.content_path); | ||
|
|
||
| let mut max_generation = 0; | ||
|
|
There was a problem hiding this comment.
The race being fixed intentionally allows old and new generation files for one key to coexist until stale unref runs. If the process exits during that window, both remain. Here's the flow roughly:
- On restart,
read_filesusesbuffer_unordered - Then, this loop inserts them in arbitrary completion order
- The second insertion replaces and unrefs the first.
For mutable string keys this can resurrect stale data and delete the newest generation.
Please consider grouping entries by logical key and deterministically retain the authoritative generation, with a restart regression test containing two generations for one string key. If “newest” means publish order, generation also needs to be assigned at publication rather than when the temp file is created.
| &PathType::Content, | ||
| encoded_file_path.shared_context.as_ref(), | ||
| &key, | ||
| encoded_file_path.generation, |
There was a problem hiding this comment.
This only checks that some entry exists for the key. A concurrent same-key upload can replace entry before this check, leaving a different Arc in the map. The stale task then publishes its generation-specific content file and marks it Content. If the replacement’s unref observed it while it was still Temp, nobody removes the resulting content file and it escapes eviction accounting indefinitely. Please require Arc::ptr_eq with the current map entry before renaming, leaving stale entries as Temp so drop cleans them up.
| let from_file: OsString = format!("{folder_path}/{file_name}").into(); | ||
| let to_file: OsString = format!("{folder_path}/{file_name}-0").into(); | ||
|
|
||
| if let Err(err) = rename_fn(&from_file, &to_file) { |
There was a problem hiding this comment.
This one is an edge case but customers are asking about read-only more and more.
When this rename fails on a read-only content volume, startup only logs a warning. The unchanged legacy filename is then rejected by the generation parser and never added to the map, so an upgraded read-only cache stops serving all legacy blobs. This conflicts with the store’s existing read-only startup behavior. Please support reading legacy paths without mutating them, OR maybe you could provide another migration mechanism and cover a read-only upgrade in tests.
6 participants
What and why
First of all, thank you so much for your work on Nativelink. We’ve been using it in prod for a couple of years now.
We sometimes see that when the filesystem store is under pressure (i.e., near its capacity), errors start to occur which are resolved only by restarting Nativelink. I finally got around to debugging why these errors happen.
The bug
In the logs, I see the following events.
The following happens in code. The eviction map and the file renaming (CAS → tmp dir) are two separate tokio tasks that are not synchronised. I suppose this is to make FS CAS performant. However, because file renaming is a separate task, it can run at any time in the future, i.e., there may be a significant time window between removing an entry from the eviction map and actually renaming a file.
If during this time window a new file is created, i.e., an entry is added and the file is moved from tmp to the CAS dir, the original file (now pending for deletion) in CAS is overwritten by a new file. After this, the cleanup task runs, moving the file out of the CAS dir → tmp dir. This leaves the eviction map with an entry that no longer has an underlying file in the CAS dir.
The proposed solution
I suggest stamping every file with a generation number on upload. A generation can be a current timestamp, but any monotonic u64 would suffice. The generation would be a part of the filename and stored in
EncodedFilePath.This would solve the problem because adding/removing entries to the eviction map and spawning tasks to move files from and to CAS would be completely independent actions. The lifecycle of a file (CREATED → UPLOADED_TO_EVICTION_MAP → REMOVED_FROM_EVICTION_MAP) could be encapsulated in
FileEntryImpl. Moreover, I believe we could clean up the code that tries but fails to mitigate this problem, e.g.,check_duplicate_files(although I’m not entirely sure whether there are other reasons whycheck_duplicate_filesexists).I’m willing to make a patch, since it affects our production. Before I make a patch, I'd like to hear your thoughts and confirm I'm not missing anything. I created a test to demonstrate the bug (sorry for the AI comments; I’ll remove them in a final patch).
This change is