Skip to content

pkg/server: stabilize flaky TestIssue54335#67888

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
flaky-claw:flakyfixer/case_1f747bc65647-a3
Apr 23, 2026
Merged

pkg/server: stabilize flaky TestIssue54335#67888
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
flaky-claw:flakyfixer/case_1f747bc65647-a3

Conversation

@flaky-claw

@flaky-claw flaky-claw commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #67558

Problem Summary:
Flaky test TestIssue54335 in pkg/server intermittently fails, so this PR stabilizes that path.

What changed and how does it work?

Root Cause

TestIssue54335 constructed a client connection with nil pkt and relied on failpoint behavior; when the query reached result writing, writePacket dereferenced nil and panicked.

Fix

Initializing cc.pkt in the flaky test removes the nil dereference window without changing product behavior.

Verification

Spec:

  • target: pkg/server :: TestIssue54335
  • strategy: tidb.go_flaky.default
  • plan mode: BASELINE_ONLY
  • requirements: required case must execute; no skip; repeat count = 1
  • baseline gates: required_flaky_gate, build_safety_gate, intent_guard_gate

Observed result:

  • status: passed
  • required case executed: yes
  • submission decision: ALLOWED
  • scope debt present: yes

Gate checklist:

  • Required flaky gate: PASS
  • Build safety gate: PASS
  • Intent guard gate: PASS
  • Repo-wide advisory gate: SKIPPED
  • Feedback specific gate: SKIPPED

Commands:

  • go test -json ./pkg/server -run '^TestIssue54335$' -count=1
  • go test -json ./pkg/server -count=1
  • make build

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Fixes #67558

Summary by CodeRabbit

  • Tests
    • Refactored connection test implementation to leverage dedicated test session creation and management with improved session initialization patterns and handling approaches.
    • Enhanced client connection initialization process with explicit packet IO handler configuration and setup for improved test isolation and reliability.
    • Upgraded test infrastructure architecture to strengthen overall maintainability, consistency, and reliability of test execution.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed labels Apr 18, 2026
@pantheon-ai

pantheon-ai Bot commented Apr 18, 2026

Copy link
Copy Markdown

@flaky-claw 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.

@ti-chi-bot ti-chi-bot Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 18, 2026
@tiprow

tiprow Bot commented Apr 18, 2026

Copy link
Copy Markdown

Hi @flaky-claw. 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

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown

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: 13867fd9-bc15-4859-91ff-c213cae7dcb2

📥 Commits

Reviewing files that changed from the base of the PR and between ce92298 and 950f931.

📒 Files selected for processing (1)
  • pkg/server/conn_test.go

📝 Walkthrough

Walkthrough

The pull request refactors a flaky test TestIssue54335 in pkg/server/conn_test.go to use a dedicated test session created via session.CreateSession4Test(store) instead of TestKit's session, and explicitly initializes the packet IO for the manually constructed clientConn to avoid nil references.

Changes

Cohort / File(s) Summary
Test Session Refactoring
pkg/server/conn_test.go
Replaced TestKit session usage with session.CreateSession4Test(store) to manage session lifecycle; updated TiDBContext initialization and added explicit packet IO initialization (internal.NewPacketIOForTest) to prevent nil references in manually constructed clientConn.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • #67243 — Refactors tests to replace TestKit sessions with internal test sessions (session.CreateSession4Test) to avoid race conditions in similar test patterns.

Suggested labels

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

Suggested reviewers

  • YangKeao
  • lcwangchao

Poem

🐰 A flaky test once danced around,
With TestKit sessions all unbound,
But now with sessions pure and true,
And packet IO shining new,
Stability's our proud pursuit! ✨

🚥 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 title 'pkg/server: stabilize flaky TestIssue54335' accurately describes the main change—fixing a flaky test in the pkg/server package.
Description check ✅ Passed The description addresses the template requirements: issue number is provided (close #67558), problem is clearly explained (nil pkt dereference), solution is described, testing is documented (unit test), and release note is included.
Linked Issues check ✅ Passed The PR directly addresses issue #67558 by identifying and fixing the root cause of the flaky test (nil pkt dereference) and includes verification that the test now passes.
Out of Scope Changes check ✅ Passed All changes are within scope—modifications are limited to stabilizing TestIssue54335 by initializing cc.pkt and using a dedicated test session without introducing unrelated functionality.

✏️ 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

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Apr 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.4293%. Comparing base (e3f45e4) to head (950f931).
⚠️ Report is 29 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67888        +/-   ##
================================================
- Coverage   77.7969%   77.4293%   -0.3677%     
================================================
  Files          1983       1966        -17     
  Lines        548948     549053       +105     
================================================
- Hits         427065     425128      -1937     
- Misses       120962     123923      +2961     
+ Partials        921          2       -919     
Flag Coverage Δ
integration 40.9074% <ø> (+1.1101%) ⬆️
unit 76.6556% <ø> (+0.3061%) ⬆️

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

Components Coverage Δ
dumpling 61.5065% <ø> (ø)
parser ∅ <ø> (∅)
br 50.0872% <ø> (-13.0244%) ⬇️
🚀 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.

@yinsustart
yinsustart requested a review from wshwsh12 April 21, 2026 05:25
@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 21, 2026
@yinsustart

Copy link
Copy Markdown

/retest

@tiprow

tiprow Bot commented Apr 22, 2026

Copy link
Copy Markdown

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

@xzhangxian1008 xzhangxian1008 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-chi-bot

ti-chi-bot Bot commented Apr 23, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wshwsh12, xzhangxian1008

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:

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 ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 23, 2026
@ti-chi-bot

ti-chi-bot Bot commented Apr 23, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-04-21 08:32:42.192163656 +0000 UTC m=+2068367.397523703: ☑️ agreed by wshwsh12.
  • 2026-04-23 02:14:51.211476875 +0000 UTC m=+2218496.416836922: ☑️ agreed by xzhangxian1008.

@ti-chi-bot
ti-chi-bot Bot merged commit 56490d6 into pingcap:master Apr 23, 2026
35 checks passed
premal pushed a commit to premal/tidb that referenced this pull request Apr 30, 2026
yongman pushed a commit to yongman/tidb that referenced this pull request Jun 3, 2026
@ti-chi-bot ti-chi-bot Bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Jul 24, 2026
@ti-chi-bot

Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #70014.
But this PR has conflicts, please resolve them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: TestIssue54335 in pkg/server

5 participants