Pin the crate under test, add CI, and make the gate's verdict machine-checked - #1
Merged
Conversation
The harness had no way to say which revision of client-rust it tested. The
dependency is a path dep (`tikv-client = { path = "../client-rust" }`), and
Cargo records no source or rev for a path dep, so Cargo.lock pins nothing. The
sibling checkout is currently a *clean* tree three commits ahead of upstream —
which looks entirely trustworthy and is not the baseline at all. Results
produced there are indistinguishable from pinned ones and mean something else.
Cargo cannot fix this, and rewriting the dependency to a git dep would break the
point of the repo (pointing the harness at a local branch to prove a fix). So
pin by recording and asserting instead:
- pins.toml is the single source of truth for both clients, the cluster images
and the toolchains. Every value is verified, not guessed: the client-go
pseudo-version comes from `go list -m`, the image digests from
`docker buildx imagetools inspect`.
- scripts/provenance.sh stamps what was ACTUALLY exercised. PARITY_STRICT=1 (CI)
refuses to run at all when that is not the pin; local runs warn and proceed, so
iterating on a fix still works.
- scripts/check-pins.sh asserts every consumer agrees with pins.toml, and that
the client-rust pin is an ancestor of upstream/master. A pin that is not
upstream cannot be a baseline: the "gap" might exist only on a fork branch and
the fix would have nowhere to land.
- Images are digest-pinned. `pingcap/pd:v8.5.5` is a mutable tag, so a verdict
signed against it is not reproducible later.
Plain pass/fail is a meaningless signal here. The harness carries no workarounds
for client bugs — a deficiency is a finding expressed as a failing test — so at
the pinned revision the correct outcome is "green except d6, which must be red".
scripts/gate-verdict.sh encodes that, and fails loudly if an expected-red test
ever PASSES: the gap closed upstream and the pin, the README verdict and the
findings are stale. Without that rule the gate would rot into "d6 red forever"
and nobody would notice the day it was fixed.
Two Makefile bugs, both real:
- `gate` omitted failpoint-test, so the documented one-shot never ran d7 — the
only test that empirically reproduces finding 2. Prerequisites run
sequentially, so the process-global `fail` registry was never actually at
risk; there was no reason to leave it out.
- `failpoint-test` was missing from .PHONY.
CI clones tikv/client-rust at the pin beside the harness, so the path dep
resolves deterministically without touching anyone's checkout. It needs no
protoc: client-rust has no build.rs and its proto-build member is not in our
dependency graph. The gate job must stay off `container:`/`services:` —
cluster/docker-compose.yml uses network_mode: host and needs the runner's netns.
Verified by reproducing CI's layout at the pin: provenance accepts the run,
16 tests green, d6 red.
…the bug d7 proves finding 2 (tikv/client-rust#545): after a 2PC commit fails at prewrite, `rollback()` clears the prewrite locks in optimistic mode (BatchRollback) but not in pessimistic mode — PessimisticRollback removes only LockType::Pessimistic locks and silently skips the prewritten 2PC lock, while still returning Ok. It asserted `pess_locks == 1`: that the bug is PRESENT. That inverts the signal. The test passes while client-rust is broken and fails the moment it is fixed — so a green suite means a broken client, and the fix branch for #545 (upstream PR 547) reports its own success as a test failure. It also contradicts the harness's governing principle, which d6 already follows: where the client is deficient, that is a finding expressed as a test asserting the CORRECT behavior, which turns green when the fix lands. Assert `pess_locks == 0` instead — a rollback that reports Ok while leaving the locks it placed is a rollback that did not roll back. d7 becomes the regression test for the fix rather than a preservation of the bug, and is now symmetric with d6: red at the pinned baseline, green once PR 547 merges. The optimistic arm stays as a reachability proof — the same sequence leaves 0 locks there, so the pessimistic failure is not inherent to the flow. The expectation moves into gate-verdict.sh's XFAIL table, which owns it and shouts (XPASS) if it ever passes unexpectedly. Verified both ways: at the pinned e53837d d7 is red (bug live) and the verdict passes with d6+d7 declared XFAIL; against the PR 547 branch d7 is green and the XPASS rule correctly fails the run with "the gap closed, re-pin".
…en it dies
The first CI run brought PD up and then sat for 120s waiting for a TiKV store
that was never coming:
[FATAL] the maximum number of open file descriptors is too small,
got 65536, expect greater or equal to 123880
TiKV refuses to start below ~123880 descriptors. Whether it gets them was left
to the Docker daemon's default ulimit, which differs between machines: a
workstation daemon grants enough, a GitHub runner caps containers at 65536. So
the cluster came up locally and died instantly in CI — the compose file was
portable by luck. Set nofile explicitly so every environment is identical.
wait-ready.sh made this needlessly hard to diagnose. A TiKV that dies at startup
never registers, so polling PD could only ever report the symptom ("no store
reached Up within 120s") while the cause sat in a container log nobody printed.
It now notices a container that has exited and prints what it actually said,
failing in ~2s instead of 120.
Note `docker compose ps` lists only RUNNING containers, so a dead one reports an
empty state — the precise case being detected. `ps -a` is what makes the check
work; without it the fast path silently never fires (confirmed by forcing the
65536 limit locally, which reproduced CI exactly).
Contributor
Author
|
@codex review |
Two validation paths passed without enforcing their invariant. Both are the false-green class this harness exists to prevent, and both were found by review. 1. The ancestry check never ran in CI. check-pins.sh looked for a remote literally named `upstream`. That exists in a developer's sibling checkout (origin=fork, upstream=tikv) but NOT in CI, where actions/checkout clones tikv/client-rust as ORIGIN. So CI took the "cannot verify" branch, printed a note, and reported "pins OK" — the load-bearing check that the pin is an upstream commit, and therefore that a gap stated against it is fileable at all, had never once executed where it mattered. The CI log says it plainly: "no upstream/master ref in ../client-rust — cannot verify ancestry" / "pins OK". Resolve the upstream remote by URL rather than by name, and under PARITY_STRICT treat "cannot verify" as a failure rather than a shrug: a run that cannot check an invariant is not evidence for it. CI checks out client-rust with fetch-depth: 0 so the default branch ref exists (a shallow clone has no branch refs, which the new code correctly rejects). 2. An expected-red test was accepted no matter WHY it failed. gate-verdict.sh treated any non-zero exit as XFAIL == "the gap is still open". But a test can fail on the way to its assertion: d6 panics if it cannot manufacture the orphan (its region split can be undone by pd.toml's merge scheduler on a cluster with accumulated regions), and d7 has an optimistic control assertion that must hold before the real one is reached. Since the expected-pass run skips these tests entirely, nothing else covered them — so a broken harness could masquerade as a confirmed finding. Each XFAIL row now carries the signature of the failure it predicts, and the failure output must match it. A red test that failed for the wrong reason is now WRONG FAILURE, not XFAIL. This immediately caught a real instance: on a cluster with ~105 accumulated regions, d6 never separates its keys and never reaches its assertion. On a pristine cluster (CI's regime, and the last green run's log confirms it) it splits and proves the gap. The verdict now reports the difference instead of banking a false XFAIL. That d6's cross-region precondition is manufactured rather than asserted is a real fragility, tracked separately. Verified: ancestry check passes on a full clone (via upstream/ or origin/), fails under strict on a shallow clone, and rejects a fork-only rev as "NOT an ancestor"; the signature check accepts d6's genuine failure, and rejects a wrong-reason failure (pointing the suite at a dead cluster).
4 tasks
…ean checkout Two defects found by cross-vendor review of this branch. 1. CI concurrency could cancel an unrelated PR's run. The group was keyed on `github.head_ref`, which is only the SOURCE BRANCH NAME. Two pull requests from different forks that both use a common branch name — `patch-1`, say — share a group, so opening the second cancels the first's in-flight CI. That surfaces as a mysterious failure on a PR nobody touched. Key on the pull request number instead, falling back to the ref for push runs. 2. `make cluster-down` failed from a clean checkout. Every compose invocation passes `--env-file cluster/images.env`, and that file is generated and gitignored. On a fresh clone — or immediately after `make clean` removed it — compose exits with "couldn't find env file" before it can do anything, so `cluster-down` and `cluster-logs` were unusable in exactly the state where you most want them: a cluster left running with no obvious way to stop it. Both targets now depend on the generated file, which is cheap and idempotent to rebuild. Verified: with cluster/images.env deleted, cluster-down previously exited 1 with "couldn't find env file"; it now regenerates the file, tears the cluster down, and exits 0. cluster-logs likewise.
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.)
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.
Absorbs #2 (fast-forwarded, commits preserved verbatim). 11 commits.
Why
The harness had no way to say which revision of client-rust it tested, and its cross-region obligations were assumed rather than asserted. Both are false-green vectors: a gate that can pass vacuously, against an unknown revision, is not evidence.
1. Pinning — you can now say what was tested
tikv-clientis a path dependency, and Cargo records no source or rev for one, soCargo.lockpins nothing. The sibling checkout is a clean tree three commits ahead of upstream — which looks trustworthy and is not the baseline.pins.toml— one source of truth for both clients, cluster images, toolchains. Every value verified, not guessed (client-go pseudo-version fromgo list -m; image digests fromdocker buildx imagetools).scripts/provenance.sh— stamps what was actually exercised.PARITY_STRICT=1(CI) refuses to run off-pin; local runs warn and proceed, so iterating on a fix still works.scripts/check-pins.sh— asserts the pin is an ancestor ofupstream/master. A pin that isn't upstream can't be a baseline: the gap might exist only on a fork branch and the fix would have nowhere to land.pingcap/pd:v8.5.5is a mutable tag.2. The verdict — green/red is meaningless here
The harness carries no workarounds for client bugs: a deficiency is a failing test. So the correct outcome at the pin is green except
d6andd7, each asserting correct behaviour of a gap still open upstream (#519 → PR 544, #545 → PR 547).scripts/gate-verdict.shencodes that, and fails loudly on XPASS — an expected-red test passing means the gap closed upstream and the pin/README/ledger are stale. Without it the gate rots into "red forever" and nobody notices the day it's fixed. Each XFAIL also carries the signature of the failure it predicts: a test that fails for the wrong reason isWRONG FAILURE, not silent proof.d7was inverted: it asserted the bug was present, so it went red the moment client-rust was fixed. It now asserts the correct behaviour and is the regression test for PR 547.3. The multi-region precondition — asserted, not hoped for
cluster/tikv.tomlsetsregion-max-keys = 10so batches span Raft regions. Nothing checked it happened.d6needs its two keys in different regions, and got there by writing filler and hoping — a race that is lost on a busy cluster (1 round pristine, 54 after the suite, >77/timeout on CI), whose failure was indistinguishable from the client failing to resolve the orphan.PD can be told where to cut:
POST /pd/api/v1/operatorswithpolicy: "usekey". The precondition now takes 1 split request where it previously needed 77+.tests/common/cluster.rsadds PD ground truth (region_pair— one snapshot, never two), andp0proves TiKV's own split checker still works, on a per-run range so a stale boundary can't satisfy it.4. Bugs fixed along the way
make gatenever rand7— the only empirical proof of finding 2.failpoint-testalso missing from.PHONY.nofileto the daemon default. Portable by luck: fine locally, instant death on a GitHub runner.wait-ready.shalso now fails in ~2s with TiKV's own error instead of blind-waiting 120s.github.head_ref— two fork PRs sharing a branch name would cancel each other's runs.make cluster-downfailed from a clean checkout (couldn't find env file), leaving a running cluster with no way to stop it.Evidence
CI reproduces the expected verdict on a real cluster at the pinned revision:
Cross-vendor reviewed (Codex) to convergence — 8 findings fixed across both halves, 1 dismissed with evidence (
clippy::format_collectis a pedantic lint, off by default;clippy -D warningsexits 0).Checklist
make checkgreen (now includespins-check)🤖 Generated with Claude Code