fix: Fix data correctness issues in full replication (rsync)#3267
Open
Mixficsol wants to merge 1 commit into
Open
fix: Fix data correctness issues in full replication (rsync)#3267Mixficsol wants to merge 1 commit into
Mixficsol wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds per-chunk MD5 checksum generation on the rsync server side, verification with retry-on-mismatch on the client side, and fixes RsyncWriter to use offset-based pwrite instead of append-mode write for resume-safe transfers. ChangesRsync checksum and write fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RsyncClient
participant RsyncReader
participant RsyncWriter
RsyncClient->>RsyncReader: request chunk at offset
RsyncReader-->>RsyncClient: chunk data + md5 checksum
RsyncClient->>RsyncClient: compute md5 of received chunk
alt checksum mismatch
RsyncClient->>RsyncReader: retry request at same offset
else checksum matches or empty
RsyncClient->>RsyncWriter: Write(chunk, offset)
RsyncWriter->>RsyncWriter: pwrite(fd_, chunk, offset)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@include/rsync_client.h`:
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bb828a8-246c-4242-92b4-e70fbfbdf100
📒 Files selected for processing (3)
include/rsync_client.hinclude/rsync_server.hsrc/rsync_client.cc
| // 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); |
There was a problem hiding this comment.
🩺 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.
There was a problem hiding this comment.
Pull request overview
This PR targets data correctness in the full replication (rsync) path by ensuring chunks are written to the correct file offsets and by adding per-chunk integrity verification between rsync server and client.
Changes:
- Client-side: verify chunk checksum (when provided) before persisting data; retry the same offset on mismatch.
- Server-side: compute and return per-chunk MD5 checksum in file responses.
- Writer: stop using
O_APPENDand write with explicit offsets (pwrite) to avoid misaligned data on retry/resume scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/rsync_client.cc | Adds checksum verification before writing received chunk data and retries on mismatch. |
| include/rsync_server.h | Computes per-chunk checksum in RsyncReader::Read so clients can verify data integrity. |
| include/rsync_client.h | Switches file writing from append-based write() to offset-based pwrite() to ensure correct placement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+242
to
+257
| // 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
+118
to
+121
| // 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
113
to
+127
| 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); | ||
| } | ||
| ~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); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
本 PR 修复了全量同步链路上两个可能导致从库数据损坏且不被发现的问题。
1. 写文件忽略偏移,导致数据错位
RsyncWriter此前以O_APPEND方式打开文件并使用write(),传入的offset参数从未真正参与定位,每次写入都被强制追加到文件末尾。在请求重试、断点续传或旧文件残留的场景下,数据会写到错误位置,造成文件空洞、内容重复或错位。修复:移除
O_APPEND,将write()改为pwrite()按显式偏移写入,保证每个 chunk 落到正确位置。2. rsync 缺少 chunk 级校验,损坏数据静默写入
传输链路两端都没有做 chunk 校验:server 端虽然在响应里预留了
checksum字段,但从未计算(只填了空字符串);client 端也完全没有校验,拿到数据直接写盘。这意味着即使某个 chunk 在传输中被损坏(TCP 校验存在漏检概率),从库也会照单全收。修复:
checksum字段;checksum比对,不一致则重试当前偏移,避免损坏数据落盘。兼容性
client 端仅在
checksum非空时才做校验,旧版本 master 返回空校验值时自动跳过,新 client 连接旧 master 不受影响。涉及文件
include/rsync_client.h—RsyncWriter改用pwrite按偏移写入include/rsync_server.h—RsyncReader::Read计算 chunk MD5src/rsync_client.cc— 写盘前校验 chunk checksumSummary by CodeRabbit