feat: add sane defaults for gRPC connection keepalive#72
feat: add sane defaults for gRPC connection keepalive#72
Conversation
Set default values for connection idle (300s), age (1800s), and grace (30s) to prevent indefinite connection lifetimes that cause load balancing skew, slow rolling deployments, and stale DNS. Use -1 to opt out and restore infinite lifetime for any parameter.
📝 WalkthroughWalkthroughREADME anchors updated; three gRPC keepalive envconfig fields now have defaults (300/1800/30) and note Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
Adds default gRPC server connection keepalive lifetimes (idle/age/grace) to encourage periodic connection rotation, improving load balancing behavior and reducing issues from long-lived/stale connections.
Changes:
- Set env-driven defaults for
GRPCServerMaxConnection*InSeconds(300s idle, 1800s age, 30s grace) and document-1as an opt-out to “infinite”. - Update gRPC server option construction to apply keepalive params when any keepalive field is non-zero (enabling the
-1sentinel behavior). - Add/adjust tests around keepalive option selection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates generated doc links/line references. |
| core.go | Changes keepalive option gating logic from > 0 to != 0. |
| core_coverage_test.go | Adds new keepalive-related tests and adjusts existing default test behavior. |
| config/README.md | Documents new defaults and -1 opt-out for keepalive config fields. |
| config/config.go | Adds envconfig defaults and updates comments for keepalive-related config fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Compare option count against all-zero baseline instead of hardcoded thresholds, so tests fail if keepalive params are not actually appended. Rename TestGetGRPCServerOptions_Default to _WithEnvconfigDefaults to clarify it tests the envconfig default values, not zero-value config.
Fixes govulncheck failures for GO-2026-4860 (crypto/tls), GO-2026-4861 (net/http), GO-2026-4862 (os/exec), GO-2026-4865 (html/template).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Eliminates tautological assertion by comparing all-zero config against a positive-value config, proving zero actually omits keepalive option.
![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 the current code and only fix it if needed.
Inline comments:
In `@core_coverage_test.go`:
- Around line 1371-1373: The test calls ConfigureInterceptors with five
arguments but the current ConfigureInterceptors function (named
ConfigureInterceptors) only accepts four; either remove the trailing 60 from the
TestConfigureInterceptors_BothBranches call or update ConfigureInterceptors's
signature to accept a timeout parameter and thread that new parameter through
its implementation (e.g., add a timeout int/uint param to ConfigureInterceptors
and update any call sites and internal uses). Locate the ConfigureInterceptors
function and either align the test to its existing four-arg signature or add the
fifth timeout argument to the function and its callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| func TestConfigureInterceptors_BothBranches(t *testing.T) { | ||
| ConfigureInterceptors(true, "X-My-Trace", "info", false) | ||
| ConfigureInterceptors(true, "X-My-Trace", "info", false, 60) | ||
| } |
There was a problem hiding this comment.
Fix the ConfigureInterceptors call signature.
Line 1372 passes five arguments, but ConfigureInterceptors in initializers.go currently accepts four, so this test does not compile. If the extra timeout is intentional, the matching signature change needs to land in the same PR; otherwise, drop the trailing 60.
Suggested fix
func TestConfigureInterceptors_BothBranches(t *testing.T) {
- ConfigureInterceptors(true, "X-My-Trace", "info", false, 60)
+ ConfigureInterceptors(true, "X-My-Trace", "info", false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestConfigureInterceptors_BothBranches(t *testing.T) { | |
| ConfigureInterceptors(true, "X-My-Trace", "info", false) | |
| ConfigureInterceptors(true, "X-My-Trace", "info", false, 60) | |
| } | |
| func TestConfigureInterceptors_BothBranches(t *testing.T) { | |
| ConfigureInterceptors(true, "X-My-Trace", "info", false) | |
| } |
🧰 Tools
🪛 GitHub Actions: Go
[error] 1372-1372: golangci-lint build/test failure: too many arguments in call to ConfigureInterceptors in core_coverage_test.go:1372:59.
🪛 GitHub Check: lint
[failure] 1372-1372:
too many arguments in call to ConfigureInterceptors
🪛 GitHub Check: test
[failure] 1372-1372:
too many arguments in call to ConfigureInterceptors
🪛 golangci-lint (2.11.4)
[error] 1372-1372: too many arguments in call to ConfigureInterceptors
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core_coverage_test.go` around lines 1371 - 1373, The test calls
ConfigureInterceptors with five arguments but the current ConfigureInterceptors
function (named ConfigureInterceptors) only accepts four; either remove the
trailing 60 from the TestConfigureInterceptors_BothBranches call or update
ConfigureInterceptors's signature to accept a timeout parameter and thread that
new parameter through its implementation (e.g., add a timeout int/uint param to
ConfigureInterceptors and update any call sites and internal uses). Locate the
ConfigureInterceptors function and either align the test to its existing
four-arg signature or add the fifth timeout argument to the function and its
callers.
|
Superseded by #73 which includes these keepalive changes alongside the default timeout config. |
Summary
-1to opt out and restore infinite lifetime for any parameter (outer condition changed from> 0to!= 0)Test plan
-raceTestGetGRPCServerOptions_WithEnvconfigDefaults,KeepaliveDisabledWithNegativeOne,KeepaliveMixed,KeepaliveAllZeromake lintclean,govulncheckclean (after Go 1.25.9 bump)-1opt-out preserves infinite behaviorSummary by CodeRabbit
New Features
-1.Bug Fixes
Documentation
Tests
Chores