Fix/redis sentinel credentials#320
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds support for Redis Sentinel authentication credentials across notifier, session, and OAuth2 storage components, wires them through configuration and factories, and adjusts Redis stream watching behavior to start from new messages only. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The function signatures for
NewRedisNotifierandNewRedisStorageare getting quite long with the added sentinel credentials; consider introducing a small options/config struct to group these parameters and make call sites easier to read and maintain. - In
RedisNotifier.Watch, changinglastIDfrom the previous tail (viaXRevRangeN) to"$"alters behavior to only receive new messages and not the latest existing one; if this is intended, it might be worth double-checking all callers rely on this semantics and don’t expect to catch the most recent configuration on startup. - In
TestLoadConfig_MCPGateway_RedisSentinelCredentials, theif err != nil { return }afterassert.NoError(t, err)is redundant and can be removed since the assertion already fails the test in that case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The function signatures for `NewRedisNotifier` and `NewRedisStorage` are getting quite long with the added sentinel credentials; consider introducing a small options/config struct to group these parameters and make call sites easier to read and maintain.
- In `RedisNotifier.Watch`, changing `lastID` from the previous tail (via `XRevRangeN`) to `"$"` alters behavior to only receive new messages and not the latest existing one; if this is intended, it might be worth double-checking all callers rely on this semantics and don’t expect to catch the most recent configuration on startup.
- In `TestLoadConfig_MCPGateway_RedisSentinelCredentials`, the `if err != nil { return }` after `assert.NoError(t, err)` is redundant and can be removed since the assertion already fails the test in that case.
## Individual Comments
### Comment 1
<location path="internal/mcp/storage/notifier/redis.go" line_range="72" />
<code_context>
go func() {
defer close(ch)
+ // Start from the latest message ($ means read only new messages)
+ lastID := "$"
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing `lastID` to `$` alters notifier semantics and may skip the latest config on startup.
With the previous `XRevRangeN(..., 1)` approach, `Watch` ensured consumers saw the most recent config (or at least didn’t miss updates that happened right before subscription). Using `lastID := "$"` means subscribers now only see messages after `Watch` starts and never get the current tail, which can leave them on a stale config until the next update. If you still want to avoid replaying history but deliver the current tail, consider keeping the initial `XRevRangeN` and seeding `lastID` with that ID before `XRead`, or verify that all callers are OK with no initial config snapshot.
</issue_to_address>
### Comment 2
<location path="internal/common/config/config_test.go" line_range="112-114" />
<code_context>
+ file := filepath.Join(tmp, "mcp-gateway.yaml")
+ assert.NoError(t, os.WriteFile(file, []byte(yaml), 0o644))
+
+ cfg, _, err := LoadConfig[MCPGatewayConfig]("mcp-gateway.yaml")
+ assert.NoError(t, err)
+ if err != nil {
+ return
+ }
</code_context>
<issue_to_address>
**nitpick:** Use `require.NoError` or drop the redundant `if err != nil` in this test
After `assert.NoError(t, err)`, the `if err != nil { return }` branch is dead code in a passing test. Either use `require.NoError(t, err)` and drop the `if`, or keep `assert` and remove the `if` block to simplify the control flow and avoid implying early-return is part of the behavior under test.
</issue_to_address>
### Comment 3
<location path="internal/mcp/storage/notifier/redis_test.go" line_range="39" />
<code_context>
stream := "unla:mcp:updates"
- recv, err := NewRedisNotifier(logger, cnst.RedisClusterTypeSingle, mr.Addr(), "", "", "", 0, stream, config.RoleReceiver)
+ recv, err := NewRedisNotifier(logger, cnst.RedisClusterTypeSingle, mr.Addr(), "", "", "", "", "", 0, stream, config.RoleReceiver)
assert.NoError(t, err)
assert.NotNil(t, recv)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to capture the new watch behavior (only new messages after subscription)
Since `Watch` now starts from `$`, it should only receive messages published after subscription. We already cover the happy path where the message is published after `Watch`, but we’re missing a test that asserts messages published before `Watch` are not delivered. Please add a test that:
1. Publishes a message to the stream
2. Calls `Watch` and reads from the channel for a bit
3. Asserts that the earlier message is never emitted
This will lock in the new offset semantics and prevent regressions.
Suggested implementation:
```golang
logger := zap.NewNop()
stream := "unla:mcp:updates"
recv, err := NewRedisNotifier(logger, cnst.RedisClusterTypeSingle, mr.Addr(), "", "", "", "", "", 0, stream, config.RoleReceiver)
assert.NoError(t, err)
assert.NotNil(t, recv)
assert.NotNil(t, ch)
// Create a sender and push an update
send, err := NewRedisNotifier(logger, cnst.RedisClusterTypeSingle, mr.Addr(), "", "", "", "", "", 0, stream, config.RoleSender)
assert.NoError(t, err)
cfg := &config.MCPConfig{Name: "cfg-1"}
assert.NoError(t, send.NotifyUpdate(context.Background(), cfg))
}
func TestRedisNotifierWatchIgnoresExistingMessages(t *testing.T) {
mr, err := miniredis.Run()
require.NoError(t, err)
defer mr.Close()
logger := zap.NewNop()
stream := "unla:mcp:updates"
// Create a sender and publish a message *before* starting Watch.
sender, err := NewRedisNotifier(logger, cnst.RedisClusterTypeSingle, mr.Addr(), "", "", "", "", "", 0, stream, config.RoleSender)
assert.NoError(t, err)
cfgBefore := &config.MCPConfig{Name: "cfg-before-watch"}
assert.NoError(t, sender.NotifyUpdate(context.Background(), cfgBefore))
// Now create the receiver and start Watch, which should start from `$`
recv, err := NewRedisNotifier(logger, cnst.RedisClusterTypeSingle, mr.Addr(), "", "", "", "", "", 0, stream, config.RoleReceiver)
assert.NoError(t, err)
assert.NotNil(t, recv)
ch, err := recv.Watch(context.Background())
assert.NoError(t, err)
assert.NotNil(t, ch)
// Ensure that no message is delivered from the earlier publish.
select {
case msg := <-ch:
assert.Failf(t, "expected no messages from Watch for existing entries", "got: %#v", msg)
case <-time.After(100 * time.Millisecond):
// Expected path: nothing delivered for pre-subscription messages.
}
}
```
1. Ensure the test file imports the dependencies used in the new test:
- `github.com/alicebob/miniredis/v2` as `miniredis` if not already present.
- `github.com/stretchr/testify/require` as `require` if not already present.
- The standard library `time` package for `time.After`.
2. The `recv.Watch` call assumes the existing signature `Watch(ctx context.Context) (<-chan *config.MCPConfig, error)`; if the signature differs, update the call in the new test to match the existing tests.
3. If there is a shared helper for setting up `miniredis` or notifiers in this test file, you may want to refactor the new test to reuse that helper instead of duplicating setup logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kx-byte
force-pushed
the
fix/redis-sentinel-credentials
branch
from
884b4fd to
043019f
Compare
iFurySt
approved these changes
Jun 8, 2026
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
2 participants
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 by Sourcery
Add support for Redis Sentinel credentials across notifier, session, and OAuth2 Redis clients and ensure MCP gateway configuration loads them correctly.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: