gorums: split the runtime into internal/conn and internal/impl - #333
Open
meling wants to merge 6 commits into
Open
gorums: split the runtime into internal/conn and internal/impl#333meling wants to merge 6 commits into
meling wants to merge 6 commits into
Conversation
Contributor
 |
|
Overall Grade |
Security  Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Aug 12, 2026 12:40p.m. | Review ↗ |
| Shell |  |
Aug 12, 2026 12:40p.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.
meling
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new runtime/gorumsimpl entry-point package for generated code and refactors the Gorums runtime internals into clearer layers (connectivity, call engine, and stream transport). It also updates code generation templates and regenerates affected *_gorums.pb.go files to call through the new runtime entry points, alongside various test/tooling and documentation updates.
Changes:
- Add
runtime/gorumsimpland redirect generated call sites (and version checks) to it. - Split/refactor runtime internals into
internal/conn,internal/impl, and new stream transport helpers, updating tests accordingly. - Update tooling/docs (Makefile targets, go versions, lint settings, package docs) and regenerate generated files.
Reviewed changes
Copilot reviewed 91 out of 101 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server_handler.go | Updates server context helpers, error handling helper naming, and adds request unmarshaling via proto registry. |
| server_handler_test.go | Adjusts test assertion messages for response message construction. |
| runtime/gorumsimpl/version.go | Moves runtime version enforcement into gorumsimpl package namespace. |
| runtime/gorumsimpl/call.go | New runtime entry points used by generated code for call dispatch. |
| options_test.go | Adds tests and updates dial option wiring around new internal/conn dial options. |
| node.go | Removes old root-level node implementation (moved/refactored into new internal connectivity layer). |
| Makefile | Expands targets (lint/deadcode/modernize/goplscheck) and runs tests across workspace modules. |
| local_servers.go | Renames variables/docs to reflect “outbound configuration” and updates peer config construction. |
| internal/testutils/mock/mock.go | Updates mock proto descriptor registration helpers (currently contains compile-breaking changes). |
| internal/tests/unresponsive/unresponsive_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and RemoteCall. |
| internal/tests/tls/tls_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and RemoteCall. |
| internal/tests/tls/cert.go | Adds package doc comment for TLS integration test helpers. |
| internal/tests/ordering/order_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and call constructors. |
| internal/tests/oneway/oneway_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and one-way call constructors. |
| internal/tests/metadata/metadata_test.go | Adds regression test for per-message metadata on server peer configs. |
| internal/tests/metadata/metadata_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and RemoteCall. |
| internal/tests/correctable/correctable_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and quorum call constructors. |
| internal/tests/config/config_gorums.pb.go | Regenerated: uses gorumsimpl for version checks and quorum call constructors. |
| internal/stream/transport.go | Adds Transport abstraction for node send path with swappable channel reference. |
| internal/stream/stream_grpc.pb.go | Regenerated gRPC bindings (tool version bump). |
| internal/stream/doc.go | New package doc for internal stream transport subsystem. |
| internal/strconv/number.go | Adds package doc comment. |
| internal/protoc/protoc_runner.go | Adds package doc comment. |
| internal/impl/unicast.go | Moves unicast call engine into internal/impl and adapts to new call context design. |
| internal/impl/responses.go | Moves response aggregation into internal/impl and routes errors through internal/conn. |
| internal/impl/responses_test.go | Updates tests for new package split and adds new semantics/race coverage. |
| internal/impl/remotecall.go | Moves RemoteCall into internal/impl and dispatches via node transport. |
| internal/impl/quorumcall.go | Moves quorum call constructors into internal/impl. |
| internal/impl/multicast.go | Moves multicast call constructors into internal/impl and uses unified call context. |
| internal/impl/marshaling.go | Removes server-side request unmarshaling from impl package (now in server handler). |
| internal/impl/errors.go | New: defines impl-layer sentinel errors (incomplete/send failure/skip node). |
| internal/impl/doc.go | New package doc for client-side call engine. |
| internal/impl/correctable.go | Moves correctable call logic into internal/impl and aligns error aggregation. |
| internal/impl/call.go | New: defines call handles, interceptors, and oneway dispatch/collection. |
| internal/impl/call_context.go | Updates call context to use transport-based dispatch and shared payload optimization. |
| internal/impl/call_context_test.go | New: validates shared message IDs behavior for sendShared. |
| internal/impl/async.go | Moves async future type into internal/impl with clarified docs. |
| internal/impl/aliases.go | New: aliases conn-layer types into impl layer for readability. |
| internal/conn/testhelpers.go | New: exported helpers to construct nodes for cross-package tests. |
| internal/conn/outbound_manager.go | Refactors dial options and message ID generation to new conn-layer types. |
| internal/conn/node_trysend_test.go | New: regression tests for non-blocking TrySend semantics under stuck transports. |
| internal/conn/node_test.go | Updates node tests to new transport model and adds teardown/zero-value coverage. |
| internal/conn/node_source.go | Refines NodeSource docs and seals interface semantics. |
| internal/conn/errors.go | New: conn-layer QuorumCallError/NodeError plus ErrStopped sentinel. |
| internal/conn/doc.go | New package doc for connectivity layer. |
| internal/conn/dial_opts.go | New: conn-layer dial options struct and defaults (send buffer, backoff, etc.). |
| internal/conn/config.go | New: conn-layer Config/ConfigContext implementation and set operations. |
| gorumstest/options.go | Updates option plumbing to “node source” naming and new NodeSource helpers. |
| gorumstest/gorumstest.go | Updates config construction to use new node-source selection and minor formatting/docs. |
| go.work | Bumps Go workspace version to 1.26.2. |
| go.mod | Bumps Go version and several dependencies (grpc/tools/genproto/etc.). |
| examples/storage/server.go | Adjusts peer dial option application and clarifies nested-call docs. |
| examples/storage/proto/storage.proto | Updates comments to reference ServerContext.PeerConfig(). |
| examples/storage/proto/storage_gorums.pb.go | Regenerated: uses gorumsimpl entry points and version checks. |
| examples/interceptors/server_interceptors.go | Adds package doc and updates metadata interceptor to avoid removed builder alias. |
| examples/go.mod | Bumps Go version and grpc/x/* dependency versions. |
| errors.go | Re-exports errors from new internal packages; QuorumCallError becomes alias to conn-layer type. |
| errors_testing.go | Updates test helper to construct conn-layer QuorumCallError/NodeError values. |
| errors_test.go | Updates tests for conn-layer QuorumCallError and adds transport-sentinel inspection test. |
| doc/migration-v2-to-v3.md | Clarifies status and adjusts terminology (“Configuration” vs “Config”) in examples/text. |
| doc/dev-guide.md | Updates docs for new “Configuration” alias and adds detailed message-ID/transport notes. |
| doc.go | Expands top-level package documentation to describe call types, configs, and server model. |
| dial_options.go | Moves dial option storage to conn-layer and strengthens WithBackChannel validation behavior. |
| config.go | Re-exports config/node types from conn-layer and delegates NewConfig to conn.NewConfig. |
| cmd/protoc-gen-gorums/gengorums/template_unicast.go | Updates generator templates to call gorumsimpl.Unicast. |
| cmd/protoc-gen-gorums/gengorums/template_rpc.go | Updates generator templates to call gorumsimpl.RemoteCall. |
| cmd/protoc-gen-gorums/gengorums/template_quorumcall.go | Updates generator templates to call gorumsimpl.QuorumCall*. |
| cmd/protoc-gen-gorums/gengorums/template_multicast.go | Updates generator templates to call gorumsimpl.Multicast. |
| cmd/protoc-gen-gorums/gengorums/gorums.go | Updates generated version markers to use gorumsimpl EnforceVersion/Min/Max. |
| cmd/protoc-gen-gorums/gengorums/gorums_func_map.go | Adds gorumsimpl import mapping for generator templates. |
| cmd/protoc-gen-gorums/dev/zorums_unicast_gorums.pb.go | Regenerated dev scaffold output to use gorumsimpl entry points. |
| cmd/protoc-gen-gorums/dev/zorums_types_gorums.pb.go | Regenerated dev scaffold output to use gorumsimpl version checks. |
| cmd/protoc-gen-gorums/dev/zorums_server_gorums.pb.go | Regenerated dev scaffold output to use gorumsimpl version checks. |
| cmd/protoc-gen-gorums/dev/zorums_remotecall_gorums.pb.go | Regenerated dev scaffold output to use gorumsimpl.RemoteCall. |
| cmd/protoc-gen-gorums/dev/zorums_quorumcall_gorums.pb.go | Regenerated dev scaffold output to use gorumsimpl.QuorumCall*. |
| cmd/protoc-gen-gorums/dev/zorums_multicast_gorums.pb.go | Regenerated dev scaffold output to use gorumsimpl.Multicast. |
| cmd/protoc-gen-gorums/dev/doc.go | New package doc for generator dev scaffold. |
| call.go | Re-exports call engine types from internal/impl and keeps public API surface stable (currently contains a compile-breaking generic instantiation). |
| call_test.go | Updates tests to call through gorumsimpl entry points. |
| call_quorum_test.go | Updates quorum call tests/benchmarks to call through gorumsimpl. |
| call_correctable_test.go | Updates correctable call tests/benchmarks to call through gorumsimpl. |
| call_client_interceptor_test.go | Updates interceptor tests/benchmarks to call through gorumsimpl. |
| call_async_test.go | Updates async call tests/benchmarks to call through gorumsimpl. |
| buf.yaml | Adds Buf lint configuration exceptions for enums. |
| AGENTS.md | Updates contributor guidance (workspace modules, scoped commits, testing rules, tooling checks). |
| .vscode/settings.json | Updates lint tool/flags and enables staticcheck via gopls config. |
| .vscode/gorums.txt | Updates project word list with new identifiers/terms. |
| .gitignore | Adjusts ignore patterns and adds CLAUDE.md ignore. |
| .claude/agents/scoped-commit-writer.md | Adds scoped commit message helper agent instructions. |
| .claude/agents/conventional-commit-writer.md | Removes conventional-commit writer agent instructions. |
Files not reviewed (7)
- cmd/protoc-gen-gorums/dev/zorums_multicast_gorums.pb.go: Generated file
- cmd/protoc-gen-gorums/dev/zorums_quorumcall_gorums.pb.go: Generated file
- cmd/protoc-gen-gorums/dev/zorums_remotecall_gorums.pb.go: Generated file
- cmd/protoc-gen-gorums/dev/zorums_server_gorums.pb.go: Generated file
- cmd/protoc-gen-gorums/dev/zorums_types_gorums.pb.go: Generated file
- cmd/protoc-gen-gorums/dev/zorums_unicast_gorums.pb.go: Generated file
- cmd/protoc-gen-gorums/gengorums/gorums.go: Generated file
Suppressed comments (2)
internal/testutils/mock/mock.go:83
- These
new(...)calls are invalid Go and will fail to compile.descriptorpb.MethodDescriptorProtofields are*string; useproto.String(...)(as before) instead.
internal/testutils/mock/mock.go:113 new(fmt.Sprintf(...))/new(pkg)is invalid Go and won’t compile. Useproto.String(...)to populate these*stringdescriptor fields.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+36
to
39
| // MapResponse returns an interceptor that transforms each successful response. | ||
| func MapResponse[Req, Resp proto.Message](fn func(Resp, *Node) Resp) ClientInterceptor[Req, Resp] { | ||
| return impl.MapResponse[Req](fn) | ||
| } |
Comment on lines
+103
to
106
| desc, err := protoregistry.GlobalFiles.FindDescriptorByName(protoreflect.FullName(in.GetMethod())) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("gorums: could not find method descriptor for %s", in.GetMethod()) | ||
| } |
Comment on lines
+107
to
+109
| methodDesc := desc.(protoreflect.MethodDescriptor) | ||
|
|
||
| // get the request message type (Input type) |
meling
force-pushed
the
runtime/gorumsimpl
branch
from
August 12, 2026 11:42
fcea12d to
d1bd463
Compare
The root package held both the public API and its whole implementation: connection pooling, node and configuration lifecycle, the call engine, response aggregation, and the interceptor plumbing. A reader looking for what gorums offers had to read past all of it, and generated code called the same exported functions users did, so no boundary said which surface was which. The implementation moves into two internal packages. internal/conn owns connectivity: nodes, the outbound and inbound managers, the configuration and its node source, dial options, and the errors that arise from reaching a peer. internal/impl owns the call engine: the call context, the typed handles, response aggregation, async and correctable results, and marshaling. The per-node transport, which only ever bundled a channel reference, a router, and a message-ID generator, moves down into internal/stream where those live. The root package keeps the public API and is now aliases over the two internal packages, so gorums documents a surface rather than an implementation. runtime/gorumsimpl carries what only generated code needs: the call entry points and the version gate. Generated files import it explicitly, which makes the generated-only surface visible in the import list instead of being mixed into the user-facing package, and lets the root version gate go. Also folds in the toolchain and dependency bumps the tree had accumulated.
Three checks that were being run by hand, or not at all, become make targets. lint runs golangci-lint across every workspace module; deadcode reports functions unreachable from any main or test, across modules so cross-module use is accounted for; goplscheck fails on gopls diagnostics down to hint level in non-generated Go source, which covers simplifications the standalone modernize suite does not. deadcode is advisory: exported library API with no in-repo caller, such as optional dial and server options, is expected to appear. AGENTS.md records when to run them and the naming and documentation conventions they enforce, dev-guide.md documents the new targets, and the editor settings follow the linter version the lint target uses.
Generated output only, produced by make genproto. Generated files now import runtime/gorumsimpl for the call entry points and the version gate.
The target passed --fast-only, which excludes staticcheck, so make lint could report a clean tree while the CI lint job failed on the same commit. Drop the flag so the two agree; errcheck now comes from .golangci.yml rather than a command-line flag on either side.
The doc comment said a node skipped by a request transform is not a failure, but the unicast branch returns the send error as it stands, so a skipped unicast does surface ErrSkipNode. Only the multicast aggregate filters it. Say which of the two paths drops it and why.
The field comments moved into this package still credited WithServer and WithOutbound. Neither exists: the back channel is installed by WithBackChannel, and the symmetric peer set comes from WithPeers.
meling
force-pushed
the
runtime/gorumsimpl
branch
from
August 12, 2026 12:38
d1bd463 to
9638ff6
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.
Splits the implementation out of the root package, so
gorumsdocuments a surface rather than an implementation.The root package held both the public API and all of its machinery: connection pooling, node and configuration lifecycle, the call engine, response aggregation, and the interceptor plumbing. A reader looking for what gorums offers had to read past all of it, and generated code called the same exported functions users did, so nothing marked which surface was which.
internal/connowns connectivity: nodes, the outbound and inbound managers, the configuration and its node source, dial options, and the errors that arise from reaching a peer.internal/implowns the call engine: the call context, the typed handles, response aggregation, async and correctable results, and marshaling.internal/streamgains the per-node transport, which only ever bundled a channel reference, a router, and a message-ID generator — all of which already lived there.runtime/gorumsimplcarries what only generated code needs: the call entry points and the version gate. Generated files import it explicitly, which makes the generated-only surface visible in the import list, and lets the root version gate go.The root package is now aliases over the two internal packages. No behavior changes.
Also adds three checks that were being run by hand or not at all:
make lint(golangci-lint across every workspace module),make deadcode(functions unreachable from any main or test, across modules so cross-module use is accounted for; advisory, since optional dial and server options are expected to appear), andmake goplscheck(gopls diagnostics down to hint level in non-generated Go source, covering simplifications the standalone modernize suite does not). The modernize sweep that makes them pass is included.Verification:
go test ./... -count=2,go test -C examples ./...,go vet -tags=integration ./...,gofmt -l,make goplscheck.