Skip to content

Conversation

@ljluestc
Copy link

What problem does this PR solve?

Issue Number: Close #9390

What is changed and how does it work?

server: fix io.EOF wrapping in gRPC streams (heartbeatServer, tsoServer, bucketHeartbeatServer) to ensure correct error comparison

The `bucketHeartbeatServer`, `tsoServer`, and `heartbeatServer` wrappers in `pd/server/grpc_service.go` were unconditionally wrapping errors returned by `Recv()` and `Send()` with `errors.WithStack`. This included `io.EOF`, which is a sentinel error used to signal the end of a stream.

When `io.EOF` is wrapped, standard equality checks (`err == io.EOF`) fail. Upstream callers (like `server.recv` in `ReportBuckets`) that expect to handle stream termination gracefully instead receive a generic error, potentially causing incorrect error handling flow or unnecessary noise in logs.

I modified `pd/server/grpc_service.go` to explicitly check for `io.EOF` in the error handling blocks of these methods. If `io.EOF` is detected, it is returned directly without wrapping.

Verification

I added 5 new regression test cases in server/grpc_service_test.go to verify correct io.EOF handling and state management for each affected server wrapper. These tests mock the stream to return io.EOF and assert that the wrapper returns it unwrapped and sets the closed state.

  • TestBucketHeartbeatServerRecvEOF: Verify bucketHeartbeatServer Recv behavior.
  • TestHeartbeatServerSendEOF: Verify heartbeatServer Send behavior.
  • TestHeartbeatServerRecvEOF: Verify heartbeatServer Recv behavior.
  • TestTsoServerSendEOF: Verify tsoServer Send behavior.
  • TestTsoServerRecvEOF: Verify tsoServer recv behavior.

To run the verification tests:

go test -v ./server/ -run "Test.*EOF"

Check List

Tests

  • Unit test

Code changes

  • No code

Side effects

  • No side effects

Related changes

  • No related changes

Release note

server: fix io.EOF wrapping in gRPC streams (heartbeatServer, tsoServer, bucketHeartbeatServer) to ensure correct error comparison

The bucketHeartbeatServer, tsoServer, and heartbeatServer wrappers were unconditionally wrapping errors with errors.WithStack, masking io.EOF. This change returns io.EOF explicitly to restore correct stream termination handling.

Unit tests added in server/grpc_service_test.go.
@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 23, 2026
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 23, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign husharp for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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

@coderabbitai
Copy link

coderabbitai bot commented Jan 23, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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 contribution This PR is from a community contributor. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jan 23, 2026
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 23, 2026

Hi @ljluestc. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. dco-signoff: no Indicates the PR's author has not signed dco. labels Jan 23, 2026
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 24, 2026
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 24, 2026

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

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.

@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.66%. Comparing base (1b40b8d) to head (84d76dc).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10190      +/-   ##
==========================================
- Coverage   78.73%   78.66%   -0.08%     
==========================================
  Files         522      521       -1     
  Lines       70090    70052      -38     
==========================================
- Hits        55188    55108      -80     
- Misses      10935    11000      +65     
+ Partials     3967     3944      -23     
Flag Coverage Δ
unittests 78.66% <86.66%> (-0.08%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. dco-signoff: no Indicates the PR's author has not signed dco. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wrong usage of gRPC interface

1 participant