chore: simplify, modernize (Go 1.26), update deps#147
Merged
Conversation
- Fix MaxConnectionAgeGrace using wrong field (MaxConnectionAge) in keepalive params - Guard healthServer nil in Stop() before Serve() is called - Fix Workers() to skip erroring workers instead of dropping all remaining - Add mutex reads in health_server List() and Check() to prevent data race - Drop redundant UnaryServerInterceptor type cast; use errors.Errorf directly - Replace os.IsNotExist with errors.Is(err, os.ErrNotExist) throughout config.go - Eliminate static_pool import; call Destroy via the api.Pool interface - proxy: single strings.Split replaces Contains+Split double-scan - proxy: reslice Body/Context to [:0] in putPld to preserve pool allocations - proxy: drop redundant phpCode < math.MaxUint32 check (ParseUint bitSize=32 guarantees it) - php/keywords: replace linear isReserved scan with map lookup; isSpacerChar via ContainsRune - php/keywords, php/ns: replace bytes.Buffer with strings.Builder - php/template: cache parsed template with sync.OnceValue instead of re-parsing per call
|
Warning Review limit reached
More reviews will be available in 24 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy 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 (2)
📝 WalkthroughWalkthroughThis PR addresses stability, testing, and code quality across the gRPC plugin. It fixes a critical nil-interface panic in plugin lifecycle, adds concurrency protection to health status reads, corrects server configuration bugs, modernizes error handling patterns, expands test coverage for health/metrics/status endpoints, and simplifies PHP code generation utilities. ChangesgRPC Service Stability and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
This PR modernizes and simplifies the RoadRunner gRPC plugin while fixing several lifecycle, health-check, and configuration handling issues.
Changes:
- Corrects keepalive grace configuration and improves error construction.
- Adds synchronization for health status reads and safer shutdown guards.
- Modernizes proxy and PHP generator helpers with simplified parsing, builders, cached templates, and keyword lookup.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
server.go |
Simplifies interceptor setup and fixes MaxConnectionAgeGrace assignment. |
proxy/proxy.go |
Simplifies error parsing and adjusts payload pool reset behavior. |
protoc_plugins/protoc-gen-php-grpc/php/template.go |
Caches parsed PHP template and uses strings.Builder. |
protoc_plugins/protoc-gen-php-grpc/php/ns.go |
Replaces buffer usage with strings.Builder. |
protoc_plugins/protoc-gen-php-grpc/php/keywords.go |
Replaces keyword slice scan with map lookup and simplifies string building/splitting helpers. |
plugin.go |
Guards health server shutdown paths and simplifies pool destruction. |
health_server.go |
Adds locking around health status reads. |
config.go |
Uses errors.Is for file-not-found checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 68.31% 77.56% +9.24%
==========================================
Files 7 7
Lines 464 468 +4
==========================================
+ Hits 317 363 +46
+ Misses 113 79 -34
+ Partials 34 26 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- keywords.go: use slices.Contains over the existing reservedKeywords slice instead of introducing a second package-level map (+nolint). - template.go: revert to per-call template parse instead of a package-level sync.OnceValue global (this is a codegen path, per-call cost is irrelevant).
- Serve: NewPool returns a concrete *static_pool.Pool; on error it is a nil pointer. Assigning it straight to the api.Pool interface field made the interface non-nil while wrapping a nil pointer, so Stop's `p.gPool != nil` guard passed and Destroy dereferenced a nil receiver (panic). Assign p.gPool only after NewPool succeeds. - proxy.putPld: makePayload always reassigns Body/Context to the per-request buffers, so reslicing to [:0] never reused the preallocated slices and pinned the request's backing arrays in the pooled object. Reset to nil so they can be collected (addresses review feedback). - Workers: keep return-nil-on-error to stay consistent with the other RR plugins (http/tcp/centrifuge/temporal) instead of skipping bad workers.
…I coverage The root-package coverage step ran only ./codec ./parser ./protoc_plugins ./proxy, so root-package unit tests never counted toward codecov — root coverage came solely from the PHP integration suite, which skips several auxiliary surfaces. Add white-box unit tests for the most-uncovered root files: - status.go: Status/Ready over a fake api.Pool with workers driven into Ready/Working FSM states (0% -> 100%) - metrics.go: StatsExporter Describe/Collect across all worker-state branches plus MetricsCollector, over a fake Informer (0% -> 100%) - health_server.go: Check/List/Watch/SetServingStatus/Shutdown lifecycle via a minimal Health_WatchServer (0% -> ~95%) Wire `.` into the root-module coverage step so these run in CI. Root-package unit coverage rises 12.9% -> 37.1%; the codecov union gains the lines the integration suite never exercises. client_model becomes a direct dependency (metric-family assertions via dto).
Replace the manual channel-drain counter with assert.Len on the buffered Describe channel (a /simplify pass finding). Behavior and coverage unchanged.
![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
🤖 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 `@config.go`:
- Around line 112-115: The error message for validating c.TLS.RootCA is wrong
and misleading; locate the branch that checks if os.Stat(c.TLS.RootCA) returns
os.ErrNotExist (the code that currently returns errors.Errorf("root ca path
provided, but key file '%s' does not exists", c.TLS.RootCA")) and change the
message to correctly reference the Root CA file and fix grammar (e.g. "root CA
file '%s' does not exist" or "root CA path provided, but root CA file '%s' does
not exist"), keeping the same error construction and using c.TLS.RootCA.
🪄 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: 86026694-68e1-4e7e-ba1c-526d0efd4e92
📒 Files selected for processing (12)
.github/workflows/linux.ymlconfig.gogo.modhealth_server.gohealth_server_test.gometrics_test.goplugin.goprotoc_plugins/protoc-gen-php-grpc/php/keywords.goprotoc_plugins/protoc-gen-php-grpc/php/ns.goproxy/proxy.goserver.gostatus_test.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
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.
Fixed
Bugs (R)
server.go:MaxConnectionAgeGracewas set fromMaxConnectionAge(wrong field) — now usesMaxConnectionAgeGraceplugin.go:Stop()calledp.healthServer.SetServingStatusandShutdownunconditionally, causing nil panic whenStopis called beforeServe— guarded with nil checkplugin.go:Workers()returnednilon the first worker-state error, discarding all remaining workers — changed tocontinuehealth_server.go:List()andCheck()readh.statuswithout holdingh.mu— added mutex readplugin.go: type switch to*static_pool.Poolto callDestroywas unnecessary —api.Poolinterface exposesDestroydirectlySimplifications (S)
server.go: redundantgrpc.UnaryServerInterceptor(p.interceptor)cast removedserver.go:errors.Str(fmt.Sprintf(...))→errors.Errorf(...)proxy/proxy.go: double-scanContains+Split→ singlestrings.Splitwith length guardproxy/proxy.go: redundantphpCode < math.MaxUint32check removed (ParseUintwith bitSize=32 already guarantees it);mathimport droppedphp/keywords.go:isSpacerCharmulti-case switch →strings.ContainsRune("_ :-", c)php/keywords.go,php/ns.go:bytes.NewBuffer(nil)used as string builder →strings.Builderphp/template.go:template.Must(...).Parse(phpBody)called on everybody()invocation → cached withsync.OnceValueModernizations (M)
config.go:os.IsNotExist(err)×4 →errors.Is(err, os.ErrNotExist)proxy/proxy.go:putPldnils Body/Context defeating pool preallocation → reslice[:0]php/keywords.go:isReservedlinear scan over 70 keywords →map[string]struct{}lookupphp/keywords.go:make([]rune, 0)reset in loop →word[:0]Summary by CodeRabbit
Release Notes
Bug Fixes
Tests