From 5dec954632a7eeff35ffabe6cb799ec811a3a48a Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 14 Aug 2026 00:00:36 +0100 Subject: [PATCH] fix(api): hold bannedUsersMu across reload and release flock on all paths The read-error path in loadBannedUsers returned while it held the shared file lock. One transient read failure blocked every later ban write for the process lifetime. The reload also applied a stale snapshot over a ban that landed between the read and the replace. Acquire bannedUsersMu first, then the file lock, for the whole read and replace. Release both locks with defer on every path. A deterministic test proves the lock is released after a read error. --- api.go | 52 ++++++++++++++++++++++++++++++------------ api_additional_test.go | 36 +++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 15 deletions(-) diff --git a/api.go b/api.go index 9a5f9c4..9b1124f 100644 --- a/api.go +++ b/api.go @@ -21,9 +21,11 @@ var bannedUsersIDs sync.Map // key: userID string, value: reason string // bannedUsersMu serializes in-memory banned-set mutations (admin Store/Delete) // against the periodic file reload's clear-then-rebuild, and keeps the file -// write atomic with the map change. Lock ordering: always acquire -// bannedUsersMu BEFORE the file lock; loadBannedUsers releases its file read -// lock before applying the snapshot to honour that order and avoid deadlock. +// read/write atomic with the map change. Lock ordering: always acquire +// bannedUsersMu BEFORE the file lock. loadBannedUsers holds bannedUsersMu for +// its whole read+replace, so a reload can never apply a stale snapshot over a +// write that apiBanUser/apiUnbanUser made in between, and it matches the +// mutex-then-file order those handlers use around storeBannedUsers. var bannedUsersMu sync.Mutex // authMiddleware provides API key authentication for admin endpoints @@ -353,6 +355,22 @@ func loadBannedUsers() { } } + // Hold bannedUsersMu across the file read AND the in-memory replace below. + // apiBanUser/apiUnbanUser also acquire bannedUsersMu before their file + // write, so this keeps the documented mutex-before-file-lock order and + // stops a write landing between our read and our replace from being + // overwritten by this reload's now-stale snapshot. + // + // This holds bannedUsersMu across lockFileRead below, which can block for + // up to 30s under cross-process flock contention (see lockFileRead's own + // timeout). Because apiBanUser/apiUnbanUser take the same mutex, an admin + // ban/unban request can queue behind a reload for that long. This is an + // accepted trade: it buys atomicity between the in-memory map and the + // on-disk file, and reloads are infrequent enough that the worst case is + // rare in practice. + bannedUsersMu.Lock() + defer bannedUsersMu.Unlock() + fileLock := flock.New(fmt.Sprintf("%s.lock", cfg.Api.BannedUsersFile)) if err := lockFileRead(fileLock); err != nil { cfg.Logger.Error(&libpack_logger.LogMessage{ @@ -361,6 +379,15 @@ func loadBannedUsers() { }) return } + defer func() { + if err := fileLock.Unlock(); err != nil { + cfg.Logger.Error(&libpack_logger.LogMessage{ + Message: "Failed to unlock file", + Pairs: map[string]any{"error": err.Error()}, + }) + } + }() + data, err := os.ReadFile(cfg.Api.BannedUsersFile) if err != nil { cfg.Logger.Error(&libpack_logger.LogMessage{ @@ -370,17 +397,6 @@ func loadBannedUsers() { return } - // Release the read lock BEFORE mutating the in-memory set. replaceBannedUsers - // takes bannedUsersMu; acquiring the file read lock and bannedUsersMu in the - // file->mutex order would invert the mutex->file order used by the admin - // write path (apiBanUser/apiUnbanUser) and could deadlock. - if err := fileLock.Unlock(); err != nil { - cfg.Logger.Error(&libpack_logger.LogMessage{ - Message: "Failed to unlock file", - Pairs: map[string]any{"error": err.Error()}, - }) - } - var newBannedUsers map[string]string if err := json.Unmarshal(data, &newBannedUsers); err != nil { cfg.Logger.Error(&libpack_logger.LogMessage{ @@ -390,7 +406,7 @@ func loadBannedUsers() { return } - replaceBannedUsers(newBannedUsers) + replaceBannedUsersLocked(newBannedUsers) } // snapshotBannedUsers returns a plain map copy of the current banned users. @@ -414,6 +430,12 @@ func replaceBannedUsers(newUsers map[string]string) { // (apiBanUser/apiUnbanUser) or a reload can clobber a just-applied ban. bannedUsersMu.Lock() defer bannedUsersMu.Unlock() + replaceBannedUsersLocked(newUsers) +} + +// replaceBannedUsersLocked does the work of replaceBannedUsers without +// acquiring bannedUsersMu. Callers must already hold bannedUsersMu. +func replaceBannedUsersLocked(newUsers map[string]string) { bannedUsersIDs.Range(func(k, _ any) bool { bannedUsersIDs.Delete(k) return true diff --git a/api_additional_test.go b/api_additional_test.go index 92bbb62..0292112 100644 --- a/api_additional_test.go +++ b/api_additional_test.go @@ -8,6 +8,7 @@ import ( "sync" "testing" + "github.com/gofrs/flock" libpack_logger "github.com/lukaszraczylo/graphql-monitoring-proxy/logging" "github.com/stretchr/testify/assert" ) @@ -262,3 +263,38 @@ func TestBannedUsersConcurrentModifyAndReload(t *testing.T) { t.Fatal("kept-user ban was lost after concurrent modify + reload") } } + +// TestLoadBannedUsersReleasesLockOnReadError is a regression test for the +// file-lock leak: a read error inside loadBannedUsers (after the shared file +// lock is taken) must still release the lock. Before the fix, the error +// return at the os.ReadFile check skipped the unlock, so the flock's open fd +// stayed shared-locked for the process lifetime, and every later +// storeBannedUsers (which needs the exclusive lock) would block and fail. +func TestLoadBannedUsersReleasesLockOnReadError(t *testing.T) { + dir := t.TempDir() + // A directory in place of the banned-users file makes os.Stat report the + // path exists (skipping the create-file branch) while os.ReadFile fails. + file := filepath.Join(dir, "banned.json") + if err := os.Mkdir(file, 0o755); err != nil { + t.Fatal(err) + } + + origCfg := cfg + cfg = &config{Logger: libpack_logger.New()} + cfg.Api.BannedUsersFile = file + defer func() { cfg = origCfg }() + + loadBannedUsers() // must hit the os.ReadFile error path and return + + // If the shared read lock leaked, a fresh non-blocking exclusive TryLock + // on the same lock file fails instead of succeeding immediately. + fileLock := flock.New(fmt.Sprintf("%s.lock", file)) + locked, err := fileLock.TryLock() + if err != nil { + t.Fatalf("TryLock returned an error: %v", err) + } + if !locked { + t.Fatal("exclusive lock unavailable after loadBannedUsers read error - shared lock leaked") + } + _ = fileLock.Unlock() +}