Skip to content

Fix FS storage races #34452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stanislav-shchetinin wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −7
Open

Fix FS storage races #34452

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions ydb/core/wrappers/fs_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ class TFsOperationActor : public TActorBootstrapped<TFsOperationActor> {
return RequiresKey<TEvResponse>::value;
}

static void FsyncParentDir(const TString& filePath) {
TFsPath parent = TFsPath(filePath).Parent();
TFile dirFd(parent.GetPath(), RdOnly | Seq);
dirFd.Flush();
Comment on lines +62 to +65
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
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

Copilot uses AI. Check for mistakes.
}

template<typename TEvResponse>
void ReplySuccess(const NActors::TActorId& sender, const std::optional<TString>& key) {
typename TEvResponse::TAwsResult awsResult;
Expand Down Expand Up @@ -202,11 +208,11 @@ class TFsOperationActor : public TActorBootstrapped<TFsOperationActor> {
TFsPath fsPath(key);
fsPath.Parent().MkDirs();

TFile file(fsPath.GetPath(), CreateAlways | WrOnly);
file.Flock(LOCK_EX | LOCK_NB);
file.Write(body.data(), body.size());
file.Flush();
file.Close();
TMultipartUploadSession session(key);
session.File.Write(body.data(), body.size());
session.File.Flush();
FsyncParentDir(key);
session.File.Close();
ReplySuccess<TEvPutObjectResponse>(ev->Sender, key);
} catch (const TSystemError& ex) {
if (!HandleFileLockError<TEvPutObjectResponse>(ex, ev->Sender, key, "PutObject")) {
Expand Down Expand Up @@ -503,6 +509,7 @@ class TFsOperationActor : public TActorBootstrapped<TFsOperationActor> {
session.File.Flush();

NFs::Rename(incompleteKey, key);
FsyncParentDir(key);
session.File.Close();

FS_LOG_I("CompleteMultipartUpload"
Expand Down Expand Up @@ -545,9 +552,14 @@ class TFsOperationActor : public TActorBootstrapped<TFsOperationActor> {
auto& session = it->second;
const TString filePath = session.Key;

NFs::Remove(filePath);
session.File.Close();
bool removed = NFs::Remove(filePath);
if (!removed) {
FS_LOG_W("AbortMultipartUpload: failed to delete incomplete file"
<< ": uploadId# " << uploadId
<< ", file# " << filePath);
}
ActiveUploads.erase(it);
session.File.Close();
Comment on lines 561 to +562
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
ActiveUploads.erase(it);
session.File.Close();
session.File.Close();
ActiveUploads.erase(it);

Copilot uses AI. Check for mistakes.

FS_LOG_I("AbortMultipartUpload"
<< ": uploadId# " << uploadId
Expand Down
Loading