fix(checker): stop flagging concurrent committed overwrites as corruption#28
Conversation
|
E2E rerun evidence (minikube, same ec-shard hotspot suite that false-reded):
The stale-read guard stays intact — the settled-overwrite unit test pins that an old value returned long after both writes complete is still flagged. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53b96cc825
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let history = committed_history | ||
| .get(key) | ||
| .map(Vec::as_slice) | ||
| .unwrap_or_default(); | ||
| if !concurrent_committed_read(history, record.started_at_ms, &actual) { |
There was a problem hiding this comment.
When a GET overlaps a committed overwrite but the GET record is appended before that PUT's Ok record, the matching value is not in committed_history yet because this check only looks at writes already processed in the single pass. For example, if PUT B starts while PUT A is live, the GET overlaps B, returns B's body, and B completes successfully just after the GET, this still reports expected A, got B even though the PUT/GET overlap is a legal concurrent read under the same ordering rule this patch is trying to model. The exemption needs to consider committed writes later in the history whose window overlaps the GET, not just the prefix seen so far.
Useful? React with 👍 / 👎.
…tion successful_read_anomalies kept one latest committed value per key and compared every successful GET against it — a fully serialized model. Under a hotspot workload two committed overwrites of one key can have overlapping completion windows, and a GET issued before the later write finished may legally return the earlier value; S3 gives concurrent overwrites no client-observable order. The checker reported that as successful_corrupted_reads — reproduced verbatim on a real minikube cluster the first time a large-payload (EC shard path) hotspot profile ran: two PUTs at t+11.7..11.9s and t+11.8..12.0s, a GET starting at t+11.9s returning the first value, flagged as data_corruption while the object's final state was fully consistent. This is backlog#1100 review finding C-exec-P1 (hotspot completion-order serialization) caught live. Track every committed write's value and completion time per key, and exempt a mismatching GET only when it is a legal concurrent read: - the matching write's window overlaps the GET itself, or - the latest write was still in flight when the GET started and the GET returned the immediately-previous committed value. Both legs keep real stale-read detection intact: once all writes settle before the GET starts, an old value is still corruption. Committed deletes clear the key's history, so post-delete resurrection detection is unchanged. cargo test: 239+15 pass (new: replay of the observed race must pass; settled-overwrite stale read must still flag; GET racing the latest in-flight write may read the previous value).
53b96cc to
5d693e1
Compare
|
Rebased onto latest main. Complementary to the merged #32, not redundant: #32's |
Summary
successful_read_anomalieskept one latest committed value per key and compared every successful GET against it — a fully serialized model. Under a hotspot workload, two committed overwrites of one key can have overlapping completion windows, and a GET issued before the later write finished may legally return the earlier value (S3 gives concurrent overwrites no client-observable order). The checker reported that assuccessful_corrupted_reads/data_corruption.This is review finding C-exec-P1 from rustfs/backlog#1100 ('hotspot workload treats completion order as serialization truth → false DataCorruption'), and it reproduced verbatim on a real minikube cluster the first time a large-payload (EC shard path) hotspot profile ran:
Same byte length, both values committed, GET started before the second PUT finished — a legal concurrent read, not corruption. Left unfixed, every hotspot profile (including #26's
versioned-hot-mutations) is a false-red generator.Fix
Track every committed write's value + completion time per key; exempt a mismatching GET only when it is a legal concurrent read:
Detection power is preserved deliberately:
Validation
cargo test: 239+15 pass — new tests replay the observed race (must pass), a settled-overwrite stale read (must still flag), and a GET racing the latest in-flight write returning the previous value (legal).cargo clippy --all-targets/cargo fmt --checkclean.Independent of #26/#27 (touches only
checker.rs).