client: enforce the streaming terminal contract by construction#160
Merged
Conversation
Replace the done/error/trailers field triple on ServerStream with one end: Option<StreamEnd> record written exactly once. The decode loop now returns Result<message, StreamEnd>, so ending the stream without recording the outcome is unrepresentable; classify_grpc_end is the single site deciding clean-vs-failed for gRPC/gRPC-Web ends, and process_end_stream classifies Connect ends as a pure function. The whole-call deadline moves to the body frame poll, which is observably equivalent (all inter-poll work is non-yielding) and lets every terminal cause exit the loop as a StreamEnd. No public-API or behavior change intended; streaming contract test assertions are unchanged.
…king Review hardenings on the restructure: debug_assert that the terminal record is written once, and state the third fact the per-frame-poll deadline equivalence rests on (timeout_at polls the inner future before the timer). Co-Authored-By: Claude <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
- Fix the gRPC-Web EOF-arm comment to state the truth: the residual trailer-frame parse is provably dead (the loop-top completeness check consumes any complete frame before poll_body runs, and EOF appends nothing); it is preserved verbatim for pure-structure fidelity with removal as a follow-up - Pin the early-fire direction of the deadline test (elapsed >= the deadline) and run one success-path test under an unexpired deadline - get_or_insert over insert so first-write-wins matches the written-exactly-once claim even if the guard were ever violated - Rename message_inner -> next_message_or_end to telegraph the Err-means-stream-ended inversion at call sites Co-Authored-By: Claude <noreply@anthropic.com>
iainmcgin
approved these changes
Jun 10, 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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Follow-up to #159: restructure the client streaming receive path so the contract #159 introduced —
Ok(None)⇔ the RPC succeeded, everyErrterminal and sticky,error()/trailers()always consistent withmessage()— is enforced by construction rather than convention. No public-API or intended behavior change; #159's eight streaming-contract tests pass with their assertions unchanged (only the private struct-literal initializers in tests were touched).What changes structurally:
done/error/trailersfield triple becomes one privateStreamEndrecord (outcome: Result<(), ConnectError>+ trailers), written exactly once bymessage()and read by the sticky replay,error(), andtrailers()— one fact, three readers, so they cannot disagree.message_inner's error type is the terminal record: "ended the stream without recording why" is now unrepresentable. The cross-function invariants client: streaming message() returns terminal errors as Err #159 documented anddebug_assert!ed ("everyOk(None)setsdonefirst", "errorimpliesdone") are deleted because the types carry them.poll_bodybecomes a pure transport reader returningData | Trailers(HeaderMap) | Eofas values — no moreOk(false)-means-two-things, no terminal-state writes from the transport layer.internal/unknownsplit, the deadline relabel) consolidates into oneclassify_grpc_endfunction, sitting next to the trailer parse it depends on.timeout_atpolls the inner future before the timer. Pinned by a new paused-clock test (deadline_bounds_multi_frame_message): a server trickling frames every 40ms against a 100ms deadline — a per-poll relative timeout would let every frame through and fail the test.Validation
connectrpclib tests (incl. client: streaming message() returns terminal errors as Err #159's contract tests unchanged), full workspace green, clippy/fmt/docs clean.Notes for reviewers
Debugimpl renamesdone→ended. (An earlier draft of this description claimed a second delta around gRPC-Web double-trailer handling; it is not real — the path it described is provably unreachable in both the old and new shapes.)poll_bodyruns, and EOF appends nothing) — the in-code comment now states this; removing the path belongs in a separate PR, not a pure-structure one.Post-review nits addressed (7d1bc7a)
Truthful EOF-arm comment; deadline test now pins the early-fire direction too (
elapsed >= deadlineunder paused time) and one success-path test runs under an unexpired deadline;get_or_insertso first-write-wins matches the written-exactly-once claim;message_innerrenamed tonext_message_or_endto telegraph the Err-means-stream-ended inversion.🤖 Generated with Claude Code