feat: migrate to Connect-RPC; new wire path /lock.v1.LockService/#77
Merged
Conversation
Plugin now exposes the generated lockV1connect.LockService handler instead of the legacy net/rpc + goridge codec methods. RPC() returns (path, http.Handler) so the rpc plugin's HTTP/2 mux mounts it directly at /lock.v1.LockService/. - plugin.go: RPC() any -> RPC() (string, http.Handler) - rpc.go: 6 methods adapted to connect.Request/Response shapes; caller's ctx now bounds the locker acquire (was Background); empty ID returns CodeInvalidArgument - tests: shared http2 transport via sync.OnceValue pools idle conns across the 1700-goroutine TestLockInit; address arg removed (always 127.0.0.1:6001) - deps: api-go v6.0.0-beta.5, connect v1.19.2, goridge dropped - lint: dead dupl settings block removed
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigrates server RPCs and test client from legacy net/rpc to Connect-RPC (HTTP/2 h2c). Handlers now use typed connect.Request/Response with context and bounded wait timeouts; plugin mounting, locker pointer usage, configs, and tests updated accordingly. ChangesConnect-RPC Handler and Client Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler
participant RPCImpl
participant Locks
participant Response
Client->>HTTPHandler: Connect-RPC request (Lock/Release/Exists/UpdateTTL)
HTTPHandler->>RPCImpl: *connect.Request[LockRequest] with ctx
RPCImpl->>RPCImpl: validate id and derive waitContext
RPCImpl->>Locks: call pl.locks.* operation
Locks-->>RPCImpl: boolean ok
RPCImpl->>Response: connect.NewResponse(LockResponse{Ok: ok})
Response-->>Client: Connect-RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
Go 1.26's new() now accepts expressions, so new(int64(ttl)) returns *int64 directly. Drops the ptrTo[T any] generic helper from tests/rpc.go since it has no remaining callers.
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the lock plugin’s RPC surface from the legacy goridge net/rpc codec to Connect-RPC over an HTTP/2 mux, aligning the plugin with the generated lock.v1.LockService handler and new wire path conventions.
Changes:
- Replaces
Plugin.RPC() anywithRPC() (string, http.Handler)and returns the generated Connect handler + mount path. - Adapts lock RPC methods to Connect handler signatures, returning
connect.CodeInvalidArgumentfor empty IDs and propagating caller context into lock acquisition (with a sharedwaitContexthelper). - Updates tests to use a shared h2c
http2.Transport+ Connect client, and refreshes module dependencies/lint config accordingly.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Exposes the generated Connect-RPC handler (path + http.Handler) from the lock plugin. |
rpc.go |
Converts RPC implementation to Connect request/response types, adds waitContext, and improves invalid-argument handling. |
tests/rpc.go |
Replaces net/rpc test helpers with Connect client helpers using a shared h2c HTTP/2 transport. |
tests/lock_test.go |
Updates tests to use the new helper function signatures (no address param) and minor naming cleanup. |
go.mod |
Adds Connect-RPC deps and updates api-go to v6.0.0-beta.5. |
go.sum |
Updates dependency checksums for new/updated modules. |
tests/go.mod |
Adds Connect-RPC + http2 (x/net) deps for the test module and bumps related RR deps. |
tests/go.sum |
Updates test module checksums for new/updated deps. |
go.work.sum |
Workspace sum updates after dependency graph changes. |
.golangci.yml |
Removes an unused dupl settings block. |
💡 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
🧹 Nitpick comments (1)
rpc.go (1)
38-39: ⚡ Quick winAdd a regression test for the new empty-ID contract.
This branch changes client-visible behavior, but I don't see a test locking in the new invalid-input response. A small table-driven case covering the ID-required methods would make this harder to regress later.
🤖 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 38 - 39, Add a table-driven regression test that asserts the new empty-ID contract: for each RPC handler that checks msg.GetId() and returns connect.NewError(connect.CodeInvalidArgument, errEmptyID), invoke the RPC with a request whose Id is "" and assert the response is an error with connect.CodeInvalidArgument and the errEmptyID message; use msg.GetId() to build the failing requests, cover every ID-required method in the service (the functions that call msg.GetId()), and fail the test if any method returns a different code or message to prevent regressions.
🤖 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 `@tests/rpc.go`:
- Around line 24-27: The shared HTTP client h2cClient sets a global
http.Client.Timeout of 10s which is shorter than many test waits (up to 100s)
and causes client-side timeouts; remove the blanket http.Client.Timeout from the
h2cClient initializer or make it zero (no global deadline) and instead set
per-request deadlines derived from each test's requested wait value (e.g., use
context.WithTimeout in the RPC call path before using the client) so long waits
in tests are honored; update references to h2cClient and any call sites that
currently rely on the global Timeout to use per-call contexts.
---
Nitpick comments:
In `@rpc.go`:
- Around line 38-39: Add a table-driven regression test that asserts the new
empty-ID contract: for each RPC handler that checks msg.GetId() and returns
connect.NewError(connect.CodeInvalidArgument, errEmptyID), invoke the RPC with a
request whose Id is "" and assert the response is an error with
connect.CodeInvalidArgument and the errEmptyID message; use msg.GetId() to build
the failing requests, cover every ID-required method in the service (the
functions that call msg.GetId()), and fail the test if any method returns a
different code or message to prevent regressions.
🪄 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: 77e91ef9-000f-4390-9259-2aa83893215f
⛔ 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 (7)
.golangci.ymlgo.modplugin.gorpc.gotests/go.modtests/lock_test.gotests/rpc.go
💤 Files with no reviewable changes (1)
- .golangci.yml
The 10s client-side Timeout I added cuts off any goroutine in TestLockInit whose `wait` value exceeds 10s — and TestLockInit intentionally sweeps wait values up to ~91s (genRandNum(90)+1)*secMult. CI failed with `Client.Timeout exceeded while awaiting headers` on the LockRead and other long-wait calls. Lock RPCs already carry a server-honored Wait field that bounds the acquire; the server's wait is the authoritative deadline. Removing the client Timeout restores parity with the prior goridge transport, which also had no client-level timeout.
- locker.go: drop ptrTo[T] generic helper; use Go 1.26 new(val) at all 7 atomic.Pointer[string].Store() call sites - tests/lock_test.go: - wg.Add(1)/wg.Done() pairs across 6 test setups -> wg.Go (Go 1.25) - wg2.Add(2) + 2 explicit Done() -> 2x wg.Go (Go 1.25) - for i := 0; i < 100; i++ (i unused) -> for range 100 (Go 1.22) - sort.Ints -> slices.Sort (Go 1.21); drop "sort" import No behavior change; build and lint clean on root + tests modules.
- rpc.go: rpc{pl, log} -> rpc{pl}; r.log was always p.log. Single
source of truth at p.log; six handlers now reach it via r.pl.log.
- plugin.go, rpc.go, tests/rpc.go: trim narration-style godoc that
restated the signature/body. Keep the short rationale comments.
Per user preference: Connect handler bodies pull each field directly from req.Msg, no local alias. Keeps the call site self-documenting.
New tests/lock_api_test.go runs a Lock / Exists / Release / Exists cycle three times — once per wire protocol Connect-RPC serves off the single rpc-plugin mount: - TestLockConnectAPI: Connect over h2c via connectrpc.com/connect. - TestLockHTTPApi: plain HTTP/1.1 POST with a protojson body and Content-Type: application/json (what PHP clients do via Guzzle/curl). - TestLockGRPCApi: regular gRPC via google.golang.org/grpc (used by PHP's gRPC extension). Helper startLockAPIContainer brings up rpc + lock + logger with a new minimal configs/.rr-lock-api.yaml. GET-idempotency test for Exists comes in a follow-up commit once api-go ships a beta with `option idempotency_level = NO_SIDE_EFFECTS` (api PR #72).
api-go v6.0.0-beta.7 (tagged off api PR #72) regenerates the LockServiceHandler with WithIdempotency(IdempotencyNoSideEffects) on Exists. Connect now accepts HTTP GET for that method, encoding the request body in query params. - Bump api-go: v6.0.0-beta.5 -> v6.0.0-beta.7 in root + tests modules. - Add a table-driven TestLockHTTPGetIdempotency asserting: GET Exists -> 200 OK GET Lock, LockRead, Release, ForceRelease, UpdateTTL -> 405 Method Not Allowed Regression coverage for the proto idempotency annotation.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/lock_api_test.go (1)
67-67: ⚡ Quick winConsider more robust readiness check instead of fixed sleep.
The 500ms sleep assumes the container will be ready within that time. Under heavy load or slow CI environments, this could cause flaky test failures.
Alternative: Poll for readiness
// Wait for the server to be ready deadline := time.Now().Add(5 * time.Second) for time.Now().Before(deadline) { conn, err := net.DialTimeout("tcp", lockAPIAddr, 100*time.Millisecond) if err == nil { conn.Close() break } time.Sleep(50 * time.Millisecond) }This polls the endpoint until it accepts connections, with a reasonable 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 `@tests/lock_api_test.go` at line 67, Replace the fixed time.Sleep(500 * time.Millisecond) with a polling readiness check that repeatedly attempts to open a TCP connection to lockAPIAddr until success or a deadline is reached (e.g., 5s). Use net.DialTimeout with a short per-attempt timeout (e.g., 100ms), sleep briefly between attempts (e.g., 50ms), close the connection on success and break; if the deadline expires, fail the test. Update tests/lock_api_test.go where time.Sleep is used and reference the lockAPIAddr variable for the probe.rpc.go (1)
20-25: 💤 Low valueConsider validating
waitUsfor negative values.The
waitContexthelper convertswaitUsdirectly totime.Duration. If a client sends a negativewaitUs,time.Microsecond * time.Duration(negative)will create a negative duration, which causescontext.WithTimeoutto return an already-expired context.While this fails-fast behavior might be acceptable, it's worth considering whether to:
- Explicitly validate and reject negative
waitUsvalues- Document that negative values result in immediate timeout
Optional: Add validation for negative values
func waitContext(parent context.Context, waitUs int64) (context.Context, context.CancelFunc) { + if waitUs < 0 { + waitUs = 0 + } if waitUs == 0 { return context.WithTimeout(parent, defaultImmediateTimeout) } return context.WithTimeout(parent, time.Microsecond*time.Duration(waitUs)) }🤖 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 20 - 25, The helper waitContext should guard against negative waitUs to avoid creating an already-expired context; update waitContext to check if waitUs < 0 and treat it the same as 0 (i.e., call context.WithTimeout(parent, defaultImmediateTimeout)) instead of converting a negative value to time.Duration. Modify the function handling for waitUs in waitContext to perform this validation before constructing the duration so negative inputs are clamped to the immediate timeout behavior.
🤖 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.
Nitpick comments:
In `@rpc.go`:
- Around line 20-25: The helper waitContext should guard against negative waitUs
to avoid creating an already-expired context; update waitContext to check if
waitUs < 0 and treat it the same as 0 (i.e., call context.WithTimeout(parent,
defaultImmediateTimeout)) instead of converting a negative value to
time.Duration. Modify the function handling for waitUs in waitContext to perform
this validation before constructing the duration so negative inputs are clamped
to the immediate timeout behavior.
In `@tests/lock_api_test.go`:
- Line 67: Replace the fixed time.Sleep(500 * time.Millisecond) with a polling
readiness check that repeatedly attempts to open a TCP connection to lockAPIAddr
until success or a deadline is reached (e.g., 5s). Use net.DialTimeout with a
short per-attempt timeout (e.g., 100ms), sleep briefly between attempts (e.g.,
50ms), close the connection on success and break; if the deadline expires, fail
the test. Update tests/lock_api_test.go where time.Sleep is used and reference
the lockAPIAddr variable for the probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce1ac699-e54f-46c0-8417-cd5824d36552
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.modlocker.goplugin.gorpc.gotests/configs/.rr-lock-api.yamltests/go.modtests/lock_api_test.gotests/lock_test.gotests/rpc.go
✅ Files skipped from review due to trivial changes (1)
- tests/configs/.rr-lock-api.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/rpc.go
- tests/lock_test.go
- tests/go.mod
- plugin.go
The sync.OnceValue + package-level h2cClient was a micro-optimization that didn't belong in test code. newLockClient now builds a fresh *http.Client every call — simpler, no gochecknoglobals suppression, and the cost is negligible against the RPC round-trip itself.
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.
Summary
plugin.go:RPC() any→RPC() (string, http.Handler)returninglockV1connect.NewLockServiceHandler. The rpc plugin's HTTP/2 mux mounts the handler at the generated path (/lock.v1.LockService/).rpc.go: 6 methods adapted to the generated Connect handler interface. Empty-ID validation now returnsconnect.CodeInvalidArgument. The caller'sctxis now propagated into the locker acquire (was previouslycontext.Background()), so client cancellation/deadline correctly bounds the wait.waitContexthelper DRYs out the timeout setup that was duplicated across all 6 methods.connectrpc.com/connectwith a sharedhttp2.Transportoversync.OnceValue, so the 1700-goroutineTestLockInitreuses pooled idle connections instead of dialing per call.addressparameter (always127.0.0.1:6001from the rr config).api-gov6.0.0-beta.5,connectrpc.com/connectv1.19.2,golang.org/x/netfor h2c.goridge/v4is no longer imported..golangci.yml: removed an orphansettings.duplblock (dupl wasn't in the enabled linters list).Breaking changes
goridgeRpc) cannot reach the lock plugin once a roadrunner build with this change is deployed./lock.v1.LockService/{Method}instead oflock.{Method}.idpreviously surfaced as a plain string error; now returnsCodeInvalidArgument.Summary by CodeRabbit
Refactor
Tests
Chores