tests: assert the multi-region precondition instead of assuming it - #2
Merged
Conversation
The gate's headline obligation — a multi-key commit(WriteBatch) is atomic — is only interesting when a batch genuinely spans Raft regions. cluster/tikv.toml sets region-max-keys = 10 so that it does, but nothing ever checked that it happened. If the config were not in force, or a split were undone, the cross-region tests would run inside a single region and still pass. An assumption no test can fail is not an assumption, it is a hole. d6 felt this most sharply. It needs its two keys in different regions (a prewrite is per-region and fails atomically within one, so the orphan cannot otherwise exist), and it got there by writing filler, retrying 8 times, and panicking "region split never separated the keys" if no orphan appeared. On a cluster with many small regions, pd.toml's merge scheduler (max-merge-region-size = 1) can undo the split faster than it lands, so that panic fires — and to anything reading an exit code it is indistinguishable from the client failing to resolve the orphan. The test then reads as proof of the #519 gap while having proved nothing. (The XFAIL signature check added in the previous commit is what surfaced this: d6 was failing for the wrong reason on a warm cluster and would have been banked as a finding.) Add tests/common/cluster.rs: PD's HTTP API as ground truth (region_count, region_of, stores_up) plus ensure_cross_region(), which writes filler and POLLS PD until a boundary actually separates the two keys, then panics naming the precondition if it cannot. d6 uses it before going near its assertion, so a missing split is now a precondition failure attributable to the cluster, not a silent false finding. Region bounds are reported by PD in TiKV's memcomparable encoding (8-byte groups, each followed by a 0xFF - pad marker), NOT as raw keys — verified against the live v8.5.5 cluster and pinned by unit tests, including an order-preservation test. Comparing raw keys against those bounds would silently return the wrong region, which is the sort of bug that makes a precondition check worse than none. Also add p0_cluster_can_split_regions: write 200 keys, assert against PD that they split. It fails the gate loudly if the cluster cannot split at all, rather than letting every cross-region test pass vacuously. Verified on the exact cluster that broke d6 before (100+ accumulated regions): p0 passes (9 -> 37 regions), and d6 now reports "cross-region precondition met after 4 round(s): 76 != 20" and reaches its real assertion — a genuine XFAIL.
Two defects in the new precondition helper, both found by review. 1. The cross-region check could compare regions that never coexisted. are_cross_region called region_of(a) then region_of(b), and each issued its own /pd/api/v1/regions fetch. The cluster is actively splitting and pd.toml's merge scheduler is actively undoing splits, so the layout can change between the two reads: two ids drawn from different snapshots can differ without the keys ever having been in different regions at the same moment. That reports the precondition as MET when it never held — and it is exactly the merge race this module was added to defend against, so the bug lived in the one place it could do most harm. Add region_pair(), which fetches once and locates both keys in that single view; are_cross_region and p0 both go through it, and ensure_cross_region reports the ids from the snapshot that decided it rather than re-reading PD for the log line. 2. Only the first PD endpoint was ever used. $PD_ADDRS is comma-separated and the client under test is handed all of it, so it can connect happily through the second entry while the first is down. pd_get built its URL from pd[0] and panicked, failing the precondition on a cluster that is, by the client's own standard, reachable. It now tries each endpoint and only fails if none answer, reporting every error. Also add cluster-free unit tests for locate(): start is inclusive and end is exclusive, empty bounds mean UNBOUNDED rather than "the empty key" (reading them as a literal bound would put every key in the first region and silently report every pair as same-region), and keys either side of a boundary are separated. Verified: unit tests green; PD_ADDRS=127.0.0.1:9999,127.0.0.1:2379 (first endpoint dead) now passes p0 where it previously panicked; d6 still meets the precondition from a single snapshot and still XFAILs on its own assertion.
…y PD request Two more defects, both found by review, both of the same shape: a guard that holds at the moment it is checked but not at the moment it is used. 1. d6 established the boundary once, before the orphan loop. The boundary is not stable. pd.toml's merge scheduler (max-merge-region-size = 1) actively coalesces the very small regions ensure_cross_region manufactures, so a boundary confirmed before the loop can be gone by the time the orphaner prewrites. The prewrite is then single-region, no orphan appears, and the test panics as a harness failure — reintroducing one level up the very "failed for a reason that is not the finding" problem the precondition was added to eliminate. Re-establish it on every attempt. It costs one PD read when already satisfied. 2. A blackholed PD endpoint made the fallback unreachable. pd_get tries each entry of $PD_ADDRS, but reqwest::get carries no timeout: an endpoint that accepts the TCP connection and then never answers hangs the await forever, so the healthy endpoints later in the list are never reached. The fallback existed but could not be got to, and the enclosing test deadlines cannot help — they are not running, they are blocked inside pd_get. Use a Client with a 3s request and connect timeout. Verified: with a socket that accepts and never replies as the first endpoint, p0 now completes in 13s via the second endpoint (previously: hangs indefinitely); d6 re-confirms the boundary each round and still XFAILs on its own assertion.
… not just a transport error pd_get moved to the next endpoint only when send() failed. A PD that is up but unhealthy — mid-restart, not yet the leader — answers with a non-2xx status or an HTML error page, and that was read, failed to parse as JSON, and panicked. The fallback therefore covered only one of the several ways an endpoint can be useless, and the precondition could still fail on a cluster the client under test reaches happily via a later address. An endpoint now counts as usable only if it answers 2xx with parseable JSON; every other outcome records the reason and tries the next address, and the panic (when none work) lists what each one said. Verified with a first endpoint serving HTTP 500 + HTML: p0 now passes via the second endpoint, where it previously panicked on the unparseable body.
…it checker
CI failed the precondition: 77 rounds of filler, 45s, and still no boundary
between d6's two keys — while the cluster was plainly splitting (103 -> 188
regions). The gate-verdict signature check caught it as WRONG FAILURE rather than
banking a false XFAIL, which is the machinery working, but the underlying approach
was unsound.
Coaxing a split with filler keys is a race, and on a busy cluster it is a race you
lose. TiKV chooses a split point for the WHOLE region, so when that region holds a
lot of other data the cut lands somewhere else, and many splits must happen before
one falls between two adjacent keys. Measured: 1 round on a pristine cluster, 54
rounds after the rest of the suite has run, >77 (timeout) on CI. That is why d6
passed in isolation and failed in the suite. Raising the timeout would only have
bought a slower race.
PD can be told where to cut. `POST /pd/api/v1/operators` with
`{"name":"split-region","policy":"usekey","keys":[<memcomparable hex>]}` splits the
region at exactly the key we name, immediately — and it works even where no data
exists yet, so the filler is unnecessary. ensure_cross_region now takes an explicit
`split_at` (asserted to sort strictly between the two keys) and issues that split.
It stays a loop: pd.toml's aggressive merge scheduler (max-merge-region-size = 1)
will glue the tiny regions back together, so the split is re-issued if the boundary
has been merged away, and d6 re-establishes the precondition per attempt.
p0 deliberately keeps waiting on TiKV's OWN split checker. Its job is to prove that
cluster/tikv.toml is in force (region-max-keys = 10) — an explicit split would
succeed even with the config missing, and every other test's cross-region claim
rests on natural splitting actually happening.
Verified in the exact regime that broke CI (fresh cluster, full suite, then d6, 186
regions): the precondition is met after 1 split request instead of 54-77+ rounds,
and d6 reaches its assertion and XFAILs on it.
p0 exists to prove that cluster/tikv.toml is in force — that TiKV can still split regions — because every other cross-region claim in the gate rests on natural splitting actually happening. It used a fixed prefix, and `wipe` deletes keys but NOT region boundaries. So on any cluster that had run p0 before, the boundary carved by the earlier run was still there, and the check was satisfied the instant it looked — passing even if the config were missing and TiKV could no longer split anything at all. A test that cannot fail proves nothing, which is the precise failure p0 was added to prevent. Use a per-run prefix. A fresh range has no boundary to inherit, so the split it observes must have been made by TiKV, now. Verified by running it twice against the same busy cluster: each run forces a new split (188 -> 217 -> 246 regions) and the second takes ~15s waiting for TiKV to cut its own range, rather than returning immediately on the first run's boundary. (Also reviewed: the claim that clippy::format_collect breaks `make check`. It does not — it is a pedantic lint, off by default; `cargo clippy -D warnings` exits 0 and CI's check job passes. It only fires when explicitly enabled.)
eduralph
force-pushed
the
fix/d6-cross-region-precondition
branch
from
July 12, 2026 21:42
582a415 to
37d6c8c
Compare
4 tasks
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
1 participant
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.
Stacked on #1 (base:
pin-and-ci). Retarget tomainonce #1 merges.Why
The gate's headline obligation — a multi-key
commit(WriteBatch)is atomic — is only interesting when a batch genuinely spans Raft regions.cluster/tikv.tomlsetsregion-max-keys = 10so that it does, but nothing ever checked that it happened. If the config weren't in force, or a split were undone, the cross-region tests would run inside a single region and still pass. An assumption no test can fail is not an assumption, it's a hole.d6felt this most sharply. It needs its two keys in different regions (a prewrite is per-region and fails atomically within one, so the orphan cannot otherwise exist). It got there by writing filler, retrying 8 times, and panicking "region split never separated the keys" if no orphan appeared. On a cluster with many small regions,pd.toml's merge scheduler (max-merge-region-size = 1) can undo the split faster than it lands — so that panic fires, and to anything reading an exit code it is indistinguishable from the client failing to resolve the orphan. The test then reads as proof of the #519 gap while having proved nothing.The XFAIL-signature check from #1 is what surfaced this: on a warm cluster
d6was failing for the wrong reason and would previously have been banked as a confirmed finding.What
tests/common/cluster.rs— PD's HTTP API as ground truth:region_count,region_of,stores_up, andensure_cross_region(), which writes filler and polls PD until a boundary actually separates the two keys, then panics naming the precondition if it can't.d6calls it before going anywhere near its assertion, so a missing split is a precondition failure attributable to the cluster — not a silent false finding.p0_cluster_can_split_regions— writes 200 keys and asserts against PD that they split. Fails the gate loudly if the cluster can't split at all.Key encoding (the subtle bit)
PD reports region bounds in TiKV's memcomparable encoding — 8-byte groups, each followed by a
0xFF - padmarker — not as raw keys. Verified against the live v8.5.5 cluster:Comparing raw keys against those bounds would silently return the wrong region — a precondition check that lies is worse than none — so the encoding is pinned by unit tests, including order-preservation.
Evidence
Verified on the exact cluster that broke
d6before (100+ accumulated regions):p0passes:cluster splits regions: 9 -> 37 regions, stores Up [1]d6:cross-region precondition met after 4 round(s): Some(76) != Some(20)→ reaches its real assertion → genuine XFAIL (failed on its own assertion)Checklist
make checkgreend6's finding sound, it does not paper over #519d6still red for the right reason at the pinned revision🤖 Generated with Claude Code