feat: wire GRPC_SERVER_DEFAULT_TIMEOUT_IN_SECONDS config#73
Merged
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.
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).
Eliminates tautological assertion by comparing all-zero config against a positive-value config, proving zero actually omits keepalive option.
Adds config field and wiring for the DefaultTimeoutInterceptor introduced in interceptors. The env var defaults to 60 seconds; set to 0 to disable. Depends on go-coldbrew/interceptors#37.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded a new gRPC default-unary-request timeout config and threaded it into interceptor initialization; changed interceptor function to unexported and extended its signature; adjusted keepalive gating logic to use Changes
Sequence Diagram(s)sequenceDiagram
participant Cfg as Config
participant Proc as processConfig()
participant Init as configureInterceptors()
participant GRPC as gRPC Server
rect rgba(220,235,255,0.5)
Cfg->>Proc: read GRPCServerDefaultTimeoutInSeconds
Proc->>Init: pass defaultTimeoutInSeconds
Init->>Init: set interceptors.DefaultTimeout (time.Second * n or 0)
end
rect rgba(220,255,220,0.5)
Proc->>GRPC: call getGRPCServerOptions()
note right of GRPC: apply Keepalive params when\nGRPCServerMaxConnection*InSeconds != 0
GRPC-->>Proc: return server options list
end
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 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a configurable default server-side deadline for unary gRPC requests that arrive without a client deadline, wiring it through startup interceptor configuration.
Changes:
- Introduces
GRPC_SERVER_DEFAULT_TIMEOUT_IN_SECONDSconfig (default 60; 0 disables). - Wires the new config into
ConfigureInterceptorsand applies it viainterceptors.SetDefaultTimeout(...). - Updates coverage test call site for the new function parameter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
initializers.go |
Extends ConfigureInterceptors to apply a default unary gRPC timeout when configured. |
core.go |
Passes GRPCServerDefaultTimeoutInSeconds into interceptor configuration during startup. |
core_coverage_test.go |
Updates the interceptor configuration test to match the new signature. |
config/config.go |
Adds the new env-configurable timeout field with documentation and default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core.go (1)
631-650: The keepalive gating logic is correct but could benefit from a clarifying comment.The change from
> 0to!= 0enables the-1opt-out semantics:
0: don't configure keepalive at all (outer check false)-1: configure keepalive but with infinite duration for that parameter (outer check true, inner check false → zero value → gRPC treats as infinity)>0: apply the configured durationThis is a subtle but intentional semantic change. Consider adding a brief comment explaining the tri-state behavior.
📝 Suggested comment
+ // Keepalive configuration uses tri-state semantics: + // 0 = don't configure keepalive at all (use gRPC defaults) + // -1 = explicitly disable this limit (infinite) + // >0 = apply this duration in seconds if c.config.GRPCServerMaxConnectionAgeGraceInSeconds != 0 || c.config.GRPCServerMaxConnectionAgeInSeconds != 0 || c.config.GRPCServerMaxConnectionIdleInSeconds != 0 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.go` around lines 631 - 650, Add a short clarifying comment above the outer if that explains the tri-state semantics introduced by changing the checks to != 0: 0 = skip keepalive config, -1 = include keepalive but leave that ServerParameters field zero (treated as infinite by gRPC), >0 = set the explicit duration; reference the config fields (c.config.GRPCServerMaxConnectionAgeInSeconds, c.config.GRPCServerMaxConnectionAgeGraceInSeconds, c.config.GRPCServerMaxConnectionIdleInSeconds) and the resulting ServerParameters variable (option) which is passed to grpc.KeepaliveParams so future readers understand why the outer check uses != 0 while inner checks use > 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core.go`:
- Around line 631-650: Add a short clarifying comment above the outer if that
explains the tri-state semantics introduced by changing the checks to != 0: 0 =
skip keepalive config, -1 = include keepalive but leave that ServerParameters
field zero (treated as infinite by gRPC), >0 = set the explicit duration;
reference the config fields (c.config.GRPCServerMaxConnectionAgeInSeconds,
c.config.GRPCServerMaxConnectionAgeGraceInSeconds,
c.config.GRPCServerMaxConnectionIdleInSeconds) and the resulting
ServerParameters variable (option) which is passed to grpc.KeepaliveParams so
future readers understand why the outer check uses != 0 while inner checks use >
0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f37ee367-84a0-4265-a1bb-44ae85c49402
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.mdconfig/README.mdconfig/config.gocore.gocore_coverage_test.gogo.modinitializers.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
README.md:125
- The API reference for ConfigureInterceptors is now out of sync with the actual function signature in initializers.go (the code block still shows 4 parameters, but the function now takes an additional defaultTimeoutInSeconds parameter). Please regenerate/update the gomarkdoc output so the signature matches, otherwise downstream users will call it incorrectly.
## func [ConfigureInterceptors](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L389>)
```go
func ConfigureInterceptors(DoNotLogGRPCReflection bool, traceHeaderName string, responseTimeLogLevel string, responseTimeLogErrorOnly bool)
</details>
---
💡 <a href="/go-coldbrew/core/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
README.md:125
- The README’s generated signature for ConfigureInterceptors is now out of sync with the code: initializers.go adds a new
defaultTimeoutInSecondsparameter, but the README still shows the old 4-arg signature. Please regenerate/update the docs so the signature (and any TOC entry) matches the exported function.
## func [ConfigureInterceptors](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L389>)
```go
func ConfigureInterceptors(DoNotLogGRPCReflection bool, traceHeaderName string, responseTimeLogLevel string, responseTimeLogErrorOnly bool)
</details>
---
💡 <a href="/go-coldbrew/core/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core_coverage_test.go (2)
1371-1386: This is smoke coverage, not timeout verification.All three subtests pass as long as
configureInterceptorsreturns, so losing the timeout assignment without panicking would still keep the test green. Please assert an observable postcondition for the enabled, disabled, and negative cases.🤖 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 - 1386, The test currently only calls configureInterceptors so it only checks it doesn't panic; update Test_configureInterceptors_BothBranches to assert an observable postcondition for each case: call configureInterceptors(...) and then verify its effect (for example, inspect the returned middleware or a public/accessible timeout field it sets) according to defaultTimeoutInSeconds: when defaultTimeoutInSeconds > 0 assert the configured timeout equals that value (or that a timeout-enforcing interceptor/middleware is present), when == 0 assert no timeout interceptor/zero timeout is configured, and when < 0 assert the disabled/zero-timeout behavior is applied; reference configureInterceptors and the test name to locate the code and add assertions rather than only calling the function.
452-525: Tighten what these keepalive tests are actually proving.Right now they only compare option counts, and
TestGetGRPCServerOptions_WithEnvconfigDefaultshardcodes the same values instead of loading defaults. That makes the suite brittle to unrelated option additions while still missing regressions in the config-default path. A smaller behavior-focused assertion, or a separate config-loading test, would age better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 452 - 525, The tests only compare option counts and the "WithEnvconfigDefaults" test hardcodes default values; update them to assert presence/absence of a keepalive option itself rather than len(opts). Specifically, in TestGetGRPCServerOptions_WithEnvconfigDefaults and the other keepalive tests, call cb.getGRPCServerOptions() and scan the returned opts for the keepalive option (e.g., identify grpc.KeepaliveParams/keepalive.ServerParameters by type-asserting or inspecting the option's string) to assert it is present when any of GRPCServerMaxConnectionIdleInSeconds/GRPCServerMaxConnectionAgeInSeconds/GRPCServerMaxConnectionAgeGraceInSeconds trigger keepalive, and absent when all three are zero; also replace the hardcoded defaults in TestGetGRPCServerOptions_WithEnvconfigDefaults by constructing the cb.config from the package's actual default config (use the config package's default loader/constructor function instead of literal values) so the test verifies the true envconfig defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core_coverage_test.go`:
- Around line 1371-1386: The test currently only calls configureInterceptors so
it only checks it doesn't panic; update Test_configureInterceptors_BothBranches
to assert an observable postcondition for each case: call
configureInterceptors(...) and then verify its effect (for example, inspect the
returned middleware or a public/accessible timeout field it sets) according to
defaultTimeoutInSeconds: when defaultTimeoutInSeconds > 0 assert the configured
timeout equals that value (or that a timeout-enforcing interceptor/middleware is
present), when == 0 assert no timeout interceptor/zero timeout is configured,
and when < 0 assert the disabled/zero-timeout behavior is applied; reference
configureInterceptors and the test name to locate the code and add assertions
rather than only calling the function.
- Around line 452-525: The tests only compare option counts and the
"WithEnvconfigDefaults" test hardcodes default values; update them to assert
presence/absence of a keepalive option itself rather than len(opts).
Specifically, in TestGetGRPCServerOptions_WithEnvconfigDefaults and the other
keepalive tests, call cb.getGRPCServerOptions() and scan the returned opts for
the keepalive option (e.g., identify
grpc.KeepaliveParams/keepalive.ServerParameters by type-asserting or inspecting
the option's string) to assert it is present when any of
GRPCServerMaxConnectionIdleInSeconds/GRPCServerMaxConnectionAgeInSeconds/GRPCServerMaxConnectionAgeGraceInSeconds
trigger keepalive, and absent when all three are zero; also replace the
hardcoded defaults in TestGetGRPCServerOptions_WithEnvconfigDefaults by
constructing the cb.config from the package's actual default config (use the
config package's default loader/constructor function instead of literal values)
so the test verifies the true envconfig defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d83504e-dd4e-49fd-a908-013ca51b5bb1
📒 Files selected for processing (4)
config/config.gocore.gocore_coverage_test.goinitializers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- initializers.go
- config/config.go
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
Merges keepalive defaults (#72) and adds default timeout config:
Keepalive defaults (from #72)
-1opt-out for infinite lifetimeDefault timeout (new)
GRPC_SERVER_DEFAULT_TIMEOUT_IN_SECONDSconfig field (default: 60, set to 0 to disable)interceptors.SetDefaultTimeout()viaConfigureInterceptorsDepends on go-coldbrew/interceptors#37.
Test plan
TestConfigureInterceptors_BothBranchesupdated for new parametermake buildpasses (with local interceptors replace)make testpasses with-racemake lintcleanSummary by CodeRabbit
New Features
Documentation
Tests
Chores