Skip to content

pkg/server: restore stats lease in TestInitStatsSessionBlockGC - #67206

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
jiong-nba:jiong-nba/fix-initstats-stats-lease
Mar 23, 2026
Merged

ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
jiong-nba:jiong-nba/fix-initstats-stats-lease

Conversation

@jiong-nba

@jiong-nba jiong-nba commented Mar 23, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #67228

Problem Summary:

TestInitStatsSessionBlockGC can fail when a previous test in the same pkg/server test binary has called testkit.CreateMockStoreAndDomain(). That helper disables stats globally by setting the process-wide stats lease to -1, so the init-stats code path no longer blocks GC as this test expects.

What changed and how does it work?

This PR makes TestInitStatsSessionBlockGC save the current stats lease, set it to 3s for the duration of the test, and restore the original value afterward. That keeps the test self-contained even when earlier tests leave the global stats lease disabled.

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.

Unit test commands:

make failpoint-enable && (
  cd pkg/server &&
  go test -v -run 'TestCursorExistsFlag|TestInitStatsSessionBlockGC' -tags=intest,deadlock
); rc=$?; make failpoint-disable; exit $rc
make bazel_lint_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

Summary by CodeRabbit

  • Tests
    • Improved test stability and isolation for stats session configuration testing.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 23, 2026
Copilot AI review requested due to automatic review settings March 23, 2026 01:22
@pantheon-ai

pantheon-ai Bot commented Mar 23, 2026 •

Copy link
Copy Markdown

Review Complete

Findings: 0 issues
Posted: 0
Duplicates/Skipped: 0

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 23, 2026
@tiprow

tiprow Bot commented Mar 23, 2026

Copy link
Copy Markdown

Hi @jiong-nba. 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 Mar 23, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The test TestInitStatsSessionBlockGC is modified to snapshot the current stats lease value, restore it after execution via deferred cleanup, and set it to 3 * time.Second during the test. This isolates the test from global state modifications left by previous tests.

Changes

Cohort / File(s) Summary
Test State Isolation Fix
pkg/server/stat_test.go
Added save/restore pattern for the global stats lease configuration in TestInitStatsSessionBlockGC to prevent test interference from prior tests that modify this value without restoring it.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

Suggested reviewers

  • YuJuncen
  • Leavrth

Poem

🐰 A test once broke from borrowed state,
Stats lease forgotten, sealed its fate,
But now we save, restore, and mend,
With defer's grace, clean paths transcend! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main change: restoring the stats lease in the TestInitStatsSessionBlockGC test, which is exactly what the changeset accomplishes.
Linked Issues check ✅ Passed The PR directly addresses issue #67205 by implementing the fix: saving and restoring the process-wide stats lease within TestInitStatsSessionBlockGC, making the test independent of previous test side effects.
Out of Scope Changes check ✅ Passed The changes are limited to TestInitStatsSessionBlockGC, adding four lines to save/restore the stats lease and set it for the test duration, which is entirely within scope of fixing the reported test flakiness issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed PR description comprehensively addresses the problem, solution, test strategy, and release notes with all required sections completed.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR makes pkg/server’s TestInitStatsSessionBlockGC self-contained by preventing it from inheriting a process-global “stats lease disabled” setting left behind by earlier tests (notably via testkit.CreateMockStoreAndDomain() / session.DisableStats4Test()).

Changes:

  • Save the current global stats lease at the start of TestInitStatsSessionBlockGC.
  • Force the stats lease to 3s for the duration of the test to ensure the expected init-stats behavior.
  • Restore the original stats lease via defer after the test completes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pantheon-ai pantheon-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Code looks good. No issues found.

@YangKeao YangKeao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It might be better to modify DisableStats4Test or bootstrap to restore the stats lease for tests.

But this solution is also acceptable, as most of the tests are using bootstrap.

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

codecov Bot commented Mar 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.8202%. Comparing base (3b531d4) to head (d935856).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67206        +/-   ##
================================================
+ Coverage   77.7649%   77.8202%   +0.0553%     
================================================
  Files          2019       1942        -77     
  Lines        553441     545394      -8047     
================================================
- Hits         430383     424427      -5956     
+ Misses       121316     120955       -361     
+ Partials       1742         12      -1730     
Flag Coverage Δ
integration 40.9996% <ø> (-7.1282%) ⬇️
unit 76.9642% <ø> (+0.6761%) ⬆️

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

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

@0xPoe 0xPoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! :shipit:

/hold

Please resolve all CI failures.

@ti-chi-bot ti-chi-bot Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2026
@ti-chi-bot

ti-chi-bot Bot commented Mar 23, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xPoe, 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:

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 Mar 23, 2026
@ti-chi-bot

ti-chi-bot Bot commented Mar 23, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-03-23 07:34:35.777322594 +0000 UTC m=+167271.813392844: ☑️ agreed by YangKeao.
  • 2026-03-23 08:37:14.462022059 +0000 UTC m=+171030.498092319: ☑️ agreed by 0xPoe.

@jiong-nba

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Mar 23, 2026

Copy link
Copy Markdown

@jiong-nba: 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.

@jiong-nba

Copy link
Copy Markdown
Contributor Author

/retest

@tiprow

tiprow Bot commented Mar 23, 2026

Copy link
Copy Markdown

@jiong-nba: 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.

@jiong-nba

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@ti-chi-bot ti-chi-bot Bot added the ok-to-test Indicates a PR is ready to be tested. label Mar 23, 2026
@jiong-nba

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@jiong-nba

Copy link
Copy Markdown
Contributor Author

/retest

@jiong-nba

Copy link
Copy Markdown
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2026
@ti-chi-bot
ti-chi-bot Bot merged commit 64ac12f into pingcap:master Mar 23, 2026
38 of 39 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Mar 25, 2026
1 of 13 tasks
bb7133 added a commit to bb7133/tidb that referenced this pull request Aug 12, 2026
…independent

The new TestStmtSendLongDataMemQuotaQuery uses testkit.CreateMockStoreAndDomain,
which calls session.DisableStats4Test and leaves the process-global stats lease
at -1. TestInitStatsSessionBlockGC then no longer blocks GC as expected when the
two tests share a test shard. Reset the lease explicitly (adapted from pingcap#67206,
which fixed this on master).
bb7133 added a commit to bb7133/tidb that referenced this pull request Aug 12, 2026
…independent

The new TestStmtSendLongDataMemQuotaQuery uses testkit.CreateMockStoreAndDomain,
which calls session.DisableStats4Test and leaves the process-global stats lease
at -1. TestInitStatsSessionBlockGC then no longer blocks GC as expected when the
two tests share a test shard. Reset the lease explicitly (adapted from pingcap#67206,
which fixed this on master).
bb7133 added a commit to bb7133/tidb that referenced this pull request Sep 21, 2026
Reset the stats lease required by TestInitStatsSessionBlockGC after earlier tests disable it globally. Adapted from upstream pingcap#67206 (64ac12f) using this release branch session.SetStatsLease API.

Signed-off-by: bb7133 <bb7133@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pkg/server: TestInitStatsSessionBlockGC is affected by global stats lease left by previous tests

4 participants