Skip to content

client: preserve headers on malformed end streams - #201

Merged
iainmcgin merged 2 commits into
connectrpc:mainfrom
fallintoplace:fix-server-stream-end-stream-headers
Jun 28, 2026
Merged

iainmcgin merged 2 commits into
connectrpc:mainfrom
fallintoplace:fix-server-stream-end-stream-headers

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

Preserve response headers when a server-streaming Connect response ends with malformed END_STREAM JSON.

Why

In #192, the client-streaming parse-error path started attaching response headers to malformed END_STREAM errors, but the server-streaming path still dropped them. The maintainer review on that PR called out the asymmetry as a good follow-up to make the two paths uniform.

What changed

  • attach self.headers before converting a malformed Connect END_STREAM parse error into the terminal StreamEnd
  • extend the existing server-stream malformed-END_STREAM regression to assert the headers are preserved on the first error and on the sticky replay

Validation

  • cargo test -p connectrpc malformed_end_stream_json
  • cargo test -p connectrpc

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

iainmcgin
iainmcgin previously approved these changes Jun 28, 2026

@iainmcgin iainmcgin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[claude code] Reviewed with two parallel review passes (correctness + API ergonomics), both verified against the worktree.

The fix is exactly right: it mirrors the client-streaming path from #192 (set_response_headers(self.headers.clone()) at the parse site before the error becomes the terminal StreamEnd), and the sticky-replay assertion in the test is structurally guaranteed — StreamEnd::replay clones the stored error and ConnectError's derived Clone carries the boxed header map. Nice touch using a sentinel header value rather than asserting non-emptiness; that pins provenance, not just presence.

I pushed one small commit to the branch: a CHANGELOG amendment folding this into the existing #192 entry (the repo keeps changelog discipline for behavior changes, and this one is consumer-visible).

Both reviewers flagged remaining header-attachment gaps in the same function — the carried END_STREAM error (end_stream_error_to_connect_error at the end_stream.error site attaches neither headers nor trailers, while the client-stream equivalent attaches both) and the decompression-failure arm (drops headers symmetrically on both stream shapes). Those are pre-existing and out of this PR's scope; tracked in a follow-up issue. Approving.

@iainmcgin
iainmcgin force-pushed the fix-server-stream-end-stream-headers branch from 9ef31ab to 8b60861 Compare June 28, 2026 21:01
@iainmcgin
iainmcgin added this pull request to the merge queue Jun 28, 2026
Merged via the queue into connectrpc:main with commit 82fe389 Jun 28, 2026
13 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants