txn: fix resolver cache usage for async commit

[cfzjywxk]

Ref: pingcap/tidb#59494

1. Do not call checkSecondaries if the transaction status is determined for async commit transactions.
2. Small refactor on the TxnStatus usages.
3. Do not use ttl = 0 setting in test cases.

[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 ask for approval from cfzjywxk, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

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

[Copilot]

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

txnkv/txnlock/lock_resolver.go:1029

• Typo in comment: 'wih' should be 'with'.

// The `checkAllSecondaries` finishes wih no errors.

[Copilot]

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

txnkv/txnlock/lock_resolver.go:1021

• [nitpick] Consider renaming 'toResolveKeys' to 'resolveKeys' for clarity and consistency with its usage in the async commit resolution functions.

var toResolveKeys [][]byte

txnkv/txnlock/lock_resolver.go:1044

• [nitpick] Consider adding a clarifying comment to explain the purpose and behavior when the 'resolveAsyncCommitLockReturn' failpoint is triggered to improve code readability.

if _, err := util.EvalFailpoint("resolveAsyncCommitLockReturn"); err == nil {

txnkv/txnlock/lock_resolver_test.go:18

• Consider adding a test case that covers the branch for async commit transactions with undetermined status (i.e., where checkAllSecondaries is called) to ensure this logic is fully exercised.

lock := func(key, primary string, startTS uint64, useAsyncCommit bool, secondaries [][]byte) *kvrpcpb.LockInfo {

[cfzjywxk]

/retest

[MyonKeminta]

Understanding this part of code has become much more difficult than I would imagine...

[MyonKeminta]

In my memory, whether the TxnStatus type stands for commited or rolled back is strictly defined by the field ttl and commitTS. Is the previous definition wrong now? May this change cause other unexpected side effect?

[cfzjywxk]

The TxnStatus is an abstraction that introduces some complexity. In the current implementation, TxnStatus is determined by the KV response from CheckTxnStatus. Whether a transaction is rolled back depends on the TTL and the specified action, while whether it's committed depends on whether the commit timestamp is greater than 0.

To make it clear two seperate functions IsCommitted and IsRolledback are abstarcated, according to the current CheckTxnStatus implementation.

TxnStatus::RolledBack => resp.set_action(Action::NoAction),
TxnStatus::TtlExpire => resp.set_action(Action::TtlExpireRollback),
TxnStatus::LockNotExist => resp.set_action(Action::LockNotExistRollback),
TxnStatus::Committed { commit_ts } => {
    resp.set_commit_version(commit_ts.into_inner())
}

Only from these four types of respones the transaction status is determined.

[MyonKeminta]

This might include those keys that has already been resolved. But it seems it's still faster than checking secondary again and then resolve them as 1 RPC is faster than 2.
Is it that you've already considered that? I suggest that this can be noted in comments.

[cfzjywxk]

Yes, I would add some comments. The perform should be minial as resolving async commit locks is not a common operation.

[MyonKeminta]

Can't remenber the purpose of this code. Could you explain this change?

[cfzjywxk]

The

if status.primaryLock != nil && status.primaryLock.UseAsyncCommit && !forceSyncCommit {
			if !lr.store.GetOracle().IsExpired(txnID, cmdResp.LockTtl, &oracle.Option{TxnScope: oracle.GlobalTxnScope}) {
				status.ttl = cmdResp.LockTtl
			}
		}

logic is to change TxnStatus.ttl to zero, so the

if status.ttl != 0
   return

if status.primaryLock != nil && status.primaryLock.usaAsyncCommit && !forceSyncCommit {
    resolveAsyncCommit
}

resolveAsyncCommit could be reached in the resolve function.

I’ve moved the TTL expiration check back into the resolve function. This helps avoid modifying the TxnStatus state deep inside the call stack—especially since setting the TTL to 0 can be confusing. It also ensures that the TTL field remains read-only throughout the process.

[cfzjywxk]

In actual code, all lock TTLs should be greater than 0. Whether a lock has expired should ideally be determined by the upper layer, rather than by modifying the internal state of TxnStatus IMO.