Fix case where filesystem store future dropping causes issues #496
Conversation
allada
force-pushed
the
fix-filesystem-race-condition
branch
from
f977690
to
bd61f02
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @adam-singer)
|
|
||
| /// Regression test for: https://github.com/TraceMachina/nativelink/issues/495. | ||
| #[tokio::test] | ||
| async fn update_file_future_drops_before_rename() -> Result<(), Error> { |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @allada)
nativelink-store/src/filesystem_store.rs line 583 at r1 (raw file):
let from_path = encoded_file_path.get_file_path(); // Internally tokio spawns fs commands onto a blocking thread anyways. // Since we are already on a blocking thread, we just need the `fs` wrapper to manage
maybe spell out std::fs::rename.
Given its the std::fs::rename it manages open-file permit (via FnOnce) or is the call_with_permit missing here?
nativelink-util/src/fs.rs line 246 at r1 (raw file):
/// Acquire a permit from the open file semaphore and call a raw function. #[inline] pub async fn call_with_permit<R>(func: impl FnOnce() -> Result<R, Error>) -> Result<R, Error> {
Defined but not used?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @allada)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @allada)
allada
force-pushed
the
fix-filesystem-race-condition
branch
from
bd61f02
to
76020a4
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Dismissed @MarcusSorealheis from a discussion.
Reviewable status:complete! 1 of 1 LGTMs obtained
allada
force-pushed
the
fix-filesystem-race-condition
branch
from
76020a4
to
4fd99b6
Compare
In a rare case where a file is uploaded, but while the rename() function is called the future is dropped the filesystem store can get into a bad state. fixes: #495
allada
force-pushed
the
fix-filesystem-race-condition
branch
from
4fd99b6
to
1f809a8
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04
There was a problem hiding this comment.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04
In a rare case where a file is uploaded, but while the rename() function is called the future is dropped the filesystem store can get into a bad state.
fixes: #495
This change is