fix(api): make banned-users store atomic against periodic reload

bannedUsersIDs (sync.Map) was modified by admin Store/Delete (apiBanUser/
apiUnbanUser) and by loadBannedUsers->replaceBannedUsers, which does a
clear-then-rebuild (Range-Delete then Store). The flock only serialized
FILE I/O, not the in-memory map, so a reload could clear a just-applied
ban before storeBannedUsers snapshotted it (or persist the wiped set to
disk).

Add bannedUsersMu (mutex) held across both the map update and the file
write in the admin path, and across the clear-then-rebuild in
replaceBannedUsers. Lock ordering: bannedUsersMu before the file lock;
loadBannedUsers now releases its file read lock before applying the
snapshot, so no ordering inversion and no deadlock.

Also harden the connection-pool health type assertion against a missing
map key (apiConnectionPoolHealth).

Test: TestBannedUsersConcurrentModifyAndReload (concurrent modify +
reload must not lose a persisted ban; race-free under -race).
This commit is contained in:
2026-08-13 04:45:28 +01:00
parent 2c9b0a499f
commit 9fcd324dc8
2 changed files with 87 additions and 12 deletions
+53
View File
@@ -5,6 +5,8 @@ import (
"fmt"
"os"
"path/filepath"
"sync"
"testing"
libpack_logger "github.com/lukaszraczylo/graphql-monitoring-proxy/logging"
"github.com/stretchr/testify/assert"
@@ -209,3 +211,54 @@ func (suite *Tests) Test_LoadUnloadBannedUsers() {
assert.False(suite.T(), user1Exists)
})
}
func TestBannedUsersConcurrentModifyAndReload(t *testing.T) {
// Regression for the banned-users race: an admin ban (bannedUsersIDs.Store
// + storeBannedUsers) must be atomic against periodic reloads
// (loadBannedUsers -> replaceBannedUsers). Concurrent modify+reload must
// not lose a persisted ban and must be memory-race free (-race).
dir := t.TempDir()
file := filepath.Join(dir, "banned.json")
origCfg := cfg
cfg = &config{Logger: libpack_logger.New()}
cfg.Api.BannedUsersFile = file
defer func() { cfg = origCfg }()
bannedUsersIDs = sync.Map{}
if err := os.WriteFile(file, []byte("{}"), 0o644); err != nil {
t.Fatal(err)
}
var wg sync.WaitGroup
// Admin-like writers: ban kept-user and persist, under bannedUsersMu.
for range 4 {
wg.Add(1)
go func() {
defer wg.Done()
for range 50 {
bannedUsersMu.Lock()
bannedUsersIDs.Store("kept-user", "reason")
_ = storeBannedUsers()
bannedUsersMu.Unlock()
}
}()
}
// Reloading goroutines.
for range 2 {
wg.Add(1)
go func() {
defer wg.Done()
for range 50 {
loadBannedUsers()
}
}()
}
wg.Wait()
// A final reload must reflect the persisted (still-banned) user.
loadBannedUsers()
if _, ok := bannedUsersIDs.Load("kept-user"); !ok {
t.Fatal("kept-user ban was lost after concurrent modify + reload")
}
}