Skip to content

pkg/ddl/tests/tiflash: stabilize flaky TestTiFlashProgressAfterAvailable (#68122)#69032

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:release-8.5from
ti-chi-bot:cherry-pick-68122-to-release-8.5
Jun 11, 2026
Merged

pkg/ddl/tests/tiflash: stabilize flaky TestTiFlashProgressAfterAvailable (#68122)#69032
ti-chi-bot[bot] merged 1 commit into
pingcap:release-8.5from
ti-chi-bot:cherry-pick-68122-to-release-8.5

Conversation

@ti-chi-bot

@ti-chi-bot ti-chi-bot commented Jun 9, 2026

Copy link
Copy Markdown
Member

This is an automated cherry-pick of #68122

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

  • Tests
    • Refined TiFlash test infrastructure, including improved session management and resource cleanup procedures in test teardown operations.

@ti-chi-bot ti-chi-bot added 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. type/cherry-pick-for-release-8.5 This PR is cherry-picked to release-8.5 from a source PR. labels Jun 9, 2026
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

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: d9fda663-6a65-49ab-b3ed-b0458cd2d0bd

📥 Commits

Reviewing files that changed from the base of the PR and between b7447ab and ef01e6c.

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

📝 Walkthrough

Walkthrough

This PR addresses test flakiness in TestTiFlashProgressAfterAvailable by correcting resource teardown ordering and isolating test execution to a dedicated session, preventing shared testkit state interference.

Changes

TiFlash Test Reliability Fixes

Layer / File(s) Summary
Teardown shutdown order correction
pkg/ddl/tests/tiflash/ddl_tiflash_test.go
Domain (s.dom.Close()) is now closed before acquiring the TiFlash mock lock; the TiFlash status server close is then executed under lock with a nil-check, inverting the prior close sequence.
Test session isolation
pkg/ddl/tests/tiflash/ddl_tiflash_test.go
TestTiFlashProgressAfterAvailable creates a new session via session.CreateSessionAndSetID and executes database selection, table creation, and TiFlash replica alteration through session.MustExec, replacing reliance on the shared testkit.TestKit instance.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • pingcap/tidb#68122: Both PRs modify createTiFlashContext teardown shutdown ordering and TestTiFlashProgressAfterAvailable to use session.CreateSessionAndSetID instead of testkit.TestKit, targeting the same test flakiness.
  • pingcap/tidb#68115: Both PRs update pkg/ddl/tests/tiflash/ddl_tiflash_test.go to stop using testkit.TestKit and instead execute TiFlash SQL via dedicated session.CreateSessionAndSetID, addressing similar test isolation concerns.

Suggested labels

cherry-pick-approved, approved, lgtm

Suggested reviewers

  • OliverS929
  • CalvinNeo
  • GMHDBJD
  • wjhuang2016

Poem

🐰 A teardown reordered, a session pristine,
No more shared state to muddy the scene,
The TiFlash test flakes now fade away,
Clean isolation saves test day! ✨

🚥 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 accurately describes the main change: stabilizing a flaky test in the tiflash DDL tests by adjusting teardown order.
Description check ✅ Passed The PR description follows the template with issue number, problem summary, detailed explanation of root cause and fix, verification results, and proper checklist completion.
Linked Issues check ✅ Passed The PR successfully addresses issue #67420 by implementing the necessary teardown race condition fix to stabilize the flaky test TestTiFlashProgressAfterAvailable.
Out of Scope Changes check ✅ Passed All code changes are directly related to fixing the flaky test teardown issue in TestTiFlashProgressAfterAvailable; no out-of-scope changes detected.

✏️ 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.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


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 Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (release-8.5@b7447ab). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff                @@
##             release-8.5     #69032   +/-   ##
================================================
  Coverage               ?   56.5888%           
================================================
  Files                  ?       1843           
  Lines                  ?     685006           
  Branches               ?          0           
================================================
  Hits                   ?     387637           
  Misses                 ?     268827           
  Partials               ?      28542           
Flag Coverage Δ
integration 40.5860% <ø> (?)
unit 66.0975% <ø> (?)

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

Components Coverage Δ
dumpling 55.3108% <0.0000%> (?)
parser ∅ <0.0000%> (?)
br 63.9332% <0.0000%> (?)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ti-chi-bot ti-chi-bot Bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Jun 9, 2026
@yinsustart

Copy link
Copy Markdown

/retest

@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 10, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 Jun 10, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-10 10:48:16.462303746 +0000 UTC m=+956997.532621126: ☑️ agreed by GMHDBJD.
  • 2026-06-10 14:29:32.650797063 +0000 UTC m=+970273.721114463: ☑️ agreed by wjhuang2016.

@yinsustart

Copy link
Copy Markdown

/retest

@ti-chi-bot
ti-chi-bot Bot merged commit d6b8329 into pingcap:release-8.5 Jun 11, 2026
21 of 22 checks passed
@ti-chi-bot
ti-chi-bot Bot deleted the cherry-pick-68122-to-release-8.5 branch June 11, 2026 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cherry-pick-approved Cherry pick PR approved by release team. lgtm 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. type/cherry-pick-for-release-8.5 This PR is cherry-picked to release-8.5 from a source PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants