Skip to content

sessionctx: avoid IMDS leak in TestSetTiDBCloudStorageURI#67604

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
D3Hunter:codex/fix-testsettidbcloudstorageuri-flake
Apr 8, 2026
Merged

sessionctx: avoid IMDS leak in TestSetTiDBCloudStorageURI#67604
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
D3Hunter:codex/fix-testsettidbcloudstorageuri-flake

Conversation

@D3Hunter
Copy link
Copy Markdown
Contributor

@D3Hunter D3Hunter commented Apr 8, 2026

What problem does this PR solve?

Issue Number: ref #65526

Problem Summary:
TestSetTiDBCloudStorageURI is flaky in CI because it can trigger AWS IMDS credential probing when validating s3://blackhole without explicit credentials. The test assertions pass, but background HTTP connection goroutines from that probe can still be alive when goleak runs, causing intermittent shard failures.

What changed and how does it work?

This PR adds a test-scoped environment guard in TestSetTiDBCloudStorageURI:

  • t.Setenv("AWS_EC2_METADATA_DISABLED", "true")

This disables EC2 IMDS probing only for this test process, removing the flaky goroutine source while keeping the existing behavior checks and assertions unchanged.

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
    • Improved test stability by configuring environment settings to prevent unnecessary background operations during test execution.

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 8, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 8, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 8, 2026

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: 6296c1ff-28b9-4fa7-b991-d153b6a9acfb

📥 Commits

Reviewing files that changed from the base of the PR and between 078b070 and 92939d9.

📒 Files selected for processing (1)
  • pkg/sessionctx/variable/tests/session_test.go

📝 Walkthrough

Walkthrough

A test in the session variable module now sets the AWS_EC2_METADATA_DISABLED environment variable before executing the TestSetTiDBCloudStorageURI test. This prevents the AWS SDK from initiating background HTTP metadata lookups during test execution.

Changes

Cohort / File(s) Summary
Test Environment Configuration
pkg/sessionctx/variable/tests/session_test.go
Added t.Setenv("AWS_EC2_METADATA_DISABLED", "true") to disable AWS IMDS metadata probing during the test, preventing unwanted background goroutines from spawning.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A test once danced with AWS in the night,
Background goroutines spawning out of sight,
But now with DISABLED=true, we set it right,
No more metadata chasing—clean and tight! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: disabling IMDS leak in a specific test function. It is concise, directly related to the changeset, and helps teammates understand the primary fix.
Description check ✅ Passed The description is comprehensive and follows the template structure. It includes issue number, clear problem summary, explanation of the fix, completed checklist with unit test marked, and release notes section marked as 'None'.

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

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

@ti-chi-bot ti-chi-bot Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 8, 2026
@D3Hunter D3Hunter marked this pull request as ready for review April 8, 2026 07:03
@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2026
@pantheon-ai
Copy link
Copy Markdown

pantheon-ai Bot commented Apr 8, 2026

The master agent encountered an error while processing. Please try again later.

ℹ️ Learn more details on Pantheon AI.

@tiprow
Copy link
Copy Markdown

tiprow Bot commented Apr 8, 2026

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

@D3Hunter D3Hunter requested a review from lance6716 April 8, 2026 07:03
@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.5060%. Comparing base (ee3439d) to head (92939d9).
⚠️ Report is 22 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67604        +/-   ##
================================================
- Coverage   77.7488%   77.5060%   -0.2429%     
================================================
  Files          1959       1964         +5     
  Lines        543393     551953      +8560     
================================================
+ Hits         422482     427797      +5315     
- Misses       120070     124142      +4072     
+ Partials        841         14       -827     
Flag Coverage Δ
integration 40.9044% <ø> (+4.7296%) ⬆️
unit 76.6461% <ø> (+0.2611%) ⬆️

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

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

@D3Hunter
Copy link
Copy Markdown
Contributor Author

D3Hunter commented Apr 8, 2026

/retest

@tiprow
Copy link
Copy Markdown

tiprow Bot commented Apr 8, 2026

@D3Hunter: 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
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 8, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joechenrh, lance6716

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 [joechenrh,lance6716]

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 8, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 8, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-04-08 07:05:34.768467341 +0000 UTC m=+939939.973827388: ☑️ agreed by joechenrh.
  • 2026-04-08 08:04:00.480043928 +0000 UTC m=+943445.685403995: ☑️ agreed by lance6716.

@ti-chi-bot ti-chi-bot Bot merged commit a69ecb8 into pingcap:master Apr 8, 2026
35 of 36 checks passed
@D3Hunter D3Hunter deleted the codex/fix-testsettidbcloudstorageuri-flake branch April 8, 2026 08:12
okJiang pushed a commit to okJiang/tidb that referenced this pull request Apr 23, 2026
)

ref pingcap#65526

(cherry picked from commit a69ecb8)
Signed-off-by: okjiang <819421878@qq.com>
YuJuncen added a commit to YuJuncen/tidb that referenced this pull request May 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm 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.

3 participants