feat: migrate to Connect-RPC; new wire path /jobs.v2.JobsService/#146
feat: migrate to Connect-RPC; new wire path /jobs.v2.JobsService/#146rustatian wants to merge 4 commits into
Conversation
Switches the jobs plugin from net/rpc + goridge codec to Connect-RPC.
Plugin.RPC() now returns the generated jobsV2connect.NewJobsServiceHandler
mounted at /jobs.v2.JobsService/ on the rpc plugin's HTTP/2 mux.
The 8 wire methods are reshaped from (req, *resp) error to the Connect
handler interface:
Push, PushBatch, Pause, Resume, List, Declare, Destroy, GetStats
Notable shape changes:
- Push now takes PushRequest { Job job = 1; }; the legacy runtime guard
for "exactly one job" inside PushBatchRequest is gone (proto enforces).
- Stat -> GetStats (proto rename to avoid collision with the Stat message).
- List, GetStats take google.protobuf.Empty.
- Errors wrap via connect.NewError(connect.CodeInternal, ...) for proper
gRPC status codes; OTEL spans + per-method mutex preserved.
Test helpers swapped to an h2c Connect client (jobsV2connect.JobsServiceClient)
talking to the rpc plugin's HTTP/2 cleartext listener at 127.0.0.1:6001.
Deps:
- api-go v6.0.0-beta.4 -> v6.0.0-beta.5 (new JobsService bindings)
- goridge v4.0.0-beta.1 -> v4.0.0-beta.2 (pkg/rpc removed)
- rpc/v6 v6.0.0-beta.3 -> v6.0.0-beta.4 (Connect-based RPCer interface)
- + connectrpc.com/connect, + golang.org/x/net (h2c client in tests)
Breaking changes:
- Wire protocol changes from goridge codec to Connect/gRPC. Downstream
consumers must use jobsV2connect.NewJobsServiceClient (or any gRPC
client) against the new path. The PHP goridge client migrates
separately on its own timeline.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR migrates the jobs service's RPC surface from goridge/net-rpc to Connect-RPC over HTTP/2. Dependencies are bumped, the RPC handler signature is updated to return an HTTP handler, all eight handler methods are converted to Connect signatures with context support, test helpers are rewritten to use a new Connect client factory, and all tests are updated to invoke the new client methods. ChangesConnect-RPC Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Migrates the jobs plugin RPC layer from legacy net/rpc+goridge to Connect-RPC by exposing the generated jobs.v2.JobsService handler (mounted at /jobs.v2.JobsService/) and updating tests/helpers to use an h2c Connect client against the rpc plugin’s HTTP/2 cleartext listener.
Changes:
- Reworked the 8 wire methods into Connect handler signatures and updated error returns to
connect.Errorwhere applicable (includingStat→GetStats). - Updated the plugin’s exported RPC entrypoint to return
(path, http.Handler)via the generated Connect service handler. - Migrated tests and test helpers from goridge/net/rpc to a Connect h2c client and adjusted request/response shapes (notably
PushRequest).
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
rpc.go |
Converts Jobs RPC methods to Connect handlers; updates request/response types and error mapping. |
plugin.go |
Updates Plugin.RPC() to return the Connect service handler + mount path. |
tests/helpers/helpers.go |
Introduces NewJobsClient (h2c Connect client) and migrates helper RPC calls to Connect. |
tests/jobs_general_test.go |
Migrates integration tests to use the Connect client and updated proto method shapes. |
go.mod / go.sum |
Bumps api-go and goridge versions to match the new Connect surface. |
tests/go.mod / tests/go.sum |
Adds Connect + h2c deps and updates api-go/rpc versions for tests. |
go.work.sum |
Updates workspace sums for newly introduced/updated modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
7-22:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing direct dependency
connectrpc.com/connectto rootgo.mod.
rpc.goimports"connectrpc.com/connect"directly (line 9), but the main module'sgo.moddoes not include it in therequireblock. With Go 1.17+, all directly imported modules must be listed as direct dependencies. Runninggo build ./...orgo mod tidywill fail with a missing module error. The versionv1.19.2is already correctly specified intests/go.modand should be added to the rootgo.mod.🔧 Proposed fix
require ( github.com/prometheus/client_golang v1.23.2 + connectrpc.com/connect v1.19.2 github.com/roadrunner-server/api-go/v6 v6.0.0-beta.5 github.com/roadrunner-server/api-plugins/v6 v6.0.0-beta.2(Run
go mod tidyto place it in the conventional position and refreshgo.sum.)🤖 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 `@go.mod` around lines 7 - 22, The root go.mod is missing the direct dependency connectrpc.com/connect which rpc.go imports; add a require entry for connectrpc.com/connect v1.19.2 to the root go.mod's require block (then run go mod tidy to update go.sum and position the entry correctly) so builds no longer fail when rpc.go imports "connectrpc.com/connect".
🧹 Nitpick comments (1)
rpc.go (1)
60-82: ⚡ Quick win
PushBatchlacks the input validation thatPushperforms.
Pushrejectsniljob and emptyidwithCodeInvalidArgument.PushBatchaccepts whateverreq.Msg.GetJobs()returns: an empty slice silently succeeds, and anilelement or empty-ID element will be passed straight intofrom(in[i])andr.p.PushBatch. That's inconsistent and gives clients confusing internal errors instead of clearInvalidArguments.🛡️ Proposed validation
in := req.Msg.GetJobs() + if len(in) == 0 { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.E(op, errors.Str("jobs batch is empty"))) + } + for i, j := range in { + if j == nil { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.E(op, errors.Errorf("job at index %d is nil", i))) + } + if j.GetId() == "" { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.E(op, errors.Errorf("empty ID at index %d", i))) + } + }🤖 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 `@rpc.go` around lines 60 - 82, PushBatch currently skips the input validation that Push performs; update rpc.PushBatch to validate req.Msg.GetJobs() similarly: if the slice is empty return a connect.NewError(connect.CodeInvalidArgument, errors.E(op, "empty jobs")), and for each element ensure it is non-nil and its GetId() is non-empty (return CodeInvalidArgument with errors.E(op, ...) on the first invalid item); only after validation call from(in[i]) and r.p.PushBatch(spanCtx, batch) as before so clients get clear InvalidArgument errors instead of internal errors from from or r.p.PushBatch.
🤖 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 `@rpc.go`:
- Around line 33-82: Push and PushBatch drop the incoming context (they use
rpcContextFromJob/rpcContextFromJobs which re-root tracing onto background), so
change Push and PushBatch to pass the inbound ctx into
rpcContextFromJob/rpcContextFromJobs (so the call to r.p.Push and r.p.PushBatch
inherit cancellation/deadline), update the helper signatures rpcContextFromJob,
rpcContextFromJobs, and rpcContextFromHeaders to accept a context parameter, and
update the five test calls in rpc_test.go to pass context.Background() or the
test context to those helpers so tests compile.
---
Outside diff comments:
In `@go.mod`:
- Around line 7-22: The root go.mod is missing the direct dependency
connectrpc.com/connect which rpc.go imports; add a require entry for
connectrpc.com/connect v1.19.2 to the root go.mod's require block (then run go
mod tidy to update go.sum and position the entry correctly) so builds no longer
fail when rpc.go imports "connectrpc.com/connect".
---
Nitpick comments:
In `@rpc.go`:
- Around line 60-82: PushBatch currently skips the input validation that Push
performs; update rpc.PushBatch to validate req.Msg.GetJobs() similarly: if the
slice is empty return a connect.NewError(connect.CodeInvalidArgument,
errors.E(op, "empty jobs")), and for each element ensure it is non-nil and its
GetId() is non-empty (return CodeInvalidArgument with errors.E(op, ...) on the
first invalid item); only after validation call from(in[i]) and
r.p.PushBatch(spanCtx, batch) as before so clients get clear InvalidArgument
errors instead of internal errors from from or r.p.PushBatch.
🪄 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: de060013-3fc0-44a4-8fc0-3432291e4a5b
⛔ 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 (6)
go.modplugin.gorpc.gotests/go.modtests/helpers/helpers.gotests/jobs_general_test.go
- Push/PushBatch: thread inbound handler ctx through rpcContextFromJob/ rpcContextFromJobs/rpcContextFromHeaders so client cancellation and deadlines propagate. The job-header trace context still overrides the span parent (PHP-side propagation preserved); only the propagator's parent argument changes. - Pause/Resume: wrap errors with errors.E(op, err) for consistency with other methods (rpc_pause, rpc_resume ops). - Tests (helpers): t.Cleanup(httpc.CloseIdleConnections) on each NewJobsClient to avoid leaking idle HTTP/2 connections across tests. - rpc_test.go: pass t.Context() to the 5 helper call sites for the new signature.
- Destroy: wrap errg.Wait() error with errors.E(op, err) for parity with the other 7 methods. - Push: add unit tests covering nil-job and empty-ID branches (the proto-shape replacement for the legacy len(req.Batch) != 1 guard). - Trim WHAT-restating docstrings on Plugin.RPC, rpc.from, rpc.Push, rpc.Declare, helpers.NewJobsClient. Tighten the rpcContextFromJobs doc to the single non-obvious WHY (parent-ctx layering).
|
Superseded by #147 — content merged into the priority-fix branch which is now the consolidated migration PR. |
Summary
Switches the jobs plugin's RPC layer from net/rpc + goridge codec to Connect-RPC.
Plugin.RPC()now returns the generatedjobsV2connect.NewJobsServiceHandler, which the rpc plugin (≥ v6.0.0-beta.4) mounts at/jobs.v2.JobsService/on its HTTP/2 mux.The 8 wire methods are reshaped from net/rpc's
(req, *resp) errorto the Connect handler interface ((ctx, *connect.Request[T]) (*connect.Response[U], error)):PushPushRequest { Job job = 1; }— the runtime "exactly one job" guard insidePushBatchRequestis gone (proto enforces)PushBatchPause,ResumePipelines, returnJobsHandlerResponse{}Listgoogle.protobuf.Empty, returnsPipelinesDeclareDeclareRequestDestroyPipelines, returnsPipelines(destroyed names)Stat→GetStatsStatmessage; takesEmpty, returnsStatsOTEL spans + per-method mutex preserved. Errors wrap via
connect.NewError(connect.CodeInternal, ...)for proper gRPC status codes. Push-side trace-context extraction from job headers (rpcContextFromJob) preserved for cross-process propagation from PHP workers.Test helpers swapped to an h2c Connect client (
jobsV2connect.JobsServiceClient) talking to the rpc plugin's HTTP/2 cleartext listener at127.0.0.1:6001. The new exportedhelpers.NewJobsClient(t, address)is available for sibling test packages.Deps:
api-go v6.0.0-beta.4 → v6.0.0-beta.5(newJobsServicebindings)goridge v4.0.0-beta.1 → v4.0.0-beta.2(pkg/rpcremoved)rpc/v6 v6.0.0-beta.3 → v6.0.0-beta.4(Connect-basedRPCerinterface)+ connectrpc.com/connect,+ golang.org/x/net(h2c transport for tests)Breaking changes
127.0.0.1:6001) to Connect/gRPC over HTTP/2 at/jobs.v2.JobsService/<Method>. Downstream Go consumers must usejobsV2connect.NewJobsServiceClient(or any gRPC client). The PHP goridge client migrates separately on its own timeline.Pushrequest type changed fromPushBatchRequest(1-element) toPushRequest { Job }.StatRPC method renamed toGetStats.Summary by CodeRabbit
Chores
Refactor