connectrpc / connectrpc/connect-rust
client: four independent response parsers keep producing the same class of bug
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 509
- Forks
- 66
- Avg merge
- 2d 8h
- Merged PRs (30d)
- 10
Description
Summary
The client has four independent implementations of "turn an HTTP response into a Result", and two independent implementations of the Connect END_STREAM state machine. They parse untrusted input from the remote peer, they disagree with each other, and the disagreements are the root cause of a family of bugs that has already been fixed twice in separate PRs.
This issue is the umbrella. The individual divergences are tracked separately and can land independently; this one is about removing the structure that keeps producing them.
The four parsers
All in connectrpc/src/client/mod.rs:
| Function | Covers |
|---|---|
parse_connect_unary_response |
Connect unary |
parse_grpc_unary_response |
gRPC and gRPC-Web unary, reused by gRPC client-streaming |
parse_connect_client_stream_response |
Connect client-streaming |
make_server_stream |
server-streaming and bidi, all three protocols |
Each performs some subset of {trailers-only check, HTTP status check, content-type validation, unsupported-encoding pre-check, Connect JSON error-body parse, trailer- header split, error-metadata attachment} — in a different order, with different coverage.
Measured coverage across the twelve protocol × RPC-kind cells: response content-type is validated in 2 of 12, and the unsupported-encoding pre-check runs in 1 of 12. The Connect non-2xx JSON error-body block is copy-pasted three times, near-verbatim, and the three copies differ in whether they include the body text in the fallback message and whether they split trailer--prefixed headers.
The two END_STREAM implementations
ServerStream::process_end_stream and parse_connect_client_stream_envelopes both implement: decompress envelope, parse END_STREAM JSON, attach headers on parse error, cap trailer metadata, classify the terminal outcome. They share only the leaf helpers (parse_connect_end_stream, end_stream_error_to_connect_error, append_metadata_capped); the policy around those leaves is written twice.
BidiRecvHalf shows the shape that works — it delegates to ServerStream rather than reimplementing, and has never drifted.
Evidence this is costing us
Three fixes have had to be applied twice, once per implementation:
- #146 → #163 — truncated-response detection. #163's description says it is "matching the
ServerStreamConnect EOF path (#146)". - #192 → #201 — header preservation on a malformed END_STREAM. #201's description says the maintainer review on #192 "called out the asymmetry".
- #169 — the reported trailer-
HeaderMappanic neededappend_metadata_cappedinserted at two call sites in a single commit.
And the family is still open: #202 inventories four remaining metadata-attachment gaps, #203 the content-type gap, #241 a precedence inversion between the unary and streaming paths, #242 a missing protocol gate.
None of these were caught by conformance. All six suites drive a well-behaved reference server, so every divergence lives in a cell a conformant server never exercises — proxy-synthesized responses, advertised-but-unused encodings, hostile flag bytes.
Proposed direction
Two extractions, in order:
-
One response-head classifier.
fn classify_response_head(status, &HeaderMap, Protocol, &ClientConfig) -> Result<ResponseHead, ConnectError>running one fixed sequence — trailers-only, status, content-type, encoding support — called by all four parsers. This closes #203 and #241 structurally and deletes two of the three Connect error-body copies. -
One Connect END_STREAM decoder. A
push-style state machine owning envelope classification, decompression, END_STREAM parsing, trailer capping and terminal classification.ServerStreamfeeds it body frames;parse_connect_client_stream_envelopesfeeds it the whole buffered body and adds only its own "exactly one data message" rule. This closes the remaining items in #202.
Deliberately not proposed: a trait Protocol with three impls. The protocol differences are not uniformly shaped — Connect unary has no envelope and returns Full<Bytes>, gRPC unary returns a two-frame body, gRPC-Web moves trailers into the body — and those are different body types, correctly modelled today as distinct variants. Protocol is a Copy enum whose matches inline; boxing it would add a vtable hop to a path where the code already pre-parses HeaderName statics to save under a percent of CPU. The protocol-varying pieces the classifier needs are already methods on the enum.
Size
Roughly a 300–400 line refactor of code that is well covered at the edges. Worth doing after the individual fixes land, so each fix stays small and reviewable and the refactor can be validated as behaviour-preserving against the tests they add.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in connectrpc/src/client/mod.rs by reading the four response parsers, ServerStream::process_end_stream, and parse_connect_client_stream_envelopes; review the related divergences in #202, #203, #241, and #242. Done means the shared response-head and Connect END_STREAM behavior is validated as behavior-preserving against the edge-case tests added by the individual fixes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- api, backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100