feat(workload): ranged GETs with regenerated-slice verification#30
Conversation
|
E2E evidence (minikube, rustfs:1.0.0-beta.8, stress-cpu,
Known follow-up (deliberate v1 scope): the percent knob is env-only; a |
![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: 1ea782d94f
ℹ️ 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".
| committed_history | ||
| .get(key) | ||
| .map(Vec::as_slice) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
When a ranged GET follows a timed-out/unknown overwrite that actually took effect, this new branch verifies only committed_history and then continues before the existing ambiguous-write handling below can run. The whole-object path can classify matching ambiguous bodies, but for range records the recorded hash is only a slice; if that slice belongs to the ambiguous overwrite rather than the last committed value, verify_ranged_get reports successful_corrupted_reads and fails the checker even though the observed value is a legal materialized ambiguous write. Please include active/superseded ambiguous attempts as reproducible candidates for ranged GETs, or make those cases inconclusive.
Useful? React with 👍 / 👎.
| recorder.finish( | ||
| record, | ||
| OperationOutcome::Failed, | ||
| Some(206), | ||
| Some(error.clone()), |
There was a problem hiding this comment.
When the service returns a response body whose length does not match the requested range, this records the GET as Failed; however successful_read_anomalies only inspects OperationKind::Get records with outcome Ok, so the checker never emits the length-mismatch corruption that verify_ranged_get was written to detect. In fault runs where client disruptions are allowed or expected, a successful HTTP response with truncated/expanded range data is only counted as workload disruption and can still let the checker pass. Please either keep these records checkable by the ranged verifier or have the checker explicitly consume failed range records with bodies.
Useful? React with 👍 / 👎.
| if latest.ended_at_ms >= record.started_at_ms | ||
| && let Some(previous) = prior.last() | ||
| { | ||
| candidates.push(previous); |
There was a problem hiding this comment.
When a ranged GET starts while two or more overwrites are still in flight, prior.last() can be the earlier in-flight write rather than the stable value that existed before the overlap. For example, if w1 settled and w2/w3 both finish after this GET starts, a read that legally returns w1’s slice is not in candidates and is reported as corruption. Please walk back to the last write that settled before the overlapping write window instead of only adding the immediate predecessor.
Useful? React with 👍 / 👎.
|
Pushed a fix from a pre-merge adversarial self-review: |
eb8909a to
c81b806
Compare
|
Rebased onto latest main. Stacked on #28 — merge #28 first. Note #28 is complementary to the already-merged #32 ( |
527f359 to
5b4141c
Compare
The workload issued only whole-object GETs, so the ranged read path —
offset decoding, per-shard positioning, partial-content framing — was
never exercised under fault (backlog#1100 workload-realism gap, second
half of finding D4-5).
Workload side:
- RUSTFS_FAULT_TEST_WORKLOAD_RANGED_GET_PERCENT (0-100, default 0 =
behavior unchanged) turns a deterministic, seed-stable slice of mixed-
workload GETs into ranged reads with in-bounds (offset, length) derived
purely from (seed, index) — reruns replay identical operations.
- get_object_range_result records the returned slice hash with a new
range field, treats a wrong byte count as Failed at the client, and
expects 206 framing.
- Committed writes now record payload_ref { seed, index }: every
non-multipart body comes from the seeded generator, so the checker can
regenerate the exact bytes later.
Checker side (the part that makes this a detector, not just traffic):
- Ranged records are intercepted before the whole-object comparison
(which would otherwise always misfire on slice hashes).
- verify_ranged_get regenerates the expected slice for every committed
value the GET could legally observe — latest write, writes overlapping
the GET, and the previous value while the latest was in flight (same
concurrency legs as the whole-object path) — and flags corruption when
the returned slice matches none of them and byte count when it lies.
- Candidates without payload_ref (multipart bodies, legacy records) make
a mismatch inconclusive rather than a false positive.
cargo test: 245+15 pass (new: clean latest-slice read, concurrent-
variant slice read, garbage slice -> corruption with the range in the
message, unreproducible-candidate inconclusive, length mismatch,
sampler determinism/bounds/rate).
Adversarial self-review before merge (backlog#1100 policy) caught a false-positive: verify_ranged_get only compared the returned slice against committed_history, never against ambiguous (timeout/unknown) writes -- unlike the whole-object read path. So a ranged GET that legally returned the slice of a materialized-but-unconfirmed overwrite (the routine ambiguous-write case under fault injection) matched no committed value and was reported as corrupted_reads -- the harness's harshest verdict -- spuriously failing any fault run with ranged GETs enabled. The byte-identical whole-object GET is classified unknown_writes_materialized (non-corruption). Record payload_ref on ambiguous write attempts (they already come from the seeded generator) and, in verify_ranged_get, mirror the whole-object path: a slice matching a materialized ambiguous write -> unknown_writes_materialized; an unreproducible/range-spanning ambiguous write -> inconclusive; a pending ambiguous write with no clean match -> unknown_write_value_conflicts (not visible_deleted, closing the second divergence the review flagged). Committed-value verification and real corruption detection are unchanged. cargo test 247+15 pass (new: materialized ambiguous ranged read is not corruption; only-ambiguous-write ranged read is inconclusive).
…s exemptions in ranged verification verify_ranged_get reimplemented only the concurrency exemptions of the whole-object path and produced two classes of false positives: - A ranged GET that started before an overlapping committed delete linearized and legally returned the pre-delete value was flagged as visible-deleted/resurrection, because the delete wipes the key's committed history before the GET record is processed. Carry payload_ref (and size) into the stable-at-read-start model and add the stably-live-at-start value as a candidate leg, mirroring the whole-object stable_live_at_start exemption. A GET that starts after the delete settled is still flagged. - A slice matching a materialized ambiguous (timeout) write that a later committed overwrite already superseded landed in corrupted_reads, because only pending ambiguous attempts were consulted. Classify it as unknown_writes_materialized plus unknown_write_value_conflicts, exactly like the whole-object path, and treat mismatches with any ambiguous attempt outstanding as unknown_write_value_conflicts rather than corruption. A slice matching nothing with no ambiguous writes remains corruption. Also fix the ranged_get_of_only_ambiguous_write_is_inconclusive test, whose comment claimed the spans-range-inconclusive branch while the record's payload_ref actually exercised the ambiguous-mismatch branch, and add dedicated coverage for the unreproducible spans-range branch.
5b4141c to
6dcfa7b
Compare
Summary
The workload issued only whole-object GETs, so the ranged read path — offset decoding, per-shard positioning, partial-content framing — was never exercised under fault. This is the second half of workload-realism finding D4-5 in rustfs/backlog#1100 (the first half, EC-shard payload weights, landed as the suite profile guidance in the #26 review).
Stacked on #28 (needs its per-key committed-write history for the concurrency legs; merge #28 first and this rebases clean).
Workload side
RUSTFS_FAULT_TEST_WORKLOAD_RANGED_GET_PERCENT(0-100, default 0 = behavior unchanged) turns a deterministic, seed-stable slice of mixed-workload GETs into ranged reads. The (offset, length) pair is a pure function of (seed, index) — always in bounds, covering single-byte/interior/suffix reads, and reruns replay identically.get_object_range_resultexpects 206 framing, records the returned slice hash with a newrangefield on the history record, and fails at the client when the byte count doesn't match the request (a silent 200-full-body would otherwise let every range read degenerate into a whole-object read).payload_ref { seed, index }: every non-multipart body comes from the seeded generator (seeded_bytes), so the exact bytes are reproducible after the fact. Both new record fields are optional/backward-compatible.Checker side (what makes this a detector, not just traffic)
verify_ranged_getregenerates the expected slice for every committed value the GET could legally observe — the latest write, writes whose completion window overlaps the GET, and the immediately-previous value while the latest was in flight (the same concurrency legs as fix(checker): stop flagging concurrent committed overwrites as corruption #28's whole-object path) — and flags corruption when the returned slice matches none of them, or when the byte count lies.payload_ref(multipart bodies, legacy artifacts) make a mismatch inconclusive rather than a false positive.Why this matters for coverage
RustFS's ranged reads on sharded objects walk a different path than full streams (shard offset math, partial reconstruct). With the EC-shard payload profile + this knob, faults now run against ranged reads at arbitrary offsets with content actually verified against the committed bytes — not just "the request didn't error".
Validation
cargo test: 245+15 pass — clean latest-slice read; concurrent-variant slice read (racing overwrite); garbage slice → corruption with the range in the message; unreproducible-candidate inconclusive; length mismatch flagged; sampler determinism/bounds/rate.cargo clippy --all-targets/cargo fmt --checkclean.