retry: delegate killed checks to signal handler by YangKeao · Pull Request #2042 · tikv/client-go

YangKeao · 2026-08-04T17:38:20Z

Problem

TiDB needs to run richer kill-signal handling at interruptible TiKV retry checkpoints so it can cooperatively detect a disconnected client while a statement is blocked in TiKV. The existing client-go code only reads the legacy Killed flag, and the 2PC path duplicates that check.

This is needed by pingcap/tidb#68682.

Downstream integration: pingcap/tidb#70343.

What changed

Add the optional kv.KillSignalHandler interface to kv.Variables.
Let Backoffer.CheckKilled delegate to the handler when configured, while preserving the existing Killed fallback for compatibility.
Reuse Backoffer.CheckKilled for interruptible 2PC actions instead of duplicating the atomic kill-flag check.
Add unit coverage for handler precedence, legacy fallback, and nil variables.

Compatibility

This is an additive API change. Existing callers that only populate Variables.Killed keep the previous behavior. TiDB can opt in by installing its SQL kill-signal handler.

Tests

go test ./config/retry ./txnkv/transaction

Summary by CodeRabbit

New Features
- Added configurable handling for kill signals during interruptible operations.
- Kill-signal handlers can now provide custom interruption errors.
- Existing kill-state behavior remains supported as a fallback.
Bug Fixes
- Improved cancellation checks before processing transaction batches.
- Correctly reports query interruptions caused by kill signals.
- Avoids unnecessary checks when kill-related settings are unavailable.
- Improved reliability when kill-signal handling changes during concurrent operations.

ti-chi-bot · 2026-08-04T17:38:25Z

[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 cfzjywxk 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

coderabbitai · 2026-08-04T17:38:41Z

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: c3c6ee48-c29d-402e-96aa-ad6837fbbd50

📥 Commits

Reviewing files that changed from the base of the PR and between aff3e66 and 1a233db.

📒 Files selected for processing (4)

config/retry/backoff.go
config/retry/backoff_test.go
kv/variables.go
txnkv/transaction/2pc.go

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

config/retry/backoff.go
txnkv/transaction/2pc.go
config/retry/backoff_test.go
kv/variables.go

📝 Walkthrough

Walkthrough

KillSignalHandler is added to Variables. Backoffer.CheckKilled uses the handler when configured and preserves legacy kill-flag behavior otherwise. Interruptible transaction batch actions now use this check, with tests covering interruption and concurrent handler updates.

Changes

Kill signal handling

Layer / File(s)	Summary
Handler contract and backoff integration `kv/variables.go`, `config/retry/backoff.go`	`Variables` stores and loads a kill-signal handler atomically. `Backoffer.CheckKilled` handles unset variables, invokes the configured handler, and otherwise checks `Killed`.
Transaction checkpoint and validation `txnkv/transaction/2pc.go`, `config/retry/backoff_test.go`	Interruptible batch actions return the result of `bo.CheckKilled()` before processing batches. Tests cover handler errors, signal interruption, no-kill behavior, and concurrent handler replacement.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TransactionAction
  participant Backoffer
  participant Variables
  participant KillSignalHandler
  TransactionAction->>Backoffer: CheckKilled()
  Backoffer->>Variables: LoadKillSignalHandler()
  Variables-->>Backoffer: configured handler or nil
  Backoffer->>KillSignalHandler: HandleSignal()
  KillSignalHandler-->>Backoffer: error or nil
  Backoffer-->>TransactionAction: interruption result

🚥 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 and concisely describes the primary change: delegating killed checks to a signal handler.
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

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

coderabbitai

Actionable comments posted: 2

🤖 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 `@kv/variables.go`:
- Around line 39-40: Document the exported HandleSignal method in the
KillSignalHandler interface with a clear Go doc comment describing when it
returns an error, following standard Go documentation conventions.

In `@txnkv/transaction/2pc.go`:
- Around line 1103-1105: Extend transaction-level tests in 2pc_test.go beyond
TestMinCommitTsManager to cover doActionOnBatches with an interruptible batch
and a backoffer configured with a KillSignalHandler that returns
ErrQueryInterrupted. Assert that doActionOnBatches returns that interruption
error before invoking batch handling.

🪄 Autofix

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: 285cb786-9601-4402-bc04-5b5820e4703a

📥 Commits

Reviewing files that changed from the base of the PR and between 341d469 and da5feb9.

📒 Files selected for processing (4)

config/retry/backoff.go
config/retry/backoff_test.go
kv/variables.go
txnkv/transaction/2pc.go

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 `@kv/variables.go`:
- Around line 58-66: Synchronize access to Variables.KillSignalHandler between
SetKillSignalHandler and Backoffer.CheckKilled to eliminate races when shared
Variables are reused. Treat handler configuration as lifecycle state and either
protect both reads and writes with the existing synchronization mechanism or
store and retrieve the handler through a concurrency-safe copy/source, while
preserving the current handler precedence behavior.

🪄 Autofix

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: 52f9cd7b-82ca-4a37-afcd-fb1192fa5e49

📥 Commits

Reviewing files that changed from the base of the PR and between da5feb9 and 416c4d8.

📒 Files selected for processing (4)

config/retry/backoff.go
config/retry/backoff_test.go
kv/variables.go
txnkv/transaction/2pc.go

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

config/retry/backoff.go
txnkv/transaction/2pc.go
config/retry/backoff_test.go

YangKeao · 2026-08-04T18:35:46Z

/test pull-integration-test-nextgen

coderabbitai · 2026-08-06T04:15:44Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>

coderabbitai · 2026-08-06T04:22:43Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

YangKeao · 2026-08-06T04:33:34Z

@@ -49,6 +58,30 @@ type Variables struct {
 	// When its value is 0, it's not killed
 	// When its value is not 0, it's killed, the value indicates concrete reason.
 	Killed *uint32


If we don't need to keep compatibility, we can remove this field. I'm not sure about the interface strategy for client-go so I still kept this.

YangKeao · 2026-08-06T04:41:44Z

/test pull-integration-test-nextgen

ti-chi-bot Bot added the dco-signoff: yes Indicates the PR's author has signed the dco. label Aug 4, 2026

ti-chi-bot Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 4, 2026

YangKeao mentioned this pull request Aug 4, 2026

server: detect client disconnects in explicit transactions pingcap/tidb#70343

Open

13 tasks

coderabbitai Bot reviewed Aug 4, 2026

View reviewed changes

Comment thread kv/variables.go

Comment thread txnkv/transaction/2pc.go

YangKeao force-pushed the fix-68682-explicit-txn-disconnect branch from da5feb9 to 416c4d8 Compare August 4, 2026 18:28

coderabbitai Bot reviewed Aug 4, 2026

View reviewed changes

Comment thread kv/variables.go Outdated

YangKeao force-pushed the fix-68682-explicit-txn-disconnect branch 2 times, most recently from 0c306f8 to bd0b65f Compare August 6, 2026 04:15

ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2026

retry: delegate killed checks to signal handler

1a233db

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>

YangKeao force-pushed the fix-68682-explicit-txn-disconnect branch from bd0b65f to 1a233db Compare August 6, 2026 04:22

YangKeao commented Aug 6, 2026

View reviewed changes

cfzjywxk requested review from ekexium, lcwangchao, wfxr and zyguan August 6, 2026 06:26

Navigation Menu

Uh oh!

retry: delegate killed checks to signal handler - #2042

retry: delegate killed checks to signal handler#2042
YangKeao wants to merge 1 commit into
tikv:masterfrom
YangKeao:fix-68682-explicit-txn-disconnect

YangKeao commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

ti-chi-bot Bot commented Aug 4, 2026

Uh oh!

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

YangKeao commented Aug 4, 2026

Uh oh!

coderabbitai Bot commented Aug 6, 2026

Uh oh!

coderabbitai Bot commented Aug 6, 2026

Uh oh!

YangKeao Aug 6, 2026

Uh oh!

YangKeao commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

YangKeao commented Aug 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

What changed

Compatibility

Tests

Summary by CodeRabbit

Uh oh!

ti-chi-bot Bot commented Aug 4, 2026

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YangKeao commented Aug 4, 2026

Uh oh!

coderabbitai Bot commented Aug 6, 2026

Uh oh!

coderabbitai Bot commented Aug 6, 2026

Uh oh!

YangKeao Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

YangKeao commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

YangKeao commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading