Skip to content

pkg/ddl/tests/tiflash: stabilize flaky TestTiFlashProgressAfterAvailable#68122

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
flaky-claw:flakyfixer/case_89db978500ff-a2
May 27, 2026
Merged

pkg/ddl/tests/tiflash: stabilize flaky TestTiFlashProgressAfterAvailable#68122
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
flaky-claw:flakyfixer/case_89db978500ff-a2

Conversation

@flaky-claw

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

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #67420

Problem Summary:
Flaky test TestTiFlashProgressAfterAvailable in pkg/ddl/tests/tiflash intermittently fails, so this PR stabilizes that path.

What changed and how does it work?

Root Cause

the original flaky is still a teardown race where the mock TiFlash status endpoint could be closed before the DDL poller stopped, and this round’s finding was that removing the close entirely fixed the panic but leaked the mock server goroutines.

Fix

the necessary patch restores mock-server teardown after Domain.Close() drains the DDL background loops, preserving the earlier session.CreateSessionAndSetID validation-compatibility fix for TestTiFlashProgressAfterAvailable.

Verification

Spec:

  • target: pkg/ddl/tests/tiflash :: TestTiFlashProgressAfterAvailable
  • 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
  • feedback surface source: baseline_only

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/ddl/tests/tiflash -run '^TestTiFlashProgressAfterAvailable$' -count=1
  • go test -json ./pkg/ddl/tests/tiflash -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 #67420

Summary by CodeRabbit

  • Bug Fixes

    • Improved shutdown sequence to prevent potential nil pointer issues during teardown.
  • Tests

    • Enhanced test implementation for improved reliability and proper resource cleanup.

@ti-chi-bot ti-chi-bot Bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 29, 2026
@pantheon-ai

pantheon-ai Bot commented Apr 29, 2026

Copy link
Copy Markdown

Review failed due to infrastructure/execution failure after retries. Please re-trigger review.

Root cause: Branch flakyfixer/case_89db978500ff-a2 was not found in the pingcap/tidb repository after exhaustive search (3 attempts). The only similar branch found was flakyfixer/case_98b033e509a6-a1. Please verify the branch has been pushed to origin and re-trigger the review.

ℹ️ 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 29, 2026
@tiprow

tiprow Bot commented Apr 29, 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 29, 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: 9e4b6311-ba58-47bf-ba91-7a166169b046

📥 Commits

Reviewing files that changed from the base of the PR and between 36bfd11 and 46236c6.

📒 Files selected for processing (1)
  • pkg/ddl/tests/tiflash/ddl_tiflash_test.go

📝 Walkthrough

Walkthrough

The PR fixes a flaky test in TestTiFlashProgressAfterAvailable by reordering the teardown sequence in createTiFlashContext (closing domain earlier and conditionally closing the StatusServer only when non-nil), and refactoring the test to use explicit sessions created via session.CreateSessionAndSetID instead of testkit, with proper session cleanup.

Changes

Cohort / File(s) Summary
Test Cleanup and Session Management
pkg/ddl/tests/tiflash/ddl_tiflash_test.go
Refactored createTiFlashContext teardown to close domain earlier and add nil-check before closing StatusServer; updated TestTiFlashProgressAfterAvailable to use explicit session creation with session.CreateSessionAndSetID instead of testkit, executing DDL statements through the session with proper defer cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • #67797: Switches test session management from testkit.TestKit to explicit session.CreateSessionAndSetID for DDL execution in the same test file.
  • #67888: Applies the same pattern of replacing testkit-based sessions with explicitly created session objects for test execution.
  • #67796: Modifies domain and session lifecycle ordering in test teardown/setup sequences.

Suggested labels

size/S, approved, lgtm

Suggested reviewers

  • wjhuang2016
  • expxiaoli
  • tiancaiamao

Poem

🐰 ✨ A flaky test once danced around,
But now the rabbit fixed the sound!
Domains close in proper time,
Sessions freed—oh, how sublime!
No nil pointers shall appear,
When shutdown's order's crystal clear! 🎉

🚥 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 describes the main change: stabilizing a flaky test in the tiflash DDL test suite, which matches the code modifications focusing on teardown ordering and session management.
Description check ✅ Passed The PR description is comprehensive and follows the template structure with issue linkage, root cause analysis, detailed fix explanation, and verification steps with gate results.
Linked Issues check ✅ Passed The PR directly addresses issue #67420 by stabilizing the flaky test TestTiFlashProgressAfterAvailable through proper teardown ordering and session management, eliminating the race condition between mock server closure and DDL poller shutdown.
Out of Scope Changes check ✅ Passed All changes in the PR are scoped to stabilizing the TestTiFlashProgressAfterAvailable test via teardown fixes and session handling, with no unrelated modifications to other parts of the codebase.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

Command failed


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
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

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

@codecov

codecov Bot commented Apr 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.9344%. Comparing base (6ebbfa3) to head (46236c6).
⚠️ Report is 124 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68122        +/-   ##
================================================
- Coverage   77.7563%   76.9344%   -0.8219%     
================================================
  Files          1990       2053        +63     
  Lines        551752     590460     +38708     
================================================
+ Hits         429022     454267     +25245     
- Misses       121810     132345     +10535     
- Partials        920       3848      +2928     
Flag Coverage Δ
integration 48.1866% <ø> (+8.3848%) ⬆️

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

Components Coverage Δ
dumpling 60.4888% <ø> (ø)
parser ∅ <ø> (∅)
br 67.2000% <ø> (+4.1065%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yinsustart

Copy link
Copy Markdown

/retest

@tiprow

tiprow Bot commented May 2, 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.

@yinsustart

Copy link
Copy Markdown

/retest

@tiprow

tiprow Bot commented May 2, 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.

@yinsustart

Copy link
Copy Markdown

/retest

@tiprow

tiprow Bot commented May 6, 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.

@yinsustart

Copy link
Copy Markdown

/retest

@tiprow

tiprow Bot commented May 7, 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.

@yinsustart

Copy link
Copy Markdown

/test pull-unit-test-ddlv1

@tiprow

tiprow Bot commented May 7, 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:

/test pull-unit-test-ddlv1

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

Copy link
Copy Markdown

/test pull-unit-test-ddlv1

@tiprow

tiprow Bot commented May 10, 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:

/test pull-unit-test-ddlv1

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
yinsustart requested a review from CalvinNeo May 11, 2026 03:20
@ti-chi-bot

ti-chi-bot Bot commented May 11, 2026

Copy link
Copy Markdown

@CalvinNeo: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

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 ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 14, 2026
@yinsustart
yinsustart requested a review from OliverS929 May 14, 2026 13:39
@ti-chi-bot

ti-chi-bot Bot commented May 26, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CalvinNeo, GMHDBJD, wjhuang2016

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 May 26, 2026
@ti-chi-bot

ti-chi-bot Bot commented May 26, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-05-14 09:12:03.339311219 +0000 UTC m=+343291.872090539: ☑️ agreed by GMHDBJD.
  • 2026-05-26 01:53:29.31301121 +0000 UTC m=+317079.283176274: ☑️ agreed by wjhuang2016.

@tiprow

tiprow Bot commented May 26, 2026

Copy link
Copy Markdown

@flaky-claw: The following tests 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 46236c6 link true /test fast_test_tiprow
tidb_parser_test 46236c6 link true /test tidb_parser_test

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.

@yinsustart

Copy link
Copy Markdown

/retest

@tiprow

tiprow Bot commented May 27, 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.

@ti-chi-bot
ti-chi-bot Bot merged commit 5b35a62 into pingcap:master May 27, 2026
36 of 38 checks passed
@yinsustart yinsustart added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Jun 9, 2026
@ti-chi-bot

Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #69032.

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: TestTiFlashProgressAfterAvailable in pkg/ddl/tests/tiflash

6 participants