Fix CallInfo header/trailer propagation on error responses by emcfarlane · Pull Request #892 · connectrpc/connect-go

emcfarlane · 2025-09-30T23:28:15Z

This fixes CallInfo to properly propagate headers and trailers when handlers return errors, and makes error metadata accessible through CallInfo on the client side.

Headers/trailers set via CallInfo.ResponseHeader() and CallInfo.ResponseTrailer() in handlers are now merged into the connection before error responses are sent.

Fixes #885

This fixes CallInfo to propagate headers and trailers on an error response and to receive error metadata as trailers. Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

jhump

I don't think you need to do anything special with err.Meta(). Did the tests fail until you added that? If so, I think that's worth more exploration.

jhump · 2025-10-01T13:12:38Z

context.go

+	combined := make(http.Header)
+	if w.base != nil {
+		for k, v := range w.base.ResponseTrailer() {
+			combined[k] = v
+		}
+	}
+	if w.err != nil {
+		for k, v := range w.err.Meta() {
+			combined[k] = v
+		}
+	}
+	return combined


This doesn't seem right. On the client side, we already get the response headers and trailers from the underlying streaming conn and then use those to populate err.Meta(). Won't this duplicate metadata? Also, err.Meta() is not able to distinguish between headers and trailers, while the underlying connection is, so if this does not duplicate metadata, I think we have a problem that headers sent by the server will get mashed together in the error metadata and then incorrectly reported by the client as trailers.

I've reworked this. Error metadata is not re-added to the trailers. This was duplicated for the grpc/grpcWeb case, but connect always returned the metadata as headers in the unary case. I've updated the docs for Error.Meta as this stated:

Metadata attached to errors returned by unary handlers is always sent as HTTP headers, regardless of the protocol.

Which disagrees with the implementation comments in protocol_grpc.go. This is maybe odd? But I wasn't sure if changing this behaviour would be a non-breaking change.

Yeah, this is a bit muddled. I think we ended up here because the gRPC spec didn't (and still doesn't) make it clear when trailers-only responses are used. So the worry is that implementations could arbitrarily choose to use them and mash headers and trailers together in the process.

Importantly, there are no remarks in the spec about what servers should do and, in particular, no remarks in the spec to the effect of "if handler needs to be able to classify response metadata as headers or trailers, then trailers-only cannot be used if there is header metadata in the response, all metadata in a trailers-only response is interpreted as trailers". However, that latter statement is actually what is implemented in all gRPC implementations I know of... except connect, where the handler is (or at least was, before ContextInfo) unable to classify response metadata as headers or trailers for errors to unary RPCs.

I'm the one that changed the behavior of gRPC-Web to sometimes send them as trailers instead of headers, in #677.

And the gRPC protocol has always sent them as trailers, not as headers, out of concern for how the response would be encoded into HTTP/2 frames and whether it would actually conform to the spec. Though I am now certain it could send them as headers via a "proper" trailers-only response, because I was able to get the Conformance reference server to work that way (connectrpc/conformance#904).

So we definitely need to revise that comment. I don't even think it's valuable to describe what happens on the wire: instead it should just state that clients using CallInfo should always see the error metadata interpreted as trailers.

The error metadata might not always be interpreted as trailers (in the CallInfo.ResponseTrailer field). So maybe the comment should instead be to check the connect Error.Metadata which include both the headers and trailers?

Yes, Error.Metadata is the sane way to handle this so you don't have to worry about what protocol was used.

jhump · 2025-10-01T13:14:38Z

connect_ext_test.go

+			}
+			if request.GetNumber() < 0 {
+				callInfo.ResponseHeader().Set("x-custom-key", "ping-error")
+				callInfo.ResponseHeader().Set("x-callinfo-only", "from-callinfo")


Seems like we might also want a x-callinfo-and-error key that is set from both and expected to be merged.

We should also be explicitly setting callInfo.ResponseTrailer() metadata (for both success and error cases).

jhump · 2025-10-01T13:16:54Z

connect_ext_test.go

+		headerValue := callInfo.ResponseHeader().Get("x-custom-key")
+		assert.Equal(t, headerValue, "ping-error")
+		callinfoOnlyValue := callInfo.ResponseHeader().Get("x-callinfo-only")
+		assert.Equal(t, callinfoOnlyValue, "from-callinfo")
+		// Error.Meta() should be in ResponseTrailer.
+		errorMetaValue := callInfo.ResponseTrailer().Get("x-error-meta")
+		assert.Equal(t, errorMetaValue, "from-error-meta")


I think we could use more assertions, like explicitly verifying that keys that should only be in trailers are not present in headers and vice versa.

Added asserts for empty returns in the non-expected cases.

jhump · 2025-10-01T13:20:04Z

context.go

+		for k, v := range w.err.Meta() {
+			combined[k] = v
+		}


I think the tests are passing because:

You are not asserting that items that should be headers are not present in trailers. (err.Meta() will contain both as it provides no way to distinguish the two cases).

You are straight-overwriting here instead of merging (i.e. instead of combined[k] = append(combined[k], v...). So if the values are duplicated between conn trailers and err.Meta(), this is basically a no-op and doesn't reveal an issue in the tests.

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

context.go

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

error.go

jhump · 2025-10-03T16:30:31Z

connect_ext_test.go

+			headerValue := callInfo.ResponseHeader().Get("x-custom-key")
+			assert.Equal(t, headerValue, "ping-error")
+			assert.Equal(t, callInfo.ResponseHeader().Get("x-header-only"), "should-not-be-in-trailers")
+			assert.Equal(t, callInfo.ResponseTrailer().Get("x-trailer-only"), "should-not-be-in-headers")
+			assert.Equal(t, callInfo.ResponseHeader().Get("x-trailer-only"), "")


using http.Header.Get() seems risky because it will succeed, even if the headers actually include more than one value. I think you need to use Values() and then update expectation to be a single-element slice literal.

Assert the values now.

connect_ext_test.go

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

This PR contains the following updates: | Package | Change | Age | Confidence | |---|---|---|---| | [connectrpc.com/connect](https://github.com/connectrpc/connect-go) | `v1.18.1` -> `v1.19.1` | [![age](https://developer.mend.io/api/mc/badges/age/go/connectrpc.com%2fconnect/v1.19.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/connectrpc.com%2fconnect/v1.18.1/v1.19.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>connectrpc/connect-go (connectrpc.com/connect)</summary> ### [`v1.19.1`](https://github.com/connectrpc/connect-go/releases/tag/v1.19.1) [Compare Source](connectrpc/connect-go@v1.19.0...v1.19.1)  #### What's Changed ##### Bugfixes - Fix bounds check on envelope for 32-bit archs by [@emcfarlane](https://github.com/emcfarlane) in [#887](connectrpc/connect-go#887) - Fix CallInfo header/trailer propagation on error responses by [@emcfarlane](https://github.com/emcfarlane) in [#892](connectrpc/connect-go#892) **Full Changelog**: <connectrpc/connect-go@v1.19.0...v1.19.1> ### [`v1.19.0`](https://github.com/connectrpc/connect-go/releases/tag/v1.19.0) [Compare Source](connectrpc/connect-go@v1.18.1...v1.19.0) This release introduces the highly requested "simple" flag for code generation, making Connect significantly more ergonomic for everyday RPC development. The new simple flag in protoc-gen-connect-go generates cleaner, more intuitive client and handler interfaces that eliminate request/response wrappers for most use cases. This addresses community feedback about verbosity and provides a more straightforward API. When enabled, metadata (headers/trailers) can be passed through context instead of explicit wrapper objects, optimizing for the common case where developers don't need direct access to HTTP headers. #### What's Changed ##### Enhancements - Add simple flag for more ergonomic generated code by [@bufdev](https://github.com/bufdev) and [@smaye81](https://github.com/smaye81) in [#851](connectrpc/connect-go#851) - Update Go to v1.24 and document http.Protocol use removing the dependency on `golang.org/x/net/http2` by [@maxbrunet](https://github.com/maxbrunet) in [#873](connectrpc/connect-go#873), [#877](connectrpc/connect-go#877) - Add support for Edition 2024 by [@emcfarlane](https://github.com/emcfarlane) in [#878](connectrpc/connect-go#878) ##### Bugfixes - Include valid spec and headers when calling recover handler for streaming RPCs by [@jhump](https://github.com/jhump) in [#817](connectrpc/connect-go#817) ##### Other changes - Go version support updated to latest two instead of three by [@jhump](https://github.com/jhump) in [#837](connectrpc/connect-go#837) - CI testing improvements by [@pkwarren](https://github.com/pkwarren) and [@jhump](https://github.com/jhump) in [#838](connectrpc/connect-go#838), [#839](connectrpc/connect-go#839) - Code quality improvements by [@mattrobenolt](https://github.com/mattrobenolt) and [@bufdev](https://github.com/bufdev) in [#841](connectrpc/connect-go#841), [#867](connectrpc/connect-go#867) - Documentation improvements by [@adlion](https://github.com/adlion) and [@stefanvanburen](https://github.com/stefanvanburen) in [#821](connectrpc/connect-go#821), [#880](connectrpc/connect-go#880) #### New Contributors - [@adlion](https://github.com/adlion) made their first contribution in [#821](connectrpc/connect-go#821) - [@maxbrunet](https://github.com/maxbrunet) made their first contribution in [#873](connectrpc/connect-go#873) - [@stefanvanburen](https://github.com/stefanvanburen) made their first contribution in [#880](connectrpc/connect-go#880) **Full Changelog**: <connectrpc/connect-go@v1.18.1...v1.19.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/1078 Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org> Co-authored-by: Renovate Bot <bot@kriese.eu> Co-committed-by: Renovate Bot <bot@kriese.eu>

Fix CallInfo response trailer to include error metadata

ccf2857

This fixes CallInfo to propagate headers and trailers on an error response and to receive error metadata as trailers. Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

jhump reviewed Oct 1, 2025

View reviewed changes

emcfarlane added 2 commits October 1, 2025 16:24

Merge don't override headers

cefde44

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

Assert protocol errors

a6c7c7b

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

emcfarlane force-pushed the ed/propagateErrMeta branch from e2a60cf to a6c7c7b Compare October 1, 2025 17:37

emcfarlane commented Oct 1, 2025

View reviewed changes

context.go Show resolved Hide resolved

emcfarlane requested a review from jhump October 1, 2025 18:26

Fix docs

3dc9b43

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

jhump reviewed Oct 3, 2025

View reviewed changes

emcfarlane added 5 commits October 3, 2025 21:27

Compare on header.Values

cd13ef4

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

Assert error metadata

5b6e738

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

Drop Error docs

474c71e

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

Use a test helper

55ad11e

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

Fix testcase

21431bd

Signed-off-by: Edward McFarlane <emcfarlane@buf.build>

jhump approved these changes Oct 7, 2025

View reviewed changes

Merge branch 'main' into ed/propagateErrMeta

0f6a438

emcfarlane enabled auto-merge (squash) October 7, 2025 19:31

emcfarlane merged commit a19db8a into main Oct 7, 2025
9 checks passed

emcfarlane deleted the ed/propagateErrMeta branch October 7, 2025 19:32

emcfarlane mentioned this pull request Oct 7, 2025

Prepare for v1.19.1 #895

Merged

emcfarlane mentioned this pull request Mar 5, 2026

Error metadata is sent as trailers instead of headers in unary RPCs #850

Closed

Conversation

emcfarlane commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhump left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emcfarlane Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emcfarlane commented Sep 30, 2025 •

edited

Loading

emcfarlane Oct 1, 2025 •

edited

Loading