[lohmag]

Issue Number: close pingcap/tidb#69831

Problem Summary:

An async commit transaction is committed as soon as all its prewrites succeed: the primary lock carries the secondaries and a lock resolver can commit it. execute checks MaxTxnTimeUse only after prewrite, so a transaction older than 24 hours prewrites, then gets txn takes too much time. The caller sees an ordinary failure while the cleanup runs in the background. If that cleanup does not finish (process exit, store closed, primary unreachable), a later reader resolves the locks as committed, and a retry of the "failed" transaction applies it twice.

What is changed:

• For async commit, check MaxTxnTimeUse before prewrite, so a too old transaction fails before it writes any lock.
• Skip the post-prewrite age check for async commit. A transaction that fell back to 2PC during prewrite keeps it.

Tests:

• New TestAsyncCommitFail/TestAsyncCommitTooOldTxnNotPrewritten: an oracle reports the MaxTxnTimeUse age as expired. It fails without the fix (one prewrite is sent) and passes with it, on unistore and on a real TiKV playground (nightly).
• The issue's own probe (commitFailedSkipCleanup + expired locks) recovered both keys as committed before the fix on real TiKV. With the fix Commit fails before prewrite and the keys stay absent.
• go test ./txnkv/... ./tikv/..., cd integration_tests && go test . (unistore), and go test . -run 'TestAsyncCommit|TestAsyncCommitFail|Test1PC|TestCommitter' -with-tikv against a real TiKV pass.

Summary by CodeRabbit

• Bug Fixes
  • Async transactions that exceed the allowed time limit are now rejected before any changes are written, preventing partially applied transactions.

[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 zyguan 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
• txnkv/OWNERS

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]

Hi @lohmag. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[ti-chi-bot]

Welcome @lohmag!

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

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-go. 😃

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 036660df-54fd-46d4-aeec-bcbc23c9b05e

Commits

Reviewing files that changed from the base of the PR and between a0d0bf6 and a16c9c2.

Files selected for processing: 2

• integration_tests/async_commit_fail_test.go
• txnkv/transaction/2pc.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

Async commits now check transaction age before prewrite. A new integration test verifies that an over-age transaction returns an error without prewrite requests and leaves both keys absent.

Changes: Async Commit Age Validation

Layer / File(s)	Summary
Check async transaction age before prewrite txnkv/transaction/2pc.go, integration_tests/async_commit_fail_test.go	Async commits that exceed MaxTxnTimeUse now return an age error before prewrite. The existing later age check applies only to non-async commits. The integration test checks the error, confirms zero prewrite RPCs, and verifies neither key is visible.

Priority: Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: ekexium

Merge Risk: Minimal · up to a16c9

The change rejects over-age async transactions before prewrite. No issue requiring a fix before merge was identified.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: checking async-commit transaction age before prewrite.
Linked Issues check		Issue [#69831] requires an over-age async-commit transaction to fail before prewrite, or to return a result that matches its durable outcome. The change checks MaxTxnTimeUse before prewrite when `c.…
Out of Scope Changes check		The change is limited to the async-commit age-check ordering in txnkv/transaction/2pc.go and a focused integration test in integration_tests/async_commit_fail_test.go. Both changes directly implem…
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…

Full details: Linked Issues check

Explanation

Issue [#69831] requires an over-age async-commit transaction to fail before prewrite, or to return a result that matches its durable outcome. The change checks MaxTxnTimeUse before prewrite when c.isAsyncCommit() is true. It skips the later age check only for async commit, so a transaction that falls back to 2PC retains the later check. TestAsyncCommitTooOldTxnNotPrewritten verifies the age error, zero prewrite RPCs, and absent keys.

Full details: Out of Scope Changes check

Explanation

The change is limited to the async-commit age-check ordering in txnkv/transaction/2pc.go and a focused integration test in integration_tests/async_commit_fail_test.go. Both changes directly implement and verify issue [#69831]. No unrelated production behavior or test scope is present in the reviewed diff.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.

Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.}