Repository navigation
gorums: harden message unmarshaling and address stream review findings - #340
Merged
meling merged 5 commits intoOct 7, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation consistently propagates errors, prevents unsafe descriptor assertions, and includes focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Hardens protobuf unmarshaling against invalid method descriptors, propagates peer configuration failures safely, and clarifies stream retry behavior.
Changes:
- Validates method descriptors and preserves registry errors.
- Returns inbound peer configuration errors instead of panicking.
- Documents pending-call cleanup and bounded requeue behavior.
| File | Description |
|---|---|
server.go |
Handles inbound-manager errors and uses shared unmarshaling. |
server_handler.go |
Removes duplicated request unmarshaling. |
local_servers_test.go |
Tests invalid peer error propagation. |
internal/stream/session.go |
Clarifies session and retry behavior. |
internal/stream/pending.go |
Documents expired-call removal. |
internal/stream/outbound.go |
Documents reconnect and requeue semantics. |
internal/stream/channel.go |
Clarifies pending-count behavior. |
internal/impl/marshaling.go |
Centralizes safe request/response unmarshaling. |
internal/impl/marshaling_test.go |
Tests valid, missing, and non-method descriptors. |
internal/conn/inbound_manager.go |
Returns peer configuration errors. |
internal/conn/inbound_manager_test.go |
Updates constructor and error-path tests. |
doc/dev-guide.md |
Documents send-queue-bounded requeueing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
meling
force-pushed
the
fix/message-helpers-and-review
branch
2 times, most recently
from
October 3, 2026 22:49
e93b8c7 to
b9be560
Compare
meling
force-pushed
the
fix/message-helpers-and-review
branch
from
October 4, 2026 09:44
b9be560 to
9b32628
Compare
meling
force-pushed
the
fix/message-helpers-and-review
branch
from
October 4, 2026 14:42
9b32628 to
8c99e28
Compare
UnmarshalRequest and unmarshalResponse look up the method descriptor the same way, so they belong in one file. The function moves unchanged; the server calls it through internal/impl.
Request and response unmarshaling resolve the message type from the method name carried on the wire, and asserted that the registry lookup returned a method descriptor. A name that resolves to a message, enum, service, or field made the assertion panic. A response reaches this path before any handler lookup, so a peer could crash a caller with one frame. Both directions now share one helper that checks the assertion and wraps registry errors with %w, so callers can match protoregistry.NotFound.
NewInboundManager panicked when its NodeSource rejected an address or a duplicate node. newServer builds the inbound manager before the peer configuration it already checks, so NewLocalServers panicked on such peers instead of returning the error it documents. NewInboundManager now returns the error, and newServer passes it on; NewServer still panics.
Calls pending on a failed stream are pushed onto the send queue without waiting, so once it is full the rest fail with ErrSendQueueFull. The OutboundChannel and retry docs promised that every non-streaming call is sent again; they now state the bound and list each case in which retry fails a call. The session docs also explain how a session relates to the gRPC stream it runs on and to the channel's other sessions.
A pending call whose caller is done is removed by a sweep when a later call grows the table, or by its own context watch once the stream starts draining. After a burst, calls that expired since the last sweep keep their memory and count in PendingCount until a later sweep or the end of the stream. The docs now say so.
meling
force-pushed
the
fix/message-helpers-and-review
branch
from
October 7, 2026 18:16
8c99e28 to
8365870
Compare
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.
internal/impl: move request unmarshaling next to response unmarshaling, and reject method names that do not name a method. A peer's reply carrying a message, enum, service, or field name made the caller panic on an unchecked type assertion; registry errors are now wrapped with%w.internal/conn:NewInboundManagerreturns invalid peer configuration errors, soNewLocalServersreports them instead of panicking.stream: document that requeued calls are bounded by the send queue, and when expired pending calls are removed.Stack created with GitHub Stacks CLI • Give Feedback 💬