Skip to content

server: return unavailable when PD is not serving #11066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rleungx wants to merge 3 commits into
base: master
Choose a base branch
from
Open

server: return unavailable when PD is not serving #11066

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4646b2
server: return unavailable when PD is not serving
rleungx Jul 30, 2026
2e5b0f7
mcs/tso: return unavailable when service is not running
rleungx Jul 30, 2026
46eba19
server: keep GetMembers rate limiting first
rleungx Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions server/grpc_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,7 @@ func (s *GrpcServer) GetClusterInfo(context.Context, *pdpb.GetClusterInfoRequest
// Here we purposely do not check the cluster ID because the client does not know the correct cluster ID
// at startup and needs to get the cluster ID with the first request (i.e. GetMembers).
if s.IsClosed() {
return &pdpb.GetClusterInfoResponse{
Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, errs.ErrServerNotStarted.FastGenByArgs().Error()),
}, nil
return nil, errs.ErrNotStarted
}

var tsoServiceAddrs []string
Expand Down Expand Up @@ -428,25 +426,21 @@ func (s *GrpcServer) getMinTSFromSingleServer(

// GetMembers implements gRPC PDServer.
func (s *GrpcServer) GetMembers(context.Context, *pdpb.GetMembersRequest) (*pdpb.GetMembersResponse, error) {
// Here we purposely do not check the cluster ID because the client does not know the correct cluster ID
// at startup and needs to get the cluster ID with the first request (i.e. GetMembers).
if s.IsClosed() {
return nil, errs.ErrNotStarted
}
done, err := s.rateLimitCheck()
if err != nil {
return nil, err
}
if done != nil {
defer done()
}
// Here we purposely do not check the cluster ID because the client does not know the correct cluster ID
// at startup and needs to get the cluster ID with the first request (i.e. GetMembers).
if s.IsClosed() {
return &pdpb.GetMembersResponse{
Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, errs.ErrServerNotStarted.FastGenByArgs().Error()),
}, nil
}
members, err := s.Server.GetMembers()
if err != nil {
return &pdpb.GetMembersResponse{
Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, err.Error()),
}, nil
return getMembersErrorResult(err)
}

var etcdLeader, pdLeader *pdpb.Member
Expand All @@ -456,9 +450,7 @@ func (s *GrpcServer) GetMembers(context.Context, *pdpb.GetMembersRequest) (*pdpb
if (pdLeader == nil && leader != nil) || (etcdLeader == nil && leaderID != 0) {
members, err = s.ReloadMembers()
if err != nil {
return &pdpb.GetMembersResponse{
Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, err.Error()),
}, nil
return getMembersErrorResult(err)
}
leaderID = s.member.GetEtcdLeader()
leader = s.member.GetLeader()
Expand All @@ -473,6 +465,17 @@ func (s *GrpcServer) GetMembers(context.Context, *pdpb.GetMembersRequest) (*pdpb
}, nil
}

func getMembersErrorResult(err error) (*pdpb.GetMembersResponse, error) {
if errors.ErrorEqual(err, errs.ErrServerNotStarted.FastGenByArgs()) {
return nil, errs.ErrNotStarted
}
// Keep other server-side failures in the response header so clients don't
// mistake them for service or transport availability failures.
return &pdpb.GetMembersResponse{
Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, err.Error()),
}, nil
Comment on lines +468 to +476

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve ErrServerNotStarted through standard error wrapping.

The helper can misclassify %w-wrapped unavailable errors as response-level failures, while stack wrappers are correctly detected. Use stdlib errors.Is (or a check that follows both Cause() and Unwrap()), and add a %w-wrapped ErrServerNotStarted test case.

📍 Affects 2 files
  • server/grpc_service.go#L468-L476 (this comment)
  • server/grpc_service_test.go#L138-L149
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/grpc_service.go` around lines 468 - 476, The getMembersErrorResult
helper currently misses standard %w-wrapped ErrServerNotStarted values; update
its classification check to use stdlib errors.Is or equivalent traversal of both
Cause and Unwrap while preserving the existing direct and stack-wrapped
behavior. In server/grpc_service_test.go lines 138-149, add coverage for a
%w-wrapped ErrServerNotStarted case and verify it returns the unavailable error
rather than a response-level failure.

}

func findLeadersInMembers(members []*pdpb.Member, etcdLeaderID uint64, leader *pdpb.Member) (etcdLeader, pdLeader *pdpb.Member) {
for _, m := range members {
if m.GetMemberId() == etcdLeaderID {
Expand Down
53 changes: 53 additions & 0 deletions server/grpc_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,24 @@
package server

import (
"context"
"net"
"testing"

"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/status"
"google.golang.org/grpc/test/bufconn"

"github.com/pingcap/errors"
"github.com/pingcap/kvproto/pkg/metapb"
"github.com/pingcap/kvproto/pkg/pdpb"
"github.com/pingcap/kvproto/pkg/schedulingpb"

"github.com/tikv/pd/pkg/errs"
"github.com/tikv/pd/pkg/utils/testutil"
)

Expand Down Expand Up @@ -94,3 +103,47 @@ func TestConvertSchedulingHeaderPreservesError(t *testing.T) {
})
}
}

func TestServiceDiscoveryRPCsReturnUnavailableWhenServerIsNotRunning(t *testing.T) {
grpcServer := &GrpcServer{Server: &Server{}}
listener := bufconn.Listen(1024 * 1024)
transport := grpc.NewServer()
pdpb.RegisterPDServer(transport, grpcServer)
serveErr := make(chan error, 1)
go func() {
serveErr <- transport.Serve(listener)
}()
t.Cleanup(func() {
transport.Stop()
require.NoError(t, <-serveErr)
})
conn, err := grpc.NewClient(
"passthrough:///bufnet",
grpc.WithContextDialer(func(context.Context, string) (net.Conn, error) { return listener.Dial() }),
grpc.WithTransportCredentials(insecure.NewCredentials()),
)
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, conn.Close()) })
client := pdpb.NewPDClient(conn)

members, err := client.GetMembers(context.Background(), &pdpb.GetMembersRequest{})
require.Nil(t, members)
require.Equal(t, codes.Unavailable, status.Code(err))

clusterInfo, err := client.GetClusterInfo(context.Background(), &pdpb.GetClusterInfoRequest{})
require.Nil(t, clusterInfo)
require.Equal(t, codes.Unavailable, status.Code(err))
}

func TestGetMembersErrorResult(t *testing.T) {
notStarted := errors.WithStack(errs.ErrServerNotStarted.FastGenByArgs())
response, err := getMembersErrorResult(notStarted)
require.Nil(t, response)
require.Equal(t, codes.Unavailable, status.Code(err))

internalErr := errors.New("failed to load members")
response, err = getMembersErrorResult(internalErr)
require.NoError(t, err)
require.Equal(t, pdpb.ErrorType_UNKNOWN, response.GetHeader().GetError().GetType())
require.Equal(t, internalErr.Error(), response.GetHeader().GetError().GetMessage())
}
Loading