Skip to content

transport: add status details even when aborting early#8754

Merged
arjan-bal merged 3 commits intogrpc:masterfrom
DataDog:joy.bestourous/missing-details
Dec 15, 2025
Merged

transport: add status details even when aborting early#8754
arjan-bal merged 3 commits intogrpc:masterfrom
DataDog:joy.bestourous/missing-details

Conversation

@joybestourous
Copy link
Contributor

@joybestourous joybestourous commented Dec 8, 2025

Modifies earlyAbortStreamHandler to include status details if present.

Most use cases of earlyAbortStreamHandler are for circumstances where there are certainly no error details (bad HTTP methods, bad content-type, internal error, etc). However, tap handlers also typically go through the earlyAbortStreamHandler. In http2_server.go:

	if t.inTapHandle != nil {
		var err error
		if s.ctx, err = t.inTapHandle(s.ctx, &tap.Info{FullMethodName: s.method, Header: mdata}); err != nil {
			t.mu.Unlock()
			if t.logger.V(logLevel) {
				t.logger.Infof("Aborting the stream early due to InTapHandle failure: %v", err)
			}
			stat, ok := status.FromError(err)
			if !ok {
				stat = status.New(codes.PermissionDenied, err.Error())
			}
			t.controlBuf.put(&earlyAbortStream{
				// ...
				status:         stat,  // <-- CAN have details!
			})

Yet the handler does not include error details by default, limiting how tap handlers can be used and breaking some user assumptions surrounding which information is propagated.
This PR fixes this by checking for status details and including the header for them if present.

RELEASE NOTES:

  • transport: propagate status details from tap handlers.

@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

❌ Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.14%. Comparing base (f9d2bdb) to head (82b787d).
⚠️ Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
internal/transport/controlbuf.go 42.85% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8754      +/-   ##
==========================================
- Coverage   83.35%   83.14%   -0.22%     
==========================================
  Files         418      418              
  Lines       32377    32513     +136     
==========================================
+ Hits        26989    27033      +44     
+ Misses       4022     4014       -8     
- Partials     1366     1466     +100     
Files with missing lines Coverage Δ
internal/transport/controlbuf.go 65.56% <42.85%> (-22.71%) ⬇️

... and 35 files with indirect coverage changes

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

@joybestourous joybestourous force-pushed the joy.bestourous/missing-details branch 3 times, most recently from 9bd760d to 10252fe Compare December 8, 2025 19:55
@joybestourous joybestourous force-pushed the joy.bestourous/missing-details branch from 10252fe to 1b5a905 Compare December 9, 2025 22:10
@atollena atollena added Type: Feature New features or improvements in behavior Type: Behavior Change Behavior changes not categorized as bugs labels Dec 11, 2025
@atollena atollena added this to the 1.78 Release milestone Dec 11, 2025
@arjan-bal arjan-bal modified the milestones: 1.78 Release, 1.79 Release Dec 11, 2025
@arjan-bal arjan-bal added Area: Server Includes Server, Streams and Server Options. and removed Type: Behavior Change Behavior changes not categorized as bugs labels Dec 11, 2025
@arjan-bal arjan-bal self-requested a review December 11, 2025 16:21
Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

LGTM, adding a second reviewer. Please address the minor comments.

Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

LGTM, modulo minor nits

Comment on lines 2159 to 2162
st := status.New(codes.ResourceExhausted, "rate limit exceeded")
st, err := st.WithDetails(wantDetails)
if err != nil {
t.Fatalf("status.WithDetails() failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: All these could be moved to be inside the tapHandler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Each stream is handled in a separate goroutine on the server. We should not be calling t.Fatalf outside the main test function, see https://google.github.io/styleguide/go/best-practices#dont-call-tfatal-from-separate-goroutines.

We should either use t.Errorf and return an error, or revert back to creating the status object in the main test function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should not be calling t.Fatalf from outside the main test goroutine. But we don't expect this ever fail. We've handcrafted the status right here, and WithDetails fails only when the code is OK or if the struct can't be marshaled into an Any.

I'm Ok with changing it to t.Errorf or switching it back to how it was earlier.

@easwars easwars removed their assignment Dec 11, 2025
@arjan-bal arjan-bal merged commit 4b2d967 into grpc:master Dec 15, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Server Includes Server, Streams and Server Options. Type: Feature New features or improvements in behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants