txnkv: preserve undetermined commit outcomes across retries and cancellation by gengliqi · Pull Request #2063 · tikv/client-go

gengliqi · 2026-09-11T05:42:30Z

Fix #2062

Description

Prevent ordinary errors from masking undetermined commit outcomes across retries, protocol fallback, and cancellation, including pipelined DML.

Preserve the undetermined state, return ErrResultUndetermined, and skip failure cleanup while the outcome remains unknown. Definitive failures retain their existing behavior.

Summary by CodeRabbit

Bug Fixes
- Improved transaction commit handling when the final outcome is unknown, preserving the correct undetermined-result status.
- Prevented later RPC or region errors from incorrectly overwriting an already undetermined transaction state.
- Correctly resolves previously uncertain outcomes when a definitive commit response or retry succeeds.
- Preserved server-reported transaction results when requests are interrupted or sessions are terminated.
- Improved async-commit fallback behavior when RPC errors occur during prewriting.

Signed-off-by: gengliqi <gengliqiii@gmail.com>

coderabbitai · 2026-09-11T05:42:49Z

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fad8aed0-e285-46ec-a022-ca9ebaeb7e6f

📥 Commits

Reviewing files that changed from the base of the PR and between 6c4efa4 and c8f725d.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 95392005-0eb5-42e2-b649-84979ce9d5aa

📥 Commits

Reviewing files that changed from the base of the PR and between 962f253 and 6c4efa4.

📒 Files selected for processing (11)

integration_tests/1pc_test.go
integration_tests/2pc_test.go
integration_tests/async_commit_fail_test.go
integration_tests/pipelined_memdb_test.go
internal/locate/region_request.go
internal/locate/region_request_test.go
txnkv/transaction/2pc.go
txnkv/transaction/commit.go
txnkv/transaction/prewrite.go
txnkv/transaction/txn_file.go
txnkv/transaction/txn_file_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

📝 Walkthrough

Walkthrough

The change preserves undetermined commit outcomes across request cancellation, prewrite retries, 2PC, 1PC, async commit, transaction-file commits, and pipelined commits. Tests cover unknown results, definitive failures, resolved RPC errors, fallback, cleanup, and rollback broadcasts.

Changes

Undetermined commit result handling

Layer / File(s)	Summary
Request and prewrite outcome preservation `internal/locate/region_request.go`, `internal/locate/region_request_test.go`, `txnkv/transaction/prewrite.go`	Region-request cancellation preserves undetermined region errors. Prewrite paths record uncertainty using the request’s 1PC state and preserve it across split retries and fallback.
Commit state recording and resolution `txnkv/transaction/2pc.go`, `txnkv/transaction/commit.go`, `txnkv/transaction/txn_file.go`, `txnkv/transaction/txn_file_test.go`	Commit paths record unknown primary outcomes, centralize conversion to `ErrResultUndetermined`, and clear stored state after definitive responses or valid timestamp retries.
Commit error integration coverage `integration_tests/1pc_test.go`, `integration_tests/2pc_test.go`, `integration_tests/async_commit_fail_test.go`, `integration_tests/pipelined_memdb_test.go`	Integration tests cover lost responses, server-unknown results, definitive failures, resolved RPC errors, async-commit fallback, cleanup behavior, and rollback broadcasts.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: ekexium

Merge Risk: ⚪ Minimal · up to 6c4ef

No confirmed merge-blocking issue remains. The commit-result handling and its regression coverage are ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 12 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR satisfies the coding requirements in `#2062`. `TestPipelinedCommitErrors` covers lost primary responses, undetermined region errors, and definitive commit failures. The tests verify `ErrResultUnd…
Out of Scope Changes check	✅ Passed	The changed production paths and tests remain connected to `#2062`. The region-request changes preserve an undetermined server response during session cancellation. The prewrite, commit, transaction-fil…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: preserving undetermined commit outcomes across retries and cancellation in txnkv.

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

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

Signed-off-by: gengliqi <gengliqiii@gmail.com>

zyguan · 2026-09-22T11:50:57Z

+		// A lost async/1PC response can leave the result unknown. Ignore RPC errors caused
+		// by another failed batch's cancellation. A split can disable 1PC locally, whereas
+		// disabling async commit requires TiKV to confirm fallback.
+		if (handler.committer.isAsyncCommit() || handler.req.Prewrite().TryOnePc) && handler.sender.GetRPCError() != nil && atomic.LoadUint32(&handler.committer.prewriteCancelled) == 0 {


Is the following concurrent ordering expected?

Starts two async-prewrite batches in separate regions.

Obtains a definitive async-fallback response for one batch but delays delivering it.

Lets the other batch's RPC error populate undeterminedErr.

Delivers the already-obtained fallback response.

Because the failed prewrite prevents any commit request, the transaction appears definitively uncommitted, but the recorded error remains and becomes ErrResultUndetermined, also skipping cleanup. The existing test TestConfirmedFallbackWithRPCError covers the opposite ordering; should previously recorded async uncertainty be reconciled when fallback is later confirmed?

Good catch. I reproduced this ordering on both the PR base and the current head, so this is a pre-existing issue.

ti-chi-bot · 2026-09-23T03:35:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ekexium, zyguan

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,zyguan]
~~txnkv/OWNERS~~ [ekexium,zyguan]

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 · 2026-09-23T03:35:59Z

[LGTM Timeline notifier]

Timeline:

2026-09-17 15:54:55.152116966 +0000 UTC m=+306941.089774560: ☑️ agreed by ekexium.
2026-09-23 03:35:58.640015117 +0000 UTC m=+162283.865236225: ☑️ agreed by zyguan.

gengliqi · 2026-09-23T06:50:22Z

/cherry-pick tidb-8.5

ti-chi-bot · 2026-09-23T06:50:24Z

@gengliqi: The label(s) /tidb-8.5 cannot be applied, because the repository doesn't have them.

Details

In response to this:

/cherry-pick tidb-8.5

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 ti-community-infra/tichi repository.

ti-chi-bot · 2026-09-23T06:51:01Z

@gengliqi: new pull request created to branch tidb-8.5: #2085.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick tidb-8.5

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 ti-community-infra/tichi repository.

…llation (#2063) (#2085) fix #2062\n\nSigned-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>\nSigned-off-by: gengliqi <gengliqiii@gmail.com>\n\nCo-authored-by: Liqi Geng <gengliqiii@gmail.com>\nCo-authored-by: gengliqi <gengliqiii@gmail.com>

u

962f253

Signed-off-by: gengliqi <gengliqiii@gmail.com>

ti-chi-bot Bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2026

u

6c4efa4

Signed-off-by: gengliqi <gengliqiii@gmail.com>

ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2026

gengliqi changed the title ~~txnkv: return undetermined errors for pipelined commits~~ txnkv: preserve undetermined commit outcomes across retries and cancellation Sep 16, 2026

tikv deleted a comment from coderabbitai Bot Sep 16, 2026

ekexium self-requested a review September 16, 2026 15:17

ekexium approved these changes Sep 17, 2026

View reviewed changes

ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 17, 2026

zyguan reviewed Sep 22, 2026

View reviewed changes

zyguan approved these changes Sep 23, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 23, 2026

Merge branch 'master' into fix-undetermined-pdml

c8f725d

coderabbitai Bot approved these changes Sep 23, 2026

View reviewed changes

ti-chi-bot Bot merged commit 1fd036c into tikv:master Sep 23, 2026
14 checks passed

ti-chi-bot mentioned this pull request Sep 23, 2026

txnkv: preserve undetermined commit outcomes across retries and cancellation (#2063) #2085

Merged

This was referenced Sep 24, 2026

Transactions may return ordinary errors when the commit result is undetermined pingcap/tidb#71585

Open

*: update client-go to fix undetermined commit handling pingcap/tidb#71587

Closed

Navigation Menu

txnkv: preserve undetermined commit outcomes across retries and cancellation - #2063

txnkv: preserve undetermined commit outcomes across retries and cancellation#2063
ti-chi-bot[bot] merged 3 commits into
tikv:masterfrom
gengliqi:fix-undetermined-pdml

gengliqi commented Sep 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

Review skipped

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

zyguan Sep 22, 2026

Uh oh!

gengliqi Sep 23, 2026

Uh oh!

ti-chi-bot Bot commented Sep 23, 2026

Uh oh!

ti-chi-bot Bot commented Sep 23, 2026

Uh oh!

Uh oh!

gengliqi commented Sep 23, 2026

Uh oh!

ti-chi-bot Bot commented Sep 23, 2026

Uh oh!

ti-chi-bot commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gengliqi commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

zyguan Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

gengliqi Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Sep 23, 2026

Uh oh!

ti-chi-bot Bot commented Sep 23, 2026

[LGTM Timeline notifier]

Uh oh!

Uh oh!

gengliqi commented Sep 23, 2026

Uh oh!

ti-chi-bot Bot commented Sep 23, 2026

Uh oh!

ti-chi-bot commented Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gengliqi commented Sep 11, 2026 •

edited

Loading

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading