From 1b5a905b5e6ff2f71d9c772fb10ea3b3bcf131bd Mon Sep 17 00:00:00 2001 From: joybestourous Date: Mon, 8 Dec 2025 14:54:12 -0500 Subject: [PATCH 1/3] Add status details when aborting early revive errs --- internal/transport/controlbuf.go | 9 ++++++ test/end2end_test.go | 50 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/internal/transport/controlbuf.go b/internal/transport/controlbuf.go index 2dcd1e63bdd2..f5f1c728651d 100644 --- a/internal/transport/controlbuf.go +++ b/internal/transport/controlbuf.go @@ -32,8 +32,10 @@ import ( "golang.org/x/net/http2/hpack" "google.golang.org/grpc/internal/grpclog" "google.golang.org/grpc/internal/grpcutil" + istatus "google.golang.org/grpc/internal/status" "google.golang.org/grpc/mem" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" ) var updateHeaderTblSize = func(e *hpack.Encoder, v uint32) { @@ -854,6 +856,13 @@ func (l *loopyWriter) earlyAbortStreamHandler(eas *earlyAbortStream) error { {Name: "grpc-message", Value: encodeGrpcMessage(eas.status.Message())}, } + if p := istatus.RawStatusProto(eas.status); len(p.GetDetails()) > 0 { + stBytes, err := proto.Marshal(p) + if err == nil { + headerFields = append(headerFields, hpack.HeaderField{Name: grpcStatusDetailsBinHeader, Value: encodeBinHeader(stBytes)}) + } + } + if err := l.writeHeader(eas.streamID, true, headerFields, nil); err != nil { return err } diff --git a/test/end2end_test.go b/test/end2end_test.go index 9157c525c094..665b2be0723d 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -2154,6 +2154,56 @@ func testTap(t *testing.T, e env) { } } +func (s) TestTapStatusDetails(t *testing.T) { + wantDetails := &testpb.Empty{} + st := status.New(codes.ResourceExhausted, "rate limit exceeded") + st, err := st.WithDetails(wantDetails) + if err != nil { + t.Fatalf("status.WithDetails() failed: %v", err) + } + + tapHandler := func(_ context.Context, _ *tap.Info) (context.Context, error) { + // Return error with details for all RPCs. + return nil, st.Err() + } + + ss := &stubserver.StubServer{ + EmptyCallF: func(_ context.Context, _ *testpb.Empty) (*testpb.Empty, error) { + // This should never be called since TAP handler rejects the RPC. + return &testpb.Empty{}, nil + }, + } + sopts := []grpc.ServerOption{grpc.InTapHandle(tapHandler)} + if err := ss.Start(sopts); err != nil { + t.Fatalf("Error starting server: %v", err) + } + defer ss.Stop() + + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + _, err = ss.Client.EmptyCall(ctx, &testpb.Empty{}) + if err == nil { + t.Fatal("EmptyCall() succeeded; want error") + } + + gotStatus := status.Convert(err) + if gotStatus.Code() != codes.ResourceExhausted { + t.Fatalf("EmptyCall() returned code %v; want %v", gotStatus.Code(), codes.ResourceExhausted) + } + if gotStatus.Message() != "rate limit exceeded" { + t.Fatalf("EmptyCall() returned message %q; want %q", gotStatus.Message(), "rate limit exceeded") + } + + details := gotStatus.Details() + if len(details) != 1 { + t.Fatalf("EmptyCall() returned %d details; want 1", len(details)) + } + if _, ok := details[0].(*testpb.Empty); !ok { + t.Fatalf("EmptyCall() returned detail type %T; want *testpb.Empty", details[0]) + } +} + func (s) TestEmptyUnaryWithUserAgent(t *testing.T) { for _, e := range listTestEnv() { testEmptyUnaryWithUserAgent(t, e) From 2c329c8f786b2b165ba9037f4b094fec6f3b9acf Mon Sep 17 00:00:00 2001 From: joybestourous Date: Thu, 11 Dec 2025 15:43:10 -0500 Subject: [PATCH 2/3] clean up tests, log failure to marshal --- internal/transport/controlbuf.go | 5 ++++- test/end2end_test.go | 36 +++++++++++++------------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/internal/transport/controlbuf.go b/internal/transport/controlbuf.go index f5f1c728651d..35ceb1928102 100644 --- a/internal/transport/controlbuf.go +++ b/internal/transport/controlbuf.go @@ -32,6 +32,7 @@ import ( "golang.org/x/net/http2/hpack" "google.golang.org/grpc/internal/grpclog" "google.golang.org/grpc/internal/grpcutil" + "google.golang.org/grpc/internal/pretty" istatus "google.golang.org/grpc/internal/status" "google.golang.org/grpc/mem" "google.golang.org/grpc/status" @@ -858,7 +859,9 @@ func (l *loopyWriter) earlyAbortStreamHandler(eas *earlyAbortStream) error { if p := istatus.RawStatusProto(eas.status); len(p.GetDetails()) > 0 { stBytes, err := proto.Marshal(p) - if err == nil { + if err != nil { + l.logger.Errorf("Failed to marshal rpc status: %s, error: %v", pretty.ToJSON(p), err) + } else { headerFields = append(headerFields, hpack.HeaderField{Name: grpcStatusDetailsBinHeader, Value: encodeBinHeader(stBytes)}) } } diff --git a/test/end2end_test.go b/test/end2end_test.go index 665b2be0723d..ab350c07c164 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -2155,44 +2155,38 @@ func testTap(t *testing.T, e env) { } func (s) TestTapStatusDetails(t *testing.T) { - wantDetails := &testpb.Empty{} - st := status.New(codes.ResourceExhausted, "rate limit exceeded") - st, err := st.WithDetails(wantDetails) - if err != nil { - t.Fatalf("status.WithDetails() failed: %v", err) - } - - tapHandler := func(_ context.Context, _ *tap.Info) (context.Context, error) { + tapHandler := func(context.Context, *tap.Info) (context.Context, error) { // Return error with details for all RPCs. + wantDetails := &testpb.Empty{} + st := status.New(codes.ResourceExhausted, "rate limit exceeded") + st, err := st.WithDetails(wantDetails) + if err != nil { + t.Fatalf("status.WithDetails() failed: %v", err) + } return nil, st.Err() } - ss := &stubserver.StubServer{ - EmptyCallF: func(_ context.Context, _ *testpb.Empty) (*testpb.Empty, error) { - // This should never be called since TAP handler rejects the RPC. - return &testpb.Empty{}, nil - }, - } - sopts := []grpc.ServerOption{grpc.InTapHandle(tapHandler)} - if err := ss.Start(sopts); err != nil { - t.Fatalf("Error starting server: %v", err) - } + ss := stubserver.StartTestService(t, nil, grpc.InTapHandle(tapHandler)) defer ss.Stop() + if err := ss.StartClient(); err != nil { + t.Fatalf("ss.StartClient() failed: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - _, err = ss.Client.EmptyCall(ctx, &testpb.Empty{}) + _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}) if err == nil { t.Fatal("EmptyCall() succeeded; want error") } gotStatus := status.Convert(err) if gotStatus.Code() != codes.ResourceExhausted { - t.Fatalf("EmptyCall() returned code %v; want %v", gotStatus.Code(), codes.ResourceExhausted) + t.Errorf("EmptyCall() returned code %v; want %v", gotStatus.Code(), codes.ResourceExhausted) } if gotStatus.Message() != "rate limit exceeded" { - t.Fatalf("EmptyCall() returned message %q; want %q", gotStatus.Message(), "rate limit exceeded") + t.Errorf("EmptyCall() returned message %q; want %q", gotStatus.Message(), "rate limit exceeded") } details := gotStatus.Details() From 82b787d0761a174b181ebf00f7b1e7d3d8bcea9d Mon Sep 17 00:00:00 2001 From: joybestourous Date: Fri, 12 Dec 2025 13:10:09 -0500 Subject: [PATCH 3/3] errorf -> fatalf --- test/end2end_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/end2end_test.go b/test/end2end_test.go index ab350c07c164..037ab1a8db83 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -2161,7 +2161,7 @@ func (s) TestTapStatusDetails(t *testing.T) { st := status.New(codes.ResourceExhausted, "rate limit exceeded") st, err := st.WithDetails(wantDetails) if err != nil { - t.Fatalf("status.WithDetails() failed: %v", err) + t.Errorf("status.WithDetails() failed: %v", err) } return nil, st.Err() }