fix(cache): pass watch keys to Transaction so optimistic lock actually works#701
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds Redis Cluster support, including new configuration fields and the InitRedisClusterClient utility. The PluginManager now supports cluster initialization. Key feedback highlights that the current Transaction() implementation is incompatible with Redis Cluster, suggests generalizing error messages in utility functions, and recommends refactoring redundant TLS configuration logic.
lin-snow
changed the title
Wire the missing third branch so operators can point the daemon at a Redis Cluster without falling back to dialing an empty REDIS_HOST:PORT and panicking at startup. * introduce REDIS_USE_CLUSTERS / REDIS_CLUSTERS / REDIS_CLUSTERS_PASSWORD envs, aligned with dify api naming so Helm can set a single env group * branch in PluginManager.Launch for Cluster ahead of the existing Sentinel / standalone paths; falls back to REDIS_PASSWORD if the cluster-specific password is not set * implement cache.InitRedisClusterClient via redis.NewClusterClient; downstream cache / lock / pub-sub helpers keep working because `client` is declared as redis.UniversalClient Redis Cluster disables SELECT DB, so the cluster branch does not plumb RedisDB. This is intentional and documented in the release note accompanying the overall Redis Cluster support work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Narrow the plugin-daemon Redis client to standalone and sentinel modes; remove the cluster code paths that were briefly exercised on this branch. Changes: - Remove REDIS_USE_CLUSTERS / REDIS_CLUSTERS / REDIS_CLUSTERS_PASSWORD env fields from app.Config. - Remove the cluster branch (plus the two cluster-specific fail-fast guards on REDIS_USE_CLUSTERS + REDIS_USE_SENTINEL and REDIS_DB != 0) from PluginManager.Launch. The init block is now just sentinel or standalone. - Delete cache.InitRedisClusterClient. The main redis client + all helpers (Transaction, pub/sub, lock, etc.) already route through redis.UniversalClient so downstream code needs no adjustment. Intentionally kept: - Transaction(fn, watchKeys...) signature: the variadic signature is backwards-compatible with every existing Transaction(fn) call and the one new-style caller (debugging_service) benefits from real WATCH semantics even on standalone — this is correctness-independent of cluster support. - parser.SplitAndTrimCSV: the sentinel branch migrated to it in 617e7a5 and still uses it to parse REDIS_SENTINELS, so the helper has a standalone/sentinel consumer and stays. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lin-snow
force-pushed
the
feat/redis-cluster-support
branch
from
e57d7af to
4dd87ac
Compare
dosubot
Bot
added
the
size:M
dosubot
Bot
added
bug
fatelei
approved these changes
Apr 24, 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.
Description
cache.Transaction(fn)used to callclient.Watch(ctx, fn)without passing any keys, so it ran a "MULTI/EXEC without optimistic lock" — the double-write ofid2key/key2idindebugging_servicedidn't actually guard against concurrent races.Transaction(fn, watchKeys ...string). The varargs default tonil, so existingTransaction(fn)call sites are byte-level backward compatible.serialKey()to apply the prefix before handing off to go-redis, matching other helpers.debugging_service/connection_key.goto the new signature and WATCH onid2key: concurrent creation of a connection key for the same tenant now relies on WATCH retry for correctness instead of leaning on the innerSetNX's NX semantics.Bundled helper extraction:
pkg/utils/parser/csv.goSplitAndTrimCSV("a, b , ,c"→["a", "b", "c"], empty string returnsnil), withcsv_test.gocovering the edge cases.plugin_manager/manager.goswitched fromstrings.Split(configuration.RedisSentinels, ",")to this helper, so accidental whitespace inREDIS_SENTINELSis parsed correctly.Type of Change
Transaction)Essential Checklist
Testing
go build ./internal/... ./pkg/...passes; new unit tests inparser/csv_test.goare green; existingcache/debugging_servicetests did not regress.debugging_serviceintegration smoke test (local Redis Sentinel) is pending.Bug Fix (if applicable)
Additional Information
The variadic change is source-compatible — no existing
Transaction(fn)call site needs to change. The newwatchKeysparameter is optional; callers that want WATCH just pass keys positionally.