diff --git a/llmgateway.go b/llmgateway.go index 1f16601..fac0c5b 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -143,6 +143,18 @@ func newGateway(ctx context.Context, next http.Handler, config *Config, name str // constructor error — fail fast on bad config rather than start with an // empty file-sourced user set. func attachUsersFile(auth *authStore, path string, log gatewayLogger) error { + // Stat before load, matching maybeReload's ordering: capturing the mtime + // before reading content means a write landing between the stat and the + // read still shows up as a further mtime change on the first post- + // startup reload check, instead of being silently lost forever — a + // stat-after-load can observe a newer mtime for content it never read. + // + // statErr is intentionally ignored here: if the stat fails, lastModTime + // stays at its zero value, so the first maybeReload call sees any real + // mtime as "changed" and reloads once more — a harmless extra reload of + // content already loaded, not a correctness problem. + info, statErr := os.Stat(path) + uf := newUsersFile(path) initial, err := uf.load() if err != nil { @@ -154,7 +166,7 @@ func attachUsersFile(auth *authStore, path string, log gatewayLogger) error { auth.usersFile = uf auth.log = log - if info, statErr := os.Stat(path); statErr == nil { + if statErr == nil { auth.lastModTime = info.ModTime() } auth.lastCheck = auth.nowFn() diff --git a/users_file.go b/users_file.go index 0c0f172..2b46731 100644 --- a/users_file.go +++ b/users_file.go @@ -53,19 +53,29 @@ type gatewayLogger interface { } // maybeReload reloads a's file-sourced users when a.usersFile is configured, -// reloadEvery has elapsed since the last check, and the file's mtime has -// changed since the last successful load. It runs on the request path -// (ServeHTTP calls it at entry), so it must stay cheap when nothing changed -// and must never panic: on any error — stat, load, or replaceFileUsers -// rejecting the new set — it logs via a.log.errorf and keeps the last good -// set. A successful reload that changes the file-sourced user count logs via -// a.log.logf. +// reloadEvery has elapsed since the last check, and the file's mtime differs +// from the last observed mtime (forward or backward). It runs on the request +// path (ServeHTTP calls it at entry), so it must stay cheap when nothing +// changed and must never panic: on any error — stat, load, or +// replaceFileUsers rejecting the new set — it logs via a.log.errorf and +// keeps the last good set. A successful reload that changes the +// file-sourced user count logs via a.log.logf. func (a *authStore) maybeReload() { if a.usersFile == nil { return } - a.reloadMu.Lock() + // TryLock, not Lock: ServeHTTP calls maybeReload on every request, and a + // reload's build phase can block on file I/O (resolveSecret's "file:" + // reads inside replaceFileUsers). Blocking every concurrent request + // behind one full reload would undo the auth.go buildMu/mu lock split's + // benefit one layer up. A request that finds a reload already in flight + // just skips this cycle — the next request's check (or the next + // throttle window) covers it. Reload is deliberately best-effort and + // throttled, not "every change lands on the very next request." + if !a.reloadMu.TryLock() { + return + } defer a.reloadMu.Unlock() now := a.nowFn() @@ -79,7 +89,11 @@ func (a *authStore) maybeReload() { a.log.errorf("llmgateway: cannot stat users file %q: %v", a.usersFile.path, err) return } - if !info.ModTime().After(a.lastModTime) { + // Equal, not After: an After-only check misses a backward mtime move — + // a restore from backup, or a `cp -p` from an older file. Any + // difference from the last observed mtime, forward or backward, must + // trigger a reload; only an unchanged mtime skips it. + if info.ModTime().Equal(a.lastModTime) { return } diff --git a/users_file_test.go b/users_file_test.go index c7a6078..831d34c 100644 --- a/users_file_test.go +++ b/users_file_test.go @@ -271,6 +271,28 @@ func TestAuthStore_MaybeReload_UnchangedMtime_DoesNotReload(t *testing.T) { } } +// TestAuthStore_MaybeReload_BackwardMtimeMove_TriggersReload is the +// regression for the Equal-vs-After mtime check: a file restored from a +// backup, or copied with `cp -p` from an older file, can move the mtime +// backward. That must still trigger a reload — only an mtime equal to the +// last observed one may skip it. +func TestAuthStore_MaybeReload_BackwardMtimeMove_TriggersReload(t *testing.T) { + a, fp, _, clock := newReloadableAuthStore(t, nil, []*UserConfig{{Name: "f1", Group: "eng", APIKey: "sk-f1"}}) + + past := time.Now().Add(-time.Hour) + writeUsersDoc(t, fp, []*UserConfig{{Name: "f2", Group: "eng", APIKey: "sk-f2"}}, past) + + clock.Advance(reloadEvery + time.Second) + a.maybeReload() + + if _, _, ok := identifyWithKey(a, "sk-f2"); !ok { + t.Fatal("want a backward mtime move to still trigger a reload") + } + if _, _, ok := identifyWithKey(a, "sk-f1"); ok { + t.Fatal("want f1 no longer identifiable once the backward-mtime reload replaced the file-sourced set") + } +} + func TestAuthStore_MaybeReload_UserCountChange_LogsViaLogf(t *testing.T) { a, fp, log, clock := newReloadableAuthStore(t, nil, []*UserConfig{{Name: "f1", Group: "eng", APIKey: "sk-f1"}})