Skip to content

fix: Fix data correctness issues in full replication (rsync) #3267

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
Mixficsol wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

fix: Fix data correctness issues in full replication (rsync) #3267

Show file tree
Hide file tree
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
8 changes: 6 additions & 2 deletions include/rsync_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,19 @@ class RsyncWriter {
public:
RsyncWriter(const std::string& filepath) {
filepath_ = filepath;
fd_ = open(filepath.c_str(), O_RDWR | O_APPEND | O_CREAT, 0644);
// NOTE: do NOT use O_APPEND here. rsync writes each chunk to an explicit
// offset via pwrite(). O_APPEND would force every write to the end of the
// file, ignoring the offset, which corrupts the file on retry / resumed
// transfer / stale-file scenarios (holes, duplicated or misplaced data).
fd_ = open(filepath.c_str(), O_RDWR | O_CREAT, 0644);

@coderabbitai coderabbitai Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Check open() result in the constructor.

fd_ isn't validated after open(). On failure fd_ stays -1 and the failure surfaces later as a confusing pwrite error against an invalid descriptor. Log/record the failure at the source so Write() fails with a clear cause.

🛡️ Proposed guard 
     fd_ = open(filepath.c_str(), O_RDWR | O_CREAT, 0644);
+    if (fd_ < 0) {
+      LOG(WARNING) << "open file [" << filepath << "] failed! error: " << strerror(errno);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fd_ = open(filepath.c_str(), O_RDWR | O_CREAT, 0644);
fd_ = open(filepath.c_str(), O_RDWR | O_CREAT, 0644);
if (fd_ < 0) {
LOG(WARNING) << "open file [" << filepath << "] failed! error: " << strerror(errno);
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@include/rsync_client.h` at line 119, The rsync client constructor currently
assigns the result of open() to fd_ without checking for failure, so handle the
error immediately in the constructor path. In the rsync_client class, validate
fd_ right after open(filepath.c_str(), O_RDWR | O_CREAT, 0644), record or log
the failure with the filepath and system error, and leave the object in a
clearly invalid state so Write() can surface the original cause instead of a
later pwrite() error. Ensure the fix is localized around the constructor and the
fd_ / Write() flow.

}
~RsyncWriter() {}
Status Write(uint64_t offset, size_t n, const char* data) {
const char* ptr = data;
size_t left = n;
Status s;
while (left != 0) {
ssize_t done = write(fd_, ptr, left);
ssize_t done = pwrite(fd_, ptr, left, offset);
Comment on lines 113 to +127
if (done < 0) {
if (errno == EINTR) {
continue;
Expand Down
4 changes: 4 additions & 0 deletions include/rsync_server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ class RsyncReader {
memcpy(data, block_data_ + offset_in_block, copy_count);
*bytes_read = copy_count;
*is_eof = (offset + copy_count == total_size_);
// Compute a per-chunk checksum so the client can detect corruption of this
// response (e.g. a bit flip that slips past the TCP checksum). Previously
// this was left as an empty string, so the client had nothing to verify.
*checksum = pstd::md5(std::string(data, copy_count));
Comment on lines +118 to +121
return pstd::Status::OK();
}

Expand Down
17 changes: 16 additions & 1 deletion src/rsync_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,22 @@ Status RsyncClient::CopyRemoteFile(const std::string& filename, int index) {
return s;
}

s = writer->Write((uint64_t)offset, ret_count, resp->file_resp().data().c_str());
// Verify the per-chunk checksum before writing to disk. Older masters may
// return an empty checksum; in that case we keep backward compatibility by
// skipping verification. A mismatch means this response was corrupted in
// transit, so retry the same offset instead of persisting bad data.
const std::string& expected_checksum = resp->file_resp().checksum();
if (!expected_checksum.empty()) {
std::string actual_checksum = pstd::md5(std::string(resp->file_resp().data().data(), ret_count));
if (actual_checksum != expected_checksum) {
LOG(WARNING) << "rsync client checksum mismatch, filename: " << filename
<< ", offset: " << offset << ", retry";
retries++;
continue;
}
}

s = writer->Write((uint64_t)offset, ret_count, resp->file_resp().data().data());
Comment on lines +242 to +257
if (!s.ok()) {
LOG(WARNING) << "rsync client write file error";
break;
Expand Down
Loading