session,executor: scope finishStmt failpoint to current connection

[zanmato1984]

What problem does this PR solve?

Issue Number: close #66728

Problem Summary:

TestFinishStmtError enables finishStmtError as a global failpoint, so unrelated sessions can also hit it while the test is running. That broad scope can destabilize this case and make flaky reports noisy.

What changed and how does it work?

• scope finishStmtError to the current connection when the failpoint value is a numeric connection ID
• update TestFinishStmtError to enable the failpoint with its own session connection ID
• use t.Cleanup for disable and skip when the failpoint hook is inactive in the current test binary

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test

  • I checked and no code files have been changed.

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

None

Summary by CodeRabbit

• Tests
  • Enhanced test reliability by improving error injection to be session-specific rather than universal.

[pantheon-ai]

@zanmato1984 I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

_{ℹ️ Learn more details on Pantheon AI.}

[tiprow]

Hi @zanmato1984. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 87bf1ec5-ef98-4bfc-9386-15c25743c86c

📥 Commits

Reviewing files that changed from the base of the PR and between 997e75c and 622a423.

📒 Files selected for processing (2)

• pkg/executor/staticrecordset/integration_test.go
• pkg/session/tidb.go

---

📝 Walkthrough

Walkthrough

This PR fixes a flaky test by making the finishStmtError failpoint conditional on the session's ConnectionID rather than triggering unconditionally. The test now derives the connection ID and passes it to the failpoint, while the production code checks if the injected value matches the current session before returning an error.

Changes

Cohort / File(s)

Summary

Failpoint Injection Refactoring pkg/executor/staticrecordset/integration_test.go, pkg/session/tidb.go

Updated finishStmtError failpoint to be conditional on ConnectionID matching instead of unconditional. Test now derives session connection ID via fmt, configures failpoint with return(<connID>), uses t.Cleanup() for teardown, and skips if failpoint enabling fails. Production code updated to handle multiple numeric types and only inject error when injected value matches current session's ConnectionID.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• pingcap/tidb#66851: Modifies the same finishStmt failpoint behavior and TestFinishStmtError test logic to address session-isolation issues in failpoint injection.

Suggested labels

size/XS, ok-to-test, approved, lgtm

Suggested reviewers

• YangKeao
• bb7133
• tiancaiamao

Poem

🐰 A test that danced without a care,
Now pins its error to the right affair,
By matching ConnectionID with grace,
Each session fails in its own space! ✨

🚥 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main change: scoping the finishStmt failpoint to the current connection in session and executor packages.
Description check	✅ Passed	The PR description is well-structured, follows the template, includes the linked issue (#66728), explains the problem and solution, and documents test coverage with checkboxes.
Linked Issues check	✅ Passed	The PR successfully addresses issue #66728 by scoping the finishStmtError failpoint to the current connection ID, preventing unrelated sessions from triggering the error and thus stabilizing the flaky test.
Out of Scope Changes check	✅ Passed	All changes are directly related to the scoping objective: modifications to finishStmt failpoint logic, test updates for connection ID-based failpoint triggering, and proper cleanup practices.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 and usage tips.}

[zanmato1984]

Role: Reviewer-R1

NOT_LGTM (round 1) due to one blocking issue:

• TestFinishStmtError now skips when TryDetach() returns nil error, which weakens the original test intent and can hide regressions in failpoint wiring/scope.

Please keep this path as a hard assertion (require.Error(t, err)) once failpoint enable succeeds. Handling failpoint-disabled binaries at failpoint.Enable is enough.

[zanmato1984]

This skip turns a regression into a skip/pass. The original intent is to assert the finishStmt error path is actually exercised; please keep a hard failure here (require.Error(t, err)) and only skip when failpoint cannot be enabled.

[zanmato1984]

Role: Addresser-R1
Addressed in 622a423.

Restored the hard assertion in TestFinishStmtError by replacing the skip path with require.Error(t, err).

Validation:

• ./tools/check/failpoint-go-test.sh pkg/executor/staticrecordset -run TestFinishStmtError -count=1

[zanmato1984]

Role: Reviewer-R2

LGTM (round 2).

Re-check results:

• Original test intent is preserved: TestFinishStmtError keeps require.Error(t, err) after TryDetach().
• Change remains minimal/focused (failpoint scope guard + matching test wiring).
• Recurrence analysis is evidence-based and reasonable (global failpoint scope affecting unrelated sessions).
• No unnecessary timing-only precise repro is retained in this PR.

Local validation:

• ./tools/check/failpoint-go-test.sh pkg/executor/staticrecordset -run TestFinishStmtError -count=20

[codecov]

Codecov Report

❌ Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.2064%. Comparing base (de09871) to head (622a423).
⚠️ Report is 91 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #67655        +/-   ##
================================================
- Coverage   77.5839%   77.2064%   -0.3775%     
================================================
  Files          1981       1974         -7     
  Lines        547950     564309     +16359     
================================================
+ Hits         425121     435683     +10562     
- Misses       122019     127764      +5745     
- Partials        810        862        +52     

Flag	Coverage Δ
integration	42.1298% <0.0000%> (+7.7901%)	⬆️
unit	76.7896% <57.1428%> (+0.4563%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	61.4164% <ø> (ø)
parser	∅ <ø> (∅)
br	48.8485% <ø> (-11.5827%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[YangKeao]

/retest

[zanmato1984]

/retest

[tiprow]

@zanmato1984: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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.

[yinsustart]

/retest

[tiprow]

@yinsustart: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xhebox, YangKeao

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 [YangKeao,xhebox]

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-04-14 02:20:28.838741927 +0000 UTC m=+1441234.044101984: ☑️ agreed by YangKeao.
• 2026-04-27 02:57:50.611813261 +0000 UTC m=+2566675.817173318: ☑️ agreed by xhebox.

[zanmato1984]

/retest

[tiprow]

@zanmato1984: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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.

[hawkingrei]

/retest

[tiprow]

@zanmato1984: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	622a423	link	true	/test fast_test_tiprow

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.