gorums: rework peer configuration around WithPeers - #330
Open
meling wants to merge 10 commits into
Open
Conversation
meling
changed the title
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Aug 12, 2026 12:38p.m. | Review ↗ |
| Shell | |
Aug 12, 2026 12:38p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Gorums peer configuration to be server-owned (via WithPeers) and updates the public APIs and internals to better model reachability, back-channel replies, and stream churn, while also removing the legacy benchmark toolchain from the repository.
Changes:
- Replace the
WithConfig/WithOutboundNodesmodel withWithPeers, and rename system/server accessors toPeerConfig,ConnectedPeers,ConnectedClients,WaitForPeers, andWaitForClients. - Rework stream/channel behavior to avoid receive-loop deadlocks (non-blocking reply path via
TrySend), add eager reconnect for symmetric peers, and scope pending-call ownership to prevent cross-channel interference. - Remove benchmark binaries/scripts/docs and update build/dev documentation accordingly.
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| system.go | Updates System to expose server-owned peer config and renames config/ অপেক্ষ methods; introduces NewLocalSystems option split. |
| system_test.go | Updates tests to use the new WaitForPeers / ConnectedPeers / WithBackChannel APIs. |
| server.go | Introduces WithPeers + server-owned outbound peer config; updates buffer semantics and stop cleanup. |
| opts.go | Renames back-channel dial option to WithBackChannel, sets default send-queue capacity, removes WithOutboundNodes. |
| opts_test.go | Adjusts option tests to match NewLocalSystems API changes. |
| node.go | Adds node stream-state hooks and non-blocking TrySend support; adds isUp helper. |
| node_test.go | Updates outbound channel constructor signature usage. |
| mgr.go | Enables eager reconnect and stream-state reporting for server-owned peer nodes. |
| inbound_manager.go | Splits known/client node tracking; adds connected-peer derivation from peer config and stream state. |
| inbound_manager_test.go | Updates tests for inbound-vs-connected views and new back-channel option name. |
| handler.go | Renames handler-facing configuration accessors (PeerConfig, ConnectedPeers, ConnectedClients). |
| errors.go | Re-exports new stream-level availability/queue errors; updates stop/wait documentation. |
| gorumstest/gorumstest.go | Updates helper to construct local systems using the new local option builders. |
| callopts_test.go | Updates local-system creation and wait calls to renamed APIs. |
| examples/storage/server.go | Migrates example to WithPeers and WaitForPeers; uses WithBackChannel for client back-channel. |
| internal/stream/server.go | Switches server drain path to PeerNode.TrySend to prevent receive-loop wedging. |
| internal/stream/channel.go | Adds non-blocking reply path, eager reconnect, stream health tracking, and pending-call ownership. |
| internal/stream/channel_test.go | Adds/updates extensive tests for queue semantics, churn, eager reconnect, and health reporting. |
| internal/stream/router.go | Adds pending ownership tracking and serialized dispatch for server-initiated messages. |
| internal/stream/router_test.go | Adds coverage for ownership requeue, metadata propagation, and dispatch ordering. |
| internal/stream/gorums_message.go | Adds NewMessageFromPayload helper for reuse of marshaled payload bytes. |
| internal/stream/gorums_message_test.go | Tests payload+metadata preservation for message constructors. |
| internal/stream/testhelpers.go | Adds exported constructors intended to support tests in other packages. |
| internal/stream/teardown_deadlock_test.go | Adds regression tests reproducing and guarding against teardown deadlocks. |
| Makefile | Removes benchmark build target; updates all/genproto messaging; keeps go test benchmark targets. |
| doc/user-guide.md | Updates documentation for new peer/back-channel APIs and queue semantics. |
| doc/dev-guide.md | Removes benchmark section and updates Makefile target descriptions. |
| doc/benchmarking.md | Removes benchmark documentation. |
| scripts/killall.yml | Removes obsolete benchmark ansible script. |
| scripts/deploy.yml | Removes obsolete benchmark deployment ansible script. |
| scripts/benchmark.yml | Removes obsolete benchmark ansible script. |
| scripts/benchmark.sh | Removes obsolete benchmark runner script. |
| cmd/benchmark/main.go | Removes benchmark CLI tool. |
| cmd/benchmark/profiling.go | Removes benchmark profiling helpers. |
| benchmark/benchmark.proto | Removes benchmark proto definitions. |
| benchmark/benchmark.pb.go | Removes generated benchmark protobuf code. |
| benchmark/benchmark_gorums.pb.go | Removes generated benchmark Gorums code. |
| benchmark/benchmark.go | Removes benchmark runner library. |
| benchmark/server.go | Removes benchmark server implementation. |
| benchmark/stats.go | Removes benchmark stats implementation. |
| benchmark/qspec.go | Removes benchmark quorum function. |
| AGENTS.md | Updates repository structure/docs after benchmark removal. |
| .gitignore | Removes ignored benchmark binary path. |
| go.mod | Moves golang.org/x/sync to indirect after benchmark removal. |
| go.sum | Updates sums consistent with dependency changes. |
Suppressed comments (2)
server.go:105
- Doc comment says the callback must not call [Server.Config], but Config does not exist after the rename. It should warn against calling [Server.ConnectedPeers] (or other methods that acquire the same internal locks).
doc/user-guide.md:1907 - The paragraph below still references
gorums.WithServer, but the option was renamed togorums.WithBackChannelin this PR. Leaving the old name makes the docs inconsistent and harder to follow.
Passing `clientSrv` to `gorums.WithServer` is what installs the server as the back-channel request handler.
When the remote server dispatches a reverse-direction call via `ctx.ConnectedClients()`, the call arrives on the same gRPC stream the client opened and is routed to `clientSrv` for dispatch.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
14
to
16
| // PeerNode represents a peer from the perspective of stream dispatch. | ||
| // It is implemented by Node and nilPeerNode in the gorums package. | ||
| // It is implemented by peerNode and nilPeerNode in the gorums package. | ||
| type PeerNode interface { |
Comment on lines
102
to
106
| // DispatchLocalRequest handles the request in-process for the local node, | ||
| // bypassing the network. It delivers the request to the registered handler, | ||
| // serializing execution the same way remote nodes do: the next dispatch is | ||
| // blocked until the handler returns or calls [ServerCtx.Release]. | ||
| // blocked until the handler returns or calls [ServerContext.Release]. | ||
| // |
Comment on lines
1898
to
1903
| // Connect to the server; WithServer wires up the back-channel dispatcher automatically. | ||
| config, err := gorums.NewConfig( | ||
| gorums.WithNodeList(serverAddrs), | ||
| gorums.WithServer(clientSrv), | ||
| gorums.WithBackChannel(clientSrv), | ||
| gorums.WithDialOptions(grpc.WithTransportCredentials(insecure.NewCredentials())), | ||
| ) |
meling
force-pushed
the
gorums/peer-configuration
branch
from
August 12, 2026 11:42
53ca6ac to
5416bf0
Compare
A symmetric peer group exposed several ways a node channel could lose a call without reporting it. This reworks the channel and its router together, since the fixes share state. Pending calls are now tagged with the channel that sent them. Closing or requeueing a retired channel therefore affects only its own calls, never those of the replacement channel that shares the node's router. A stream is no longer severed by a cancellation that arrives after the send it was meant to unblock has already completed. The sender records send completion under a guard the cancel watcher takes, so a late watcher becomes a no-op instead of clearing a healthy stream that later calls depend on. An outbound channel can now re-establish a lost stream from its receiver, with capped exponential backoff, rather than waiting for the next local send. A symmetric peer depends on the stream this side dialed staying registered on its inbound side, so a stream lost while this side has nothing to send would otherwise leave the peer stalled indefinitely. The send queue now has a real default capacity. Zero capacity is not viable under fail-fast semantics, because every two-way request enqueued while the sender is busy would fail, and a reply dispatched from a receive loop would be dropped whenever the sender goroutine was not already waiting. Both WithSendBufferSize and the send half of WithBufferSizes now treat 0 as the default. Replies sent from a receive or dispatch loop go through a non-blocking path: if they blocked on a full queue the handler would never return, the dispatch lock would never release, and the loop would stop reading inbound frames. Replies that cannot be enqueued are counted rather than lost silently. The channel also tracks whether its stream is up, reports it through a callback on every transition, and records the outcome of operations that move data as its last error, so a caller can tell an unreachable node from an idle one.
The benchmark package, its cmd/benchmark binary, the iago deploy scripts, and doc/benchmarking.md are removed. A later PR in this stack reintroduces all of them under a separate benchkit module, rewritten against the measurement harness rather than hand-rolled statistics. Removing them now rather than at that point keeps the API changes in the PRs that follow honest. The benchmark package is a consumer of the gorums API, so every renaming PR would otherwise have to rewrite it, three times over, only for the result to be deleted. The scripts and the documentation go with the package: keeping either would leave the repository describing a binary it can no longer build. Root go.mod drops golang.org/x/sync, which only the benchmark package used.
The inbound manager kept configured peers and dynamically accepted clients in one map, telling them apart by comparing the node ID against clientIDStart. They are now two maps, so the distinction is structural rather than numeric. That removes a real constraint: a configured peer could not use an ID at or above clientIDStart without being mistaken for a client. The dynamic-ID allocator now skips every occupied ID instead of assuming the range is its own, and its counter widens to uint64 so exhaustion is representable rather than detected one ID early.
Configuring a symmetric server took three options that had to agree with each other: WithConfig named the peers to track, WithOutboundNodes named the peers to call, and WithServerOptions carried the first into NewSystem while the second was consumed by NewSystem itself. Nothing checked that the two node sources matched, and only NewSystem could act on the second. WithPeers replaces both. It records one node source for both roles and takes the dial options for the connections the server establishes, and NewServer builds the peer configuration from it, available as Server.PeerConfig. A server therefore has its peers whether it was created by NewServer, NewSystem, or NewLocalSystems, and Server.Stop closes the configuration it built. WithPeerChange replaces the variadic onChange parameter of WithConfig, which read as optional but was a distinct concern from naming the peer set. WithServer becomes WithBackChannel and is now only for a client that must accept calls from the servers it dials; a server no longer needs it, because WithPeers installs the back channel on the configuration it builds. NewServer now panics on an invalid peer node source, which it can do because it resolves the source at construction. The panic matches the existing contract for configuration errors detectable at startup.
Server.Config reported which peers had opened a stream to this server. That is the wrong direction to ask about: a call goes out over the connection this server established, so a peer visible in the inbound view could still be unreachable for calls, and a peer this server had successfully dialed could be missing from it. The connected-peer configuration is now the subset of the server's own peer configuration whose nodes have a live stream, plus the local node, which is always reachable in-process. Outbound peer nodes report stream transitions to the inbound manager, which rebuilds the view on each one. A server without WithPeers has no peer configuration, so it keeps reporting the inbound view. The accessors are renamed to match what they now answer: Config becomes ConnectedPeers and ClientConfig becomes ConnectedClients, on the server, on the system, and on the handler context, with WaitForConfig and WaitForClientConfig becoming WaitForPeers and WaitForClients. ServerCtx gains PeerConfig, the full peer set, so a quorum size derived inside a handler does not shift as peers connect and disconnect. It replaces ConfigContext and ClientConfigContext, which only wrapped an accessor in a context and hid whether the configuration was empty. A handler dispatching a nested call to PeerConfig must release the server first, since that configuration includes the local node and its in-process dispatch waits on the handler's dispatch lock. The nested-call tests and the storage example do so.
NewLocalSystems took DialOptions and reached inside them for server options via WithServerOptions, so a caller had to know that one option type smuggled another, and dial options meant for the peer connections were indistinguishable from those meant for nothing at all. LocalServerOption separates the two: WithLocalServerOptions carries server options and WithLocalDialOptions carries the dial options applied to every server's peer configuration. WithServerOptions stays, since NewSystem still needs a way to pass server options through a dial-option list. gorumstest.Systems takes ServerOptions to match, which the tests that need a per-server option can now use directly.
Covers the option and accessor changes: WithPeers replacing WithConfig and WithOutboundNodes, WithPeerChange replacing the variadic onChange argument, WithBackChannel replacing WithServer, ConnectedPeers and ConnectedClients replacing Config and ClientConfig, and WaitForPeers and WaitForClients replacing WaitForConfig and WaitForClientConfig. Records what the connected-peer configuration now means, that PeerConfig is the full peer set a handler should derive a quorum size from, and that a symmetric server re-establishes an idle outbound stream on its own. Adds a section on send queue capacity, documenting the default capacity and which callers fail fast on a full queue rather than waiting.
The five grpc.ServerStream filler methods on fakeNodeStream never touch their receiver, so name it away. Also record why echoPeerNode.RouteInbound starts its own goroutine: keeping the receive loop unblocked is the property the test exists to check, so the goroutine cannot move to the caller.
Three references named identifiers this PR replaced. Server has no Config method, so [Server.Config] rendered as a dead link in the WithPeers and WithPeerChange comments; ConnectedPeers is the reachable-subset accessor they mean, promoted onto Server from the embedded inbound manager. The dialOptions field comment and the user guide both still credited WithServer, which is now WithBackChannel.
Three comments named things that do not exist. PeerNode is implemented by gorums.Node and nilPeerNode, not by a peerNode type; the symmetric peer set comes from WithPeers, not a WithOutbound option; and [ServerContext.Release] cannot resolve from this package, so describe the release callback the dispatch passes in instead.
meling
force-pushed
the
gorums/peer-configuration
branch
from
August 12, 2026 12:38
5416bf0 to
3bb77c0
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.
Reworks how a symmetric server is configured, and fixes several ways a node channel could lose a call without reporting it.
WithPeersreplaces three options that had to agree.WithConfignamed the peers to track,WithOutboundNodesnamed the peers to call, andWithServerOptionscarried the first intoNewSystemwhile the second was consumed byNewSystemitself. Nothing checked that the two node sources matched.WithPeersrecords one node source for both roles, andNewServerbuilds the peer configuration itself, available asServer.PeerConfig.WithPeerChangetakes over the variadiconChangeparameter, andWithServerbecomesWithBackChannel, now only for a client that must accept calls from the servers it dials.ConnectedPeersnow means what it says.Server.Configreported which peers had opened a stream to this server — the wrong direction, since a call goes out over the connection this server established. It is now the subset of the server's own peer configuration whose nodes have a live stream, plus the local node.Config/ClientConfigbecomeConnectedPeers/ConnectedClientsaccordingly.Transport reliability. Pending calls are tagged with the channel that sent them, so retiring a channel no longer disturbs its replacement's calls. A cancellation arriving after the send it was meant to unblock no longer severs a healthy stream. An outbound channel re-establishes a lost stream from its receiver with capped backoff, rather than waiting for a local send that may never come. The send queue gains a real default capacity — zero is not viable under fail-fast semantics — and replies dispatched from a receive loop take a non-blocking path, since a blocked reply would stop the loop reading.
Note: the first commit retires the old
benchmarkpackage,cmd/benchmark, the iago deploy scripts, anddoc/benchmarking.md. They are superseded bybenchkit, which arrives later in this stack. Carrying that package through the renaming PRs would have meant rewriting a consumer three times for a result that is then deleted.Verification:
go test ./... -count=2,go test -C examples ./...,go vet -tags=integration ./...,gofmt -l.