[#105] rpc: replace goridge with Connect-RPC over HTTP/2 (typed handlers)#105
Conversation
Drops the custom net/rpc + goridge codec stack in favor of a connectrpc.com/connect server speaking gRPC, gRPC-Web and the Connect protocol on the same listener (HTTP/1.1 + h2c). - bridge.go reflects on each plugin's RPC() struct and registers a Connect unary handler for every method matching the proto-shaped contract func(*A, *B) error where A, B implement proto.Message. Methods that do not match (resetter / metrics / informer's hybrid shape) are skipped with a warning; migrating them is tracked as a follow-up. - plugin.go now serves an *http.Server with http.Server.Protocols for HTTP/1.1 and unencrypted HTTP/2, plus optional TLS. Init / Serve / Stop / Name / RPCer / Collects signatures are unchanged. - rpc.go's API surface (Config, Version) moves to proto wrapper types (emptypb.Empty, wrapperspb.BytesValue, wrapperspb.StringValue) so the bridge picks them up. - config.go gains optional TLS cert/key and a request_timeout field. - gRPC reflection (grpcreflect v1 + v1alpha) is enabled so grpcurl can list services for operator debugging. - Tests rewritten: PluginRPC.Hello uses wrapperspb.StringValue end to end and Plugin2 calls it via connect.NewClient over HTTP/1.1. This is a breaking wire-protocol change; existing PHP / JS / Python goridge clients will fail until they migrate to a Connect or gRPC client. Listener address (default tcp://127.0.0.1:6001) and DSN parsing are preserved for operator continuity.
…Endure The bridge already iterates plugins by their Endure-assigned name, which is unique by construction, so deduplicating service names from the route list was dead weight. build() now returns the services list directly (filtered to plugins that registered at least one route) and plugin.go feeds it straight into the gRPC reflector. Removes uniqueServices() and the route-string parsing it required.
…ndler) The previous bridge.go reflected on each plugin's RPC() any return value to find proto-shaped methods, then wired them to Connect via a runtime type-erased generic handler with two custom codecs and a request initializer. That whole layer existed only because we kept the goridge- era contract. Inverting the design: change the contract to what Connect natively produces. RPCer.RPC() now returns (string, http.Handler) — the same shape as connect.NewXxxServiceHandler(impl). Each plugin owns its own typed handler; the rpc plugin's job collapses to a mount loop. - Delete bridge.go entirely. - RPCer.RPC() any → RPCer.RPC() (string, http.Handler). - Plugin.Serve() iterates collected plugins and mux.Handle()s each (path, handler) pair. Service tokens for the gRPC reflector come from serviceName(path), which slices between the first two slashes. - Plugin.RPC() delegates to newSelfHandlers in rpc.go. - rpc.go: API struct replaced with newSelfHandlers(cfg, version) — two inline connect.NewUnaryHandler calls under a /rpc/ sub-mux. When buf is set up later, this regenerates from a tiny rpc.proto. - tests/plugin1.go: PluginRPC struct dropped; Plugin1.RPC() returns a typed connect.NewUnaryHandler[wrapperspb.StringValue, ...] directly. Net: -169 lines. No reflection, no custom codecs, no initializers. Other plugins (kv, jobs, status, lock, service, centrifuge, resetter, metrics, informer) currently return any from RPC() and no longer satisfy the new interface; each will migrate in its own PR by adding a service block to its api-go proto and using the generated Connect handler.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRefactors the RPC plugin system from ChangesConnect-RPC Server Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Static reflection only buys grpcurl-list ergonomics; it can't describe methods without FileDescriptors, and clients that use generated Connect stubs don't need it. Dropping it removes the only consumer of the serviceName helper, the services slice, and the connectrpc.com/grpcreflect dependency. The startup log keeps a list of mounted plugin names for operator visibility.
Reverts the previous removal of grpcreflect. Static reflection lets operators run `grpcurl localhost:6001 list` to see which services the rpc plugin currently mounts. The path → service-name extraction is inlined at the only call site (four lines) instead of pulled into a named helper. Adds TestRpcReflection: spins up the plugin via endure with Plugin1, opens a real gRPC client (google.golang.org/grpc + insecure), calls ServerReflectionInfo's ListServices, and asserts both `rpc` and `rpc_test.plugin1` are advertised.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #105 +/- ##
=============================
=============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the rpc plugin from the legacy goridge net/rpc wire protocol to a Connect-RPC (connect-go) HTTP server that can speak Connect, gRPC, and gRPC-Web over a single listener (h2c/HTTP2 + HTTP/1.1), and updates the test plugins and e2e tests accordingly.
Changes:
- Redefines the
RPCercontract to return(string, http.Handler)and mounts typed Connect handlers viahttp.ServeMux(no net/rpc reflection/codecs). - Adds gRPC server reflection handlers (via
connectrpc.com/grpcreflect) and introduces optional TLS +request_timeoutconfig. - Updates tests to use Connect clients and adds a reflection-based test to assert advertised services.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Replaces net/rpc server with http.Server + mux mounting for Connect/gRPC and adds reflection + TLS support. |
rpc.go |
Implements the rpc plugin’s own /rpc/Config and /rpc/Version endpoints as Connect unary handlers under a sub-mux. |
config.go |
Adds request_timeout and optional TLS config, with defaults and validation. |
tests/plugin1.go |
Converts Plugin1 to expose a Connect unary handler via the new (path, handler) RPCer interface. |
tests/plugin2.go |
Converts Plugin2 to call Plugin1 using a Connect client over HTTP. |
tests/rpc_test.go |
Adds a gRPC reflection test and updates imports/deps to validate reflection behavior. |
go.mod |
Drops goridge dependency; adds connect + grpcreflect + protobuf requirements. |
go.sum |
Updates module checksums after dependency changes. |
go.work.sum |
Updates workspace checksums due to dependency graph changes. |
tests/go.mod |
Updates the tests module to use connect + grpc + protobuf. |
tests/go.sum |
Updates test module checksums after dependency changes. |
.github/workflows/linux.yml |
Updates the CI matrix PHP version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if path == "" || handler == nil { | ||
| s.log.Warn("plugin returned empty rpc handler", "plugin", name) | ||
| continue | ||
| } |
| s.server = &http.Server{ | ||
| Handler: mux, | ||
| Protocols: protocols, | ||
| ReadHeaderTimeout: s.cfg.RequestTimeout, |
| conn, err := net.Dial("tcp", "127.0.0.1:6001") | ||
| client := connect.NewClient[wrapperspb.StringValue, wrapperspb.StringValue]( | ||
| http.DefaultClient, | ||
| "http://127.0.0.1:6001/rpc_test.plugin1/Hello", |
| // give the http listener a moment to come up | ||
| time.Sleep(500 * time.Millisecond) | ||
|
|
||
| conn, err := grpc.NewClient("127.0.0.1:6001", grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| require.NoError(t, err) | ||
| defer func() { _ = conn.Close() }() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| stream, err := reflectionpb.NewServerReflectionClient(conn).ServerReflectionInfo(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, stream.Send(&reflectionpb.ServerReflectionRequest{ | ||
| MessageRequest: &reflectionpb.ServerReflectionRequest_ListServices{ListServices: ""}, | ||
| })) | ||
|
|
||
| resp, err := stream.Recv() | ||
| require.NoError(t, err) | ||
|
|
||
| listResp := resp.GetListServicesResponse() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/linux.yml:
- Line 20: The matrix currently includes "php: [ \"8.5\" ]" but no step uses it;
either remove the php entry from the workflow matrix to avoid misleading job
names, or add a step that uses the matrix value (e.g., add a setup-php action
that references matrix.php) so the PHP version is actually tested; locate the
matrix declaration containing "php" and update it accordingly and, if adding PHP
setup, ensure the step references matrix.php (e.g., uses: actions/setup-php or
equivalent).
In `@config.go`:
- Around line 42-50: In Config.Valid(), add a validation that rejects negative
request_timeout values by returning an error when c.RequestTimeout < 0; update
the Valid() function (on type Config) to check the RequestTimeout field and
return a clear error message like "request_timeout must be non-negative" so
configs with negative timeouts fail fast before startup.
In `@plugin.go`:
- Around line 156-161: The TLS setup branch in Serve() opens the socket
(s.listener) before calling tls.LoadX509KeyPair; on failure the listener is left
open—fix by closing s.listener before returning when tls.LoadX509KeyPair returns
an error: in the useTLS branch around tls.LoadX509KeyPair(s.cfg.TLS.Cert,
s.cfg.TLS.Key) call, call s.listener.Close() (or defer a conditional close)
prior to sending the error on errCh and returning so the bound socket is
released; adjust error handling in the Serve function to ensure s.listener is
closed on TLS setup failures.
- Around line 114-120: The loop over s.plugins calls rpcer.RPC() and then
mux.Handle(path, handler) which can panic on duplicate/conflicting patterns; add
validation before calling mux.Handle: keep a local or Server-level set (e.g.,
s.registeredRPCPaths or a local map[string]struct{}) and check the returned path
from rpcer.RPC() for emptiness and for prior registration/conflict, log an error
via s.log (instead of calling Handle) and skip or return an error if a duplicate
is detected; update the logic around the loop that iterates s.plugins so
duplicates are detected and handled safely before calling mux.Handle to avoid
startup panics.
- Around line 150-154: The server currently only sets ReadHeaderTimeout on
s.server which doesn't cap handler execution; wrap your handlers served by mux
with http.TimeoutHandler using s.cfg.RequestTimeout and also set
s.server.ReadTimeout (in addition to or instead of ReadHeaderTimeout) so both
request reading and handler execution are bounded; locate the http.Server
construction (s.server = &http.Server{ Handler: mux, Protocols: protocols,
ReadHeaderTimeout: s.cfg.RequestTimeout }) and replace/augment it to set
ReadTimeout: s.cfg.RequestTimeout and wrap mux routes (or mux itself) with
http.TimeoutHandler(timeout) so each RPC is cancelled after
s.cfg.RequestTimeout.
In `@tests/rpc_test.go`:
- Around line 118-123: Replace the fixed time.Sleep with a readiness poll that
repeatedly attempts a TCP connection to "127.0.0.1:6001" (using net.Dial or
net.DialTimeout) until success or a short deadline (e.g., 5s); on first
successful dial proceed to create the gRPC client with grpc.NewClient, otherwise
fail the test with require.NoError(t, err) after the timeout. Add the net
import, use a small backoff (e.g., 100ms) between attempts, and keep the
existing deferred conn.Close() behavior for the gRPC connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17ab786c-90eb-4283-98e1-1f024af912a1
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/linux.ymlconfig.gogo.modplugin.gorpc.gotests/go.modtests/plugin1.gotests/plugin2.gotests/rpc_test.go
| s.server = &http.Server{ | ||
| Handler: mux, | ||
| Protocols: protocols, | ||
| ReadHeaderTimeout: s.cfg.RequestTimeout, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "plugin.go" -type fRepository: roadrunner-server/rpc
Length of output: 77
🏁 Script executed:
cat -n ./plugin.go | sed -n '140,165p'Repository: roadrunner-server/rpc
Length of output: 825
🏁 Script executed:
rg -i "RequestTimeout" -B 5 -A 2 plugin.go | head -40Repository: roadrunner-server/rpc
Length of output: 234
🏁 Script executed:
rg "type.*Config|struct {" plugin.go -A 20 | head -80Repository: roadrunner-server/rpc
Length of output: 171
🏁 Script executed:
rg "RequestTimeout" -B 10 -A 10 plugin.goRepository: roadrunner-server/rpc
Length of output: 530
🏁 Script executed:
rg "s\.cfg" plugin.go | head -10Repository: roadrunner-server/rpc
Length of output: 352
🏁 Script executed:
rg "type.*Config\s+struct" -A 15Repository: roadrunner-server/rpc
Length of output: 812
🏁 Script executed:
web_search
Go http.Server timeout fields documentation ReadHeaderTimeout vs ReadBodyTimeout vs TimeoutRepository: roadrunner-server/rpc
Length of output: 157
🌐 Web query:
Go http.Server ReadHeaderTimeout vs full request timeout documentation
💡 Result:
In Go’s net/http, the Server timeouts apply to different phases of an HTTP request, and the “full request timeout” is effectively the ReadHeaderTimeout plus the ReadTimeout (because the connection read deadline is set using those two values). 1) What ReadHeaderTimeout does (header read only) - ReadHeaderTimeout is the maximum duration allowed to read the request headers; net/http sets a read deadline for that header-reading window.[1] 2) What “full request timeout” means in practice - ReadTimeout is the maximum duration for reading the entire request, including the body.[2] - In the server implementation, net/http computes: - hdrDeadline = now + ReadHeaderTimeout (if set > 0) - wholeReqDeadline = now + ReadTimeout (if set > 0) - then sets the connection read deadline to hdrDeadline (initially) and later may adjust it to wholeReqDeadline after parsing headers.[1] - A key consequence is: if both ReadHeaderTimeout and ReadTimeout are set, the effective overall deadline for reading the whole request becomes aligned with ReadTimeout (and header time is accounted for via the deadline adjustment logic), so the “full request timeout” is not an independent timer that starts after headers; it’s based on those absolute deadlines within the same request handling window.[3][1] 3) If you’re looking for a separate “handler execution timeout” - net/http does not have a single generic “full request timeout” flag on the Server that only limits handler runtime. Instead: - http.TimeoutHandler limits the duration of the handler (ServeHTTP) and returns 503 if it runs longer than the configured time limit.[4] - ReadTimeout/ReadHeaderTimeout/WriteTimeout limit network I/O and reading/writing phases, not handler CPU/runtime.[2][4] 4) Related nuance: connection idle vs header timeout - ReadHeaderTimeout can also close idle connections if IdleTimeout is not set, because the read deadline that enforces ReadHeaderTimeout can cover elapsed idle time between requests.[5] Summary - Use ReadHeaderTimeout to cap time spent reading headers. - Use ReadTimeout to cap time spent reading the rest of the request body as part of the overall request read window.[2][1] - If you need a timeout for the handler code itself, use http.TimeoutHandler.[4] If you want, share the exact Go version you’re targeting and your timeout values, and I can describe the precise effective deadlines for that configuration based on the server code path.
Citations:
- 1: https://github.com/golang/go/blob/go1.20/src/net/http/server.go
- 2: https://pkg.go.dev/net/http@go1.20.3
- 3: net/http: ReadTimeout is not honored when ReadHeaderTimeout > ReadTimeout golang/go#35626
- 4: https://pkg.go.dev/net/http@go1.22rc1
- 5: net/http: ReadHeaderTimeout starts when previous request ends, not when a new request starts golang/go#54784
🏁 Script executed:
rg "TimeoutHandler" plugin.goRepository: roadrunner-server/rpc
Length of output: 47
🏁 Script executed:
rg "mux\s*:=|ServeMux" plugin.go -A 5 -B 2Repository: roadrunner-server/rpc
Length of output: 339
Use appropriate timeout mechanisms to enforce full request lifetime cap.
The config comment states that RequestTimeout should cap a single RPC request lifetime, but http.Server.ReadHeaderTimeout only limits the header reading phase. Handler execution and request body reading are not bounded by this timeout, allowing slow or hung RPCs to outlive the configured timeout.
To properly enforce a per-request lifetime cap, use http.TimeoutHandler to wrap handlers and set ReadTimeout on the server in addition to ReadHeaderTimeout (or instead of it, depending on the desired behavior). This ensures both the read phase and handler execution are bounded by the timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin.go` around lines 150 - 154, The server currently only sets
ReadHeaderTimeout on s.server which doesn't cap handler execution; wrap your
handlers served by mux with http.TimeoutHandler using s.cfg.RequestTimeout and
also set s.server.ReadTimeout (in addition to or instead of ReadHeaderTimeout)
so both request reading and handler execution are bounded; locate the
http.Server construction (s.server = &http.Server{ Handler: mux, Protocols:
protocols, ReadHeaderTimeout: s.cfg.RequestTimeout }) and replace/augment it to
set ReadTimeout: s.cfg.RequestTimeout and wrap mux routes (or mux itself) with
http.TimeoutHandler(timeout) so each RPC is cancelled after
s.cfg.RequestTimeout.
- plugin.go: skip plugin handlers whose path is missing a leading slash (avoids the http.ServeMux.Handle panic on bad input from a misbehaving plugin). Duplicate-path detection intentionally not added — plugin names are unique by Endure invariant. - plugin.go: close the bound listener if tls.LoadX509KeyPair fails so the port doesn't stay occupied. - plugin.go: set ReadTimeout in addition to ReadHeaderTimeout so RequestTimeout actually caps the request-read phase, not just header reading. - config.go: reject negative request_timeout in Valid(), and rewrite the field doc to describe what RequestTimeout actually bounds (read phases — not handler runtime, not streaming RPCs). - tests/plugin2.go: use the plugin1HelloPath constant instead of a hardcoded URL so server and client paths can't drift. - tests/rpc_test.go: replace the 500ms readiness sleep with a require.Eventually TCP-dial poll to avoid CI flakes on slow runners. Skipped review suggestions: - linux.yml php matrix entry: pre-existing, owned by a separate commit. - Duplicate-path validation: redundant given Endure's name-uniqueness guarantee.
rustatian
changed the title
Summary
grpcurllists registered services.RPCercontract fromRPC() anytoRPC() (string, http.Handler)— exactly whatconnect.NewXxxServiceHandler(impl)returns. Every plugin now owns its typed Connect handler; the rpc plugin'sServecollapses to amux.Handleloop. No reflection, no custom codecs, no runtime initializers./rpc/Configand/rpc/Versionendpoints with twoconnect.NewUnaryHandlercalls under a/rpc/sub-mux (usingemptypb.Empty+wrapperspb.{BytesValue,StringValue}); they regenerate from a small proto once buf is wired up in this repo.tls.cert/tls.key) andrequest_timeoutto the config; DSN format and defaulttcp://127.0.0.1:6001are preserved.Breaking changes
RPCerinterface change breaks every other plugin currently returningany. Each follows up in its own PR by adding aserviceblock to its api-go proto and switchingRPC()toreturn foopbconnect.NewFooServiceHandler(impl). This branch is review-only; do not deploy alongside un-migrated plugins.Summary by CodeRabbit
New Features
Refactor
Tests
Chores