Skip to content

test: take 128+SIGINT as graceful exit in graceshutdown test - #71651

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
ChangRui-Ryan:changrui_exit
Sep 29, 2026
Merged

ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
ChangRui-Ryan:changrui_exit

Conversation

@ChangRui-Ryan

@ChangRui-Ryan ChangRui-Ryan commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #67765

The user wants the same two paragraphs but with the section titles included. Output only the paste-ready content with the headings.

Problem Summary

Since #68096, tidb-server exits with code 128+SIGINT (130) after a SIGINT-triggered graceful shutdown, instead of 0. tests/graceshutdown stops tidb-server by sending os.Interrupt, so cmd.Wait() now returns an ExitError ("exit status 130"). Because this assertion (require.NoError) runs in a background goroutine, the failure calls runtime.Goexit() before close(done), leaving the main test goroutine blocked at <-done until the 10-minute go test timeout. This has been failing the merged_e2e_test Jenkins job since #68096 was merged, with no error message visible in the logs (testify output is never flushed because the test goroutine never returns).

What changed and how does it work

stopService in tests/graceshutdown now treats exit code 128+SIGINT as a graceful stop: when cmd.Wait() returns an error, it compares cmd.ProcessState.ExitCode() with 128+int(syscall.SIGINT) (the same value as exitCodeInt in cmd/tidb-server/main.go). Since SIGINT is exactly the signal the test itself sends to stop the service, this exit code means the shutdown completed gracefully. Any other non-zero exit (crash, killed by signal, etc.) is still reported as a test failure.

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

Summary by CodeRabbit

  • Tests
    • Graceful shutdown tests now recognize a process exit caused by an interrupt signal as a successful stop.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked 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. and removed do-not-merge/needs-tests-checked labels Sep 28, 2026
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 6dffd1fb-5885-40bc-8109-2946f3c1d82c

📥 Commits

Reviewing files that changed from the base of the PR and between fa3d56e and 0641629.

📒 Files selected for processing (1)
  • tests/graceshutdown/graceshutdown_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The shutdown test helper now treats process exit status 128 + SIGINT as a graceful stop. Other cmd.Wait() errors continue to be traced and returned.

Changes

Shutdown test

Layer / File(s) Summary
Handle SIGINT exit status
tests/graceshutdown/graceshutdown_test.go
The helper logs a graceful-stop message and returns nil when the process exit status is 128 + SIGINT. It continues to trace and return other wait errors.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 06416

This test-only change recognizes the server’s SIGINT exit code as a graceful stop. The supplied evidence supports that mapping, and no concrete merge-blocking risk remains.

🚥 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: treating the 128+SIGINT exit code as a graceful exit in the graceshutdown test.
Description check ✅ Passed The description includes the issue reference, problem summary, implementation details, test checklist, side-effect declarations, documentation checklist, and release note. It is mostly complete, altho…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

A rabbit checks the signal’s trail,
Then marks the graceful stop without fail.
Other errors keep their trace,
And return from their proper place.
The test hops on, its branch complete.

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

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.0264%. Comparing base (633a9e3) to head (0641629).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #71651        +/-   ##
================================================
- Coverage   76.2984%   72.0264%   -4.2721%     
================================================
  Files          2041       2110        +69     
  Lines        554492     603938     +49446     
================================================
+ Hits         423069     434995     +11926     
- Misses       130523     167193     +36670     
- Partials        900       1750       +850     
Flag Coverage Δ
integration 41.2506% <ø> (+1.5789%) ⬆️

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

Components Coverage Δ
dumpling 58.8395% <ø> (ø)
parser ∅ <ø> (∅)
br 46.5727% <ø> (-16.1371%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ChangRui-Ryan

Copy link
Copy Markdown
Contributor Author

/test pull-e2e-test

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

Reviewed the complete merge-base diff at 0641629. No P0/P1 findings. The test sends SIGINT, and the server returns 128+SIGINT after its shutdown path completes. Other non-zero exit codes remain failures, and the transaction completion assertions are preserved. Static review only; no local builds or tests were run. The current build, unit-test, and e2e checks are successful.

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

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

LGTM

@ti-chi-bot

ti-chi-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

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

ti-chi-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-09-28 09:05:07.267463297 +0000 UTC m=+614032.492684404: ☑️ agreed by wjhuang2016.
  • 2026-09-29 08:35:16.188899147 +0000 UTC m=+698641.414120254: ☑️ agreed by bb7133.

@ti-chi-bot
ti-chi-bot Bot merged commit 12b639a into pingcap:master Sep 29, 2026
35 checks passed
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