From 66adf53832399eb9b6ce3097dcb415e6548ac9eb Mon Sep 17 00:00:00 2001 From: Kuchizu Date: Sat, 2 May 2026 00:19:13 +0300 Subject: [PATCH 1/3] fix(redis): load full blocked set, fix IsBlocked race --- pkg/redis/client.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/redis/client.go b/pkg/redis/client.go index 727f60d..236db3e 100644 --- a/pkg/redis/client.go +++ b/pkg/redis/client.go @@ -13,7 +13,7 @@ import ( ) type RedisClient struct { - mx sync.Mutex + mx sync.RWMutex rdb *redis.Client blockedIDsSetName string allIPsMapName string @@ -107,9 +107,9 @@ func (r *RedisClient) load() error { log.Errorf("Failed to SCARD blocked IDs set %q: %s", r.blockedIDsSetName, cardErr) } - keys, _, err := r.rdb.SScan(r.ctx, r.blockedIDsSetName, 0, "", 0).Result() + keys, err := r.rdb.SMembers(r.ctx, r.blockedIDsSetName).Result() if err != nil { - log.Errorf("Failed to SScan blocked IDs set %q: %s", r.blockedIDsSetName, err) + log.Errorf("Failed to SMembers blocked IDs set %q: %s", r.blockedIDsSetName, err) return err } @@ -122,7 +122,7 @@ func (r *RedisClient) load() error { } } if cardErr == nil && int64(len(keys)) != cardinality { - log.Warnf("SScan returned %d entries but SCARD reports %d for key %q — set may be larger than one SScan batch", len(keys), cardinality, r.blockedIDsSetName) + log.Warnf("SMembers returned %d entries but SCARD reports %d for key %q (concurrent modification?)", len(keys), cardinality, r.blockedIDsSetName) } if len(keys) == 0 && prevCount > 0 { log.Warnf("Blocked projects list is now empty (was %d). Key %q may have been cleared", prevCount, r.blockedIDsSetName) @@ -178,6 +178,8 @@ func (r *RedisClient) updateBlacklist() ([]string, []string, error) { // IsBlocked checks if the provided ID is blocked. func (r *RedisClient) IsBlocked(val string) bool { + r.mx.RLock() + defer r.mx.RUnlock() for _, id := range r.blockedIDs { if id == val { log.Debugf("IsBlocked: project %q matched in cache (size=%d)", val, len(r.blockedIDs)) @@ -200,6 +202,8 @@ func (r *RedisClient) IncrementIP(ip string) error { // CheckBlacklist checks if the provided IP is in blacklist. func (r *RedisClient) CheckBlacklist(ip string) bool { + r.mx.RLock() + defer r.mx.RUnlock() if len(r.blacklistIPs) == 0 { return false } From 8612b458de160c5ee4bcbbc4094fae6c37d308c9 Mon Sep 17 00:00:00 2001 From: Kuchizu Date: Sat, 2 May 2026 00:38:04 +0300 Subject: [PATCH 2/3] refactor(redis): minimize lock hold time and add load test --- pkg/redis/client.go | 14 +++++++------- pkg/redis/client_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/pkg/redis/client.go b/pkg/redis/client.go index 236db3e..5e22c20 100644 --- a/pkg/redis/client.go +++ b/pkg/redis/client.go @@ -92,9 +92,6 @@ func (r *RedisClient) LoadBlacklist() ([]string, []string, error) { // loads list with provided name from Redis. func (r *RedisClient) load() error { - r.mx.Lock() - defer r.mx.Unlock() - exists, existsErr := r.rdb.Exists(r.ctx, r.blockedIDsSetName).Result() if existsErr != nil { log.Errorf("Failed to check existence of blocked IDs set %q: %s", r.blockedIDsSetName, existsErr) @@ -113,7 +110,11 @@ func (r *RedisClient) load() error { return err } + r.mx.Lock() prevCount := len(r.blockedIDs) + r.blockedIDs = keys + r.mx.Unlock() + if len(keys) != prevCount { if cardErr == nil { log.Infof("Banned projects list updated %d -> %d (key=%q, scard=%d)", prevCount, len(keys), r.blockedIDsSetName, cardinality) @@ -139,15 +140,11 @@ func (r *RedisClient) load() error { log.Debugf("Loaded blocked project IDs from %q (count=%d, sample=%v)", r.blockedIDsSetName, len(keys), sample) } - r.blockedIDs = keys return nil } // updateBlacklist loads IPs blacklist and resets current period map. func (r *RedisClient) updateBlacklist() ([]string, []string, error) { - r.mx.Lock() - defer r.mx.Unlock() - ipAddrs, err := r.rdb.HKeys(r.ctx, r.currentPeriodMapName).Result() if err != nil { return nil, nil, err @@ -157,7 +154,10 @@ func (r *RedisClient) updateBlacklist() ([]string, []string, error) { if err != nil { return nil, nil, err } + + r.mx.Lock() r.blacklistIPs = ips + r.mx.Unlock() if len(ipAddrs) > 0 { requests, err := r.rdb.HVals(r.ctx, r.currentPeriodMapName).Result() diff --git a/pkg/redis/client_test.go b/pkg/redis/client_test.go index 2a11d2b..e8e659b 100644 --- a/pkg/redis/client_test.go +++ b/pkg/redis/client_test.go @@ -136,6 +136,30 @@ func TestUpdateRateLimit(t *testing.T) { } } +// Regression: load() must return all entries for sets larger than one SScan batch. +func TestLoadBlockedIDsLargeSet(t *testing.T) { + client, mr := setupTestRedis(t) + defer mr.Close() + + client.blockedIDsSetName = "DisabledProjectsSet" + + const total = 100 + expected := make([]string, 0, total) + for i := 0; i < total; i++ { + id := fmt.Sprintf("%024x", i) + client.rdb.SAdd(client.ctx, client.blockedIDsSetName, id) + expected = append(expected, id) + } + + err := client.load() + assert.NoError(t, err) + + for _, id := range expected { + assert.True(t, client.IsBlocked(id), "expected %q to be reported as blocked", id) + } + assert.False(t, client.IsBlocked("not-in-set")) +} + func TestUpdateRateLimitConcurrent(t *testing.T) { client, mr := setupTestRedis(t) defer mr.Close() From a5fb14aa958ff923f8b787410fdff0c6249e892d Mon Sep 17 00:00:00 2001 From: Kuchizu <70284260+Kuchizu@users.noreply.github.com> Date: Sat, 2 May 2026 00:45:10 +0300 Subject: [PATCH 3/3] Update pkg/redis/client_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/redis/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/redis/client_test.go b/pkg/redis/client_test.go index e8e659b..ccf3993 100644 --- a/pkg/redis/client_test.go +++ b/pkg/redis/client_test.go @@ -136,7 +136,7 @@ func TestUpdateRateLimit(t *testing.T) { } } -// Regression: load() must return all entries for sets larger than one SScan batch. +// Regression: load() must return all entries from large blocked ID sets. func TestLoadBlockedIDsLargeSet(t *testing.T) { client, mr := setupTestRedis(t) defer mr.Close()