client: resolve the gRPC unary response status before the HTTP status - #257
Merged
Conversation
iainmcgin
force-pushed
the
fix/grpc-unary-response-divergences
branch
from
July 24, 2026 18:33
c5cce68 to
438e7f3
Compare
Collaborator
Author
|
The conformance gap this PR relies on is now filed upstream as connectrpc/conformance#1119 (non-200 carrying a gRPC status — zero cases pair the two, and connect-go and grpc-go disagree) and connectrpc/conformance#1120 (reserved envelope flag bytes). #1119 is the one that bears on reviewing this PR: it records that the divergence is between the two reference implementations rather than something we invented, with the measured behaviour of each and the mechanism in their source. If upstream settles the question one way, this PR's choice should follow it. |
Two divergences between the unary gRPC response parser and its streaming sibling. A trailers-only error carries its status in the initial headers, and a proxy may pair those headers with a non-200 status - an Envoy local reply sends 503 alongside `grpc-status: 14`. The unary path reported only `HTTP error 503` and discarded the server's code, message and metadata. It now resolves the gRPC status whenever the reply plausibly speaks gRPC, falling back to the HTTP status otherwise. The trailers-only test is `!has_body_data`, matching the streaming classifier and the conformance suite's ignore-header-if-body-present cases. This matches grpc-go, which decides gRPC-ness from the content-type and disregards the HTTP status entirely, and diverges from connect-go, which decides from the HTTP status and never reads the gRPC one - discarding the server's code, message and metadata in the process. Verified by running both against the same wire bytes. A reply that does not speak gRPC keeps the HTTP-derived code, so a 502 carrying an HTML error page is still `Unavailable` and still retryable, with the content-type or framing failure appended as detail. That rule is applied wherever a body fails to parse, so the two spellings of the same proxy failure cannot produce different retry decisions. The gRPC-Web trailer-frame check appears twice in the same function and only the first was gated on the protocol. `0x80` is not a defined flag in plain gRPC and envelope decoding ignores unknown bits, so a body frame with the high bit set was parsed as a trailer block and replaced trailers already captured from the HTTP/2 trailer section. Both Go clients reject such a frame; we now do too. Adds `envelope::flags::GRPC_WEB_TRAILER` and moves the remaining raw flag-bit tests onto the named constants. Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
iainmcgin
force-pushed
the
fix/grpc-unary-response-divergences
branch
from
July 24, 2026 23:23
438e7f3 to
78b94e5
Compare
iainmcgin
marked this pull request as ready for review
August 1, 2026 21:13
asacamano
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #241
Fixes #242
Two divergences between the unary gRPC response parser and its streaming sibling.
#241 — a proxy's trailers-only error lost its message and metadata on unary calls. The unary path checked the HTTP status first and returned immediately, so
grpc-statuswas never consulted. Given an Envoy local reply carrying503alongsidegrpc-status: 14andgrpc-message: upstream connect error, a server-streaming call reportedUnavailable("upstream connect error")with the server's metadata and a unary call reportedUnavailable("HTTP error 503")with both discarded.The unary path now resolves the gRPC status whenever the reply plausibly speaks gRPC, and keeps the HTTP-derived code otherwise. The header status is honoured only for a genuine trailers-only response, tested as
!has_body_data.#242 — on plain gRPC, a high bit in the first body byte could overwrite the real trailers. The gRPC-Web trailer-frame check appears twice in this function and only the first was gated on the protocol.
0x80is the gRPC-Web trailer sentinel and is not a defined flag in plain gRPC, butEnvelope::decode_with_limitdoes not validate unknown flag bits, so the frame was parsed andgrpc_trailerswas replaced — including over trailers already captured from the HTTP/2 trailer section. Both sites are gated now, and the sentinel gets a name.Verified against real Go implementations
Both changes are wire-visible interpretation, and neither case is reachable through conformance — the suite has no non-200 raw-response case and no conformant server emits
0x80. So I built a raw-h2c harness emitting each shape exactly and drove connect-go v1.19.1, grpc-go v1.81.1, this branch and its base against identical bytes.The premise in #241 was half wrong, in our favour. connect-go does check the HTTP status first —
grpcValidateResponsereturnshttpToCode(status)before touching content-type or any gRPC status. grpc-go does not: a valid gRPC content-type setsisGRPCand the HTTP-status branch is never entered. Measured on the Envoy shape, grpc-go returnsUnavailable("upstream connect error")and so does this branch, byte for byte; on500+grpc-status: 3both returnInvalidArgument("bad argument"). It is the base tree that matched connect-go. So this matches grpc-go and diverges from connect-go, on a case where connect-go discards the server's code, message and metadata entirely.On
0x80, both Go clients already reject the frame — connect-go with "invalid envelope flags 128", grpc-go with "received unexpected payload format 128". The base tree was the outlier: it parsed the attacker's frame as gRPC-Web trailers and reportedNotFound, overriding real HTTP/2 trailers that saidgrpc-status: 0. This removes a divergence rather than creating one.No incompatibility is introduced. Across fourteen scenarios there is no case where a Go client completes a call and this branch fails it. Every difference is a different error for a call that fails in all four clients.
The interop run also caught a regression in an earlier revision of this PR: a 502 carrying an HTML page reported
Unknownwhere base, connect-go and grpc-go all reportUnavailable, losing the retryability signal. Fixed, and the rule is now applied wherever a body fails to parse, so the two spellings of the same proxy failure cannot produce different retry decisions.One pre-existing divergence found and deliberately not adopted: grpc-go accepts a successful response delivered on HTTP 503. We and connect-go reject it, which seems right.
Conformance
The trailers-only gate is load-bearing and an earlier revision got it wrong — hoisting the header check above the body read fails four cases named
trailers-only/ignore-header-if-body-present, measured at 12 passed / 4 failed versus 16/16 for this one. Full suites: gRPC-Web client 2838/0, gRPC client 1452/1454 with both failures in the load-inducedTimeoutsflake that is equally present on base, and thegRPC Unexpected Responsessubset 22/0.One note on CI: two
handler::testselement-budget tests fail onmainright now, independently of this change — fixture rot from buffa 0.9.1, fixed by #239.