fix(router): connection leak with redis pub/sub#2916
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughRedis Pub/Sub connection cleanup in the provider adapter is refactored to use deferred ChangesRedis Pub/Sub connection cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![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 `@router/pkg/pubsub/redis/adapter_test.go`:
- Around line 65-69: The test eagerly evaluates p.conn.PoolStats().TotalConns in
the require.Eventually call, producing misleading args on failure; fix by
capturing the observed connection count inside the polling closure (introduce a
local var like observed int and set observed = p.conn.PoolStats().TotalConns
inside the func passed to the poller), change require.Eventually to
assert.Eventually (which returns bool) and then, if it returns false, call
t.Fatalf("redis pub/sub connections leaked: have %d, baseline %d", observed,
baseline) so the printed "have" value reflects the last observed measurement at
failure time; update references to p.conn.PoolStats().TotalConns and baseline in
the new code accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91753c39-0739-4ad0-b58e-ceeca0d9228f
📒 Files selected for processing (2)
router/pkg/pubsub/redis/adapter.gorouter/pkg/pubsub/redis/adapter_test.go
| require.Eventually(t, func() bool { | ||
| return p.conn.PoolStats().TotalConns <= baseline | ||
| }, 5*time.Second, 10*time.Millisecond, | ||
| "redis pub/sub connections leaked: have %d, baseline %d", | ||
| p.conn.PoolStats().TotalConns, baseline) |
There was a problem hiding this comment.
require.Eventually failure message args are evaluated eagerly.
p.conn.PoolStats().TotalConns at Line 69 is evaluated once when Eventually is invoked (right after cancelling, before any polling), not at failure time. For a leak-detection test, the printed "have %d" can be misleading during CI debugging. Capture the value inside the condition or via a local closure variable.
💚 Proposed fix to report the value observed at failure time
- require.Eventually(t, func() bool {
- return p.conn.PoolStats().TotalConns <= baseline
- }, 5*time.Second, 10*time.Millisecond,
- "redis pub/sub connections leaked: have %d, baseline %d",
- p.conn.PoolStats().TotalConns, baseline)
+ var lastTotal uint32
+ require.Eventually(t, func() bool {
+ lastTotal = p.conn.PoolStats().TotalConns
+ return lastTotal <= baseline
+ }, 5*time.Second, 10*time.Millisecond,
+ "redis pub/sub connections leaked: have %d, baseline %d",
+ lastTotal, baseline)📝 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.
| require.Eventually(t, func() bool { | |
| return p.conn.PoolStats().TotalConns <= baseline | |
| }, 5*time.Second, 10*time.Millisecond, | |
| "redis pub/sub connections leaked: have %d, baseline %d", | |
| p.conn.PoolStats().TotalConns, baseline) | |
| var lastTotal uint32 | |
| require.Eventually(t, func() bool { | |
| lastTotal = p.conn.PoolStats().TotalConns | |
| return lastTotal <= baseline | |
| }, 5*time.Second, 10*time.Millisecond, | |
| "redis pub/sub connections leaked: have %d, baseline %d", | |
| lastTotal, baseline) |
🤖 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 `@router/pkg/pubsub/redis/adapter_test.go` around lines 65 - 69, The test
eagerly evaluates p.conn.PoolStats().TotalConns in the require.Eventually call,
producing misleading args on failure; fix by capturing the observed connection
count inside the polling closure (introduce a local var like observed int and
set observed = p.conn.PoolStats().TotalConns inside the func passed to the
poller), change require.Eventually to assert.Eventually (which returns bool) and
then, if it returns false, call t.Fatalf("redis pub/sub connections leaked: have
%d, baseline %d", observed, baseline) so the printed "have" value reflects the
last observed measurement at failure time; update references to
p.conn.PoolStats().TotalConns and baseline in the new code accordingly.
1 participant
Fix for: #2915
Summary by CodeRabbit
Bug Fixes
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.