Fix FS storage races#34452
Conversation
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
🟢 |
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate file-system storage race conditions in the FS external storage wrapper, improving durability guarantees for object writes.
Changes:
- Reworked
PutObjectto avoid a truncate/write race by reusing the multipart session locking/truncation pattern. - Added directory
fsync(viaFlush()on the parent directory fd) after object creation and after multipart completion rename. - Improved logging for failed cleanup during
AbortMultipartUpload.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ActiveUploads.erase(it); | ||
| session.File.Close(); |
There was a problem hiding this comment.
session is taken by reference (auto& session = it->second) and then ActiveUploads.erase(it) is called before session.File.Close(). Erasing the map element destroys the session, so the subsequent session.File.Close() is a use-after-free/UB. Close the file (and finish any other session access) before erasing, or move the TFile out before erase.
| ActiveUploads.erase(it); | |
| session.File.Close(); | |
| session.File.Close(); | |
| ActiveUploads.erase(it); |
| static void FsyncParentDir(const TString& filePath) { | ||
| TFsPath parent = TFsPath(filePath).Parent(); | ||
| TFile dirFd(parent.GetPath(), RdOnly | Seq); | ||
| dirFd.Flush(); |
There was a problem hiding this comment.
FsyncParentDir opens a directory with TFile(parent.GetPath(), RdOnly | Seq) and calls Flush() (fsync). On Windows, TFileHandle uses CreateFileW without FILE_FLAG_BACKUP_SEMANTICS, so opening a directory path will fail and make Put/Complete operations error out. Consider guarding this code for non-Windows platforms or implementing directory-open with the required Windows flags (or using a dedicated helper in util).
| static void FsyncParentDir(const TString& filePath) { | |
| TFsPath parent = TFsPath(filePath).Parent(); | |
| TFile dirFd(parent.GetPath(), RdOnly | Seq); | |
| dirFd.Flush(); | |
| static void FsyncParentDir(const TString& filePath) { | |
| #if defined(_win_) | |
| (void)filePath; | |
| #else | |
| TFsPath parent = TFsPath(filePath).Parent(); | |
| TFile dirFd(parent.GetPath(), RdOnly | Seq); | |
| dirFd.Flush(); | |
| #endif |
Changelog entry
...
Changelog category
Description for reviewers
fsyncдля директорий.