transaction: resolve orphaned locks whose primary was never written

[eduralph]

check_txn_status matched only ExtractedErrors, but its plan delivers per-key errors as MultipleKeyErrors, so the Rollback_if_not_exist escalation in get_txn_status_from_lock was unreachable and an orphaned secondary lock poisoned its key permanently. Accept both wrappers.

Closes #543

Signed-off-by: Eduard Ralph eduard@ralphovi.net

Summary by CodeRabbit

• Bug Fixes
  • Improved transaction status handling when multiple errors are returned.
  • Transaction-not-found conditions are now reported consistently.
  • Expired locks with missing primary records are now properly rolled back and cleaned up.
• Tests
  • Added coverage for expired-lock cleanup and verified that lock resolution occurs exactly once.

[ti-chi-bot]

Welcome @eduralph!

It looks like this is your first PR to tikv/client-rust 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to tikv/client-rust. 😃

[coderabbitai]

📝 Walkthrough

Walkthrough

check_txn_status now handles Error::MultipleKeyErrors when converting txn_not_found key failures. Tests cover an orphaned lock whose primary transaction was never written and verify rollback escalation removes the lock.

Changes

Transaction status error handling

Layer / File(s)	Summary
Expand transaction status error handling src/transaction/lock.rs	check_txn_status processes Error::MultipleKeyErrors like Error::ExtractedErrors, converting a matching KeyError into Error::TxnNotFound.
Validate orphaned-lock rollback escalation src/transaction/lock.rs	An async test captures status requests and verifies rollback_if_not_exist escalation, lock removal, and one ResolveLock invocation.

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

🚥 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 clearly identifies the primary change: resolving orphaned locks whose primary transaction was never written.
Linked Issues check	✅ Passed	The change handles both error wrappers and converts missing-primary errors to TxnNotFound, enabling rollback_if_not_exist as required by issue #543.
Out of Scope Changes check	✅ Passed	The changes are limited to check_txn_status handling and focused regression coverage for orphaned lock resolution.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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]

@ekexium to raise awareness

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ekexium

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ekexium]

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

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-07-20 02:41:27.429603463 +0000 UTC m=+1199873.465698519: ☑️ agreed by ekexium.

[eduralph]

@pingyu - we're 1 LGTM away from getting this merged

[pingyu]

How about replacing ExtractedError by reusing MultipleKeyErrors and introducing a new MultipleRegionErrors ?

It seems that the other interfaces have the same issue, and it could introduce potential bugs into future code.

p.s. Removing ExtractedError is a breaking change, but I think the next release will be a semver-major release anyway, since we have bumped the Rust toolchain and the protos upgrade is WIP.

[eduralph]

Agreed, and I think the diagnosis is sharper than "the other interfaces have the same issue" — the two variants aren't just redundant, which one you get is decided by plan shape, invisibly, at a distance.

ExtractedErrors is produced only by ExtractError, and it carries either key errors or region errors. MultipleKeyErrors is produced by single_shard_handler and always carries key errors. Which one reaches the caller depends on whether a merge sits between retry_multi_region and extract_error: without one, ExtractError sees the per-shard Vec<Result<_>> and normalizes to ExtractedErrors; with CollectSingle (or Collect), the merge pops the single Result out first, so MultipleKeyErrors becomes the plan's own error and ExtractError re-raises it without ever calling key_errors(). That is precisely the bug this PR fixes.

That coupling is already visible in the public raw API, for the same class of failure:

method	plan shape	error the user sees
delete, put_with_ttl	retry_multi_region → merge(CollectSingle) → extract_error	MultipleKeyErrors
batch_delete, batch_put_with_ttl, delete_range	retry_multi_region → extract_error	ExtractedErrors

So a caller matching Error::ExtractedErrors to inspect key errors handles batch_delete and silently misses delete. Nothing about either signature suggests that.

On the internal side I went through the four places that match ExtractedErrors today. Only LockResolver::check_txn_status (this PR) was actually broken; resolve_lock_with_retry, commit_primary_with_retry and CleanupLocks::execute are correct — but only because their chains happen to have no collapsing merge. None of them is correct by construction. Adding a merge(CollectSingle) to any of those chains would silently flip the variant and break a distant match arm with no compile error and no test failure. commit_primary_with_retry is the one I'd least like to discover that way: it matches on commit_ts_expired to retry with a newer commit ts, so a silent variant change turns a recoverable commit into a failed one.

So yes to MultipleKeyErrors + MultipleRegionErrors, and dropping ExtractedErrors. Splitting by what the payload is instead of which plan combinator produced it is what makes the match arms verifiable — and it kills a whole class of "worked until someone recomposed the plan" bugs. Agreed too that a semver-major is the moment to do it.

One request on sequencing: could this land as a follow-up rather than in this PR? This one is a two-line match-arm widening for a live bug — an orphaned lock whose primary was never written currently poisons its key forever — and it's already approved. The refactor is a breaking change touching the error enum, both producer sites, and every consumer, and it deserves its own review rather than riding in on a bug fix.

Happy to open that PR — I'd plan to introduce MultipleRegionErrors, split ExtractError's two branches across the two variants, update the four consumers, and add a test per public entry point pinning which variant it surfaces so plan recomposition can't silently change it again. Say the word and I'll put it up.

[eduralph]

Put it up as #557 (draft, stacked on this branch) so it is concrete rather than hypothetical.

It removes ExtractedErrors, reports key errors as MultipleKeyErrors and region errors as a new
MultipleRegionErrors, and collapses this PR's two-variant arm back to one. It also drops a latent
bug in CleanupLocks::execute, which popped the last error to type-test it and on the key-error path
assigned the remaining vec to key_error — so a lone key error was dropped and key_errors()
reported Some(vec![]), an error carrying no errors.

One thing worth your eye, since it is the least obvious part: HasKeyErrors for Result<T, Error>
reports any per-shard Err as a key error, so ExtractError now wraps only what the response
itself reported (KeyError from a kvrpcpb::KeyError field, KvError from a raw endpoint's string
error field) and propagates transport or exhausted-retry errors unchanged. Testing for KeyError
alone would have left the raw operations plan-shape-dependent — the very thing the split removes.

Marked draft because it is stacked; happy to rebase it onto master once this one lands, or to fold it
in here instead if you would rather have them together.