tests: scope failpoint lock counts to each test's own keys (fixes #525, mitigates #516)

[eduralph]

The txn_cleanup_* failpoint tests count locks with a whole-keyspace scan (count_locks / wait_for_locks_count over ..). txn_cleanup_locks_batch_size stubs cleanup via the before-cleanup-locks failpoint and leaves its locks in place — and since they're async-commit locks, they resolve in the background and drain at an unpredictable rate. A later serial test's unbounded scan then catches the still-draining residue and over-counts — the flaky 575 vs 512 / 513 vs 512 in #525.

Fix (test-only)

• Add count_locks_of / wait_for_locks_count_of that count only the caller's own keys, making the assertions immune to residual locks from other tests.
• Switch all txn_cleanup_* count assertions to the scoped variants.
• Have txn_cleanup_locks_batch_size resolve its own locks before returning, so it no longer seeds residue for the next serial test.
• Remove the now-unused whole-keyspace wrappers.

No client (src/) change.

Fixes #525.

Scope note: this does not address #516 (the txn_cleanup_2pc_locks 600s timeout). An earlier revision of this description suggested it might, by removing residue that amplifies cleanup_locks — that reasoning was wrong: txn_cleanup_2pc_locks runs first in the serial suite (it is consistently reported as ( 1/54 )), so no residue from the other cleanup tests can reach it. #516 is a separate, still-undiagnosed stall and is left untouched here.

Testing

Ran the suite repeatedly against a local multi-region cluster (MULTI_REGION=1, the aggressive-split config/tikv.toml) — stable across 10+ runs at ~5s each; the previously-observed count drift does not recur. make check clean.

Summary by CodeRabbit

• Tests
  • Improved transaction cleanup and lock-resolution reliability by validating lock state only within per-test key ranges/bands (not the full keyspace).
  • Reduced flakiness caused by residual asynchronous lock activity between serial integration tests.
  • Enhanced cleanup and verification coverage across async commit, range cleanup, resolve, pessimistic rollback, and 2PC lock scenarios to ensure locks fully settle before assertions complete.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign coocood for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b33f306e-bbcc-47ab-acb8-982b7a0b7358

📥 Commits

Reviewing files that changed from the base of the PR and between 76c7808 and 3a73fcf.

📒 Files selected for processing (1)

• tests/failpoint_tests.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/failpoint_tests.rs

---

📝 Walkthrough

Walkthrough

Failpoint transaction tests now generate keys in isolated bands and scope lock counting, polling, and cleanup to those bands or individual keys. Cleanup, async-commit, lock-resolution, and rollback assertions use bounded helpers.

Changes

Failpoint lock assertions

Layer / File(s)	Summary
Define isolated key bands tests/failpoint_tests.rs	Adds keyspace-band constants and constrains generated test keys to the selected band.
Implement range-scoped lock helpers tests/failpoint_tests.rs	Replaces whole-keyspace counting and waiting with range-, band-, and single-key lock helpers.
Update transaction cleanup tests tests/failpoint_tests.rs	Scopes cleanup and lock assertions across batch cleanup, async commit, 2PC cleanup, lock resolution, and pessimistic rollback scenarios.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

• tikv/client-rust#547: Updates related pessimistic rollback lock-clearing behavior and verification in the same failpoint test suite.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main test-only change: scoping failpoint lock counts to each test's own keys.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[eduralph]

@pingyu - ping for review

[pingyu]

How about prepend a distinct prefix to generated keys for each case, and scan locks in the range [prefix, prefix + 1).

This solution would be more comprehensive. Test cases run in separated key ranges and there will be less interference with each other.

[eduralph]

Thanks — good call. Each case now writes into its own disjoint band of the key space and scans (and cleans up) locks only within that band, so one case's residual locks can't be seen by another.

One adjustment worth flagging: I started with a literal one-byte prefix as you suggested, but that confines all ~512 keys to [prefix, prefix+1), which against the ~40 pre-split regions usually lands in a single region and loses the multi-region cleanup coverage these tests rely on. A wide band (1/N of the key space) keeps the isolation you're after while still spanning several regions.

Also rebased onto latest master (was conflicting after #550/#553). PTAL.

[coderabbitai]

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 `@tests/failpoint_tests.rs`:
- Around line 178-186: Update the test cleanup after the scoped lock-count
assertion: disable the before-cleanup-locks failpoint, run real cleanup for this
test’s band, and assert the band-scoped lock count reaches zero before calling
scenario.teardown().

🪄 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 Plus

Run ID: eb5449e1-6626-40bb-947e-4eb3f05c97d6

📥 Commits

Reviewing files that changed from the base of the PR and between 5cc7b3f and 76c7808.

📒 Files selected for processing (1)

• tests/failpoint_tests.rs

[eduralph]

@pingyu — some concrete evidence that this PR helps with #516 (the CI timeouts), not only #525.

#544's integration-test-txn just timed out on the current-master failpoint tests: run. #544 only touches src/ error handling, so this is a pre-existing master flake, not a regression — and with #553's phase logging in place the trace now localizes the hang precisely. txn_cleanup_2pc_locks is killed at the 600s cap, and the last phase to open never closes:

SLOW [> 60.000s] … txn_cleanup_2pc_locks
… (every 60s) …
TERMINATING [>600.000s] … txn_cleanup_2pc_locks
    TIMEOUT [ 600.939s] (1/54) txn_cleanup_2pc_locks

phase > 2pc/all-committed: wait for locks to drain      ← opened, never returns

That drain-wait is wait_for_locks_count(&client, 0), which polls a whole-keyspace scan_locks(.., 65536). Under MULTI_REGION=1 the cluster had 105 regions (region split finished ... (105 regions)), so every poll walks the entire keyspace and the loop stacks past the 600s cap.

This PR removes exactly that pattern: each lock scan becomes bounded to the test's own key band (~1/10 of the keyspace, hence far fewer regions per scan), so the drain-wait (wait_for_locks_count_in_band) no longer walks all 105 regions. On the same 3-kv MULTI_REGION=1 runner, this PR's own integration-test-txn finished green in ~5m. So beyond fixing the #525 over-count, the band-bounded scans should take the pressure off #516 as well. PTAL 🙏