fix(auth): non-blocking reload check, stat-before-load, mtime inequality trigger

- users_file.go: maybeReload uses reloadMu.TryLock() instead of Lock().
  A full reload (stat+read+parse+replaceFileUsers, incl. per-user
  resolveSecret file reads) could otherwise block every concurrent
  ServeHTTP request for its whole duration, negating the buildMu/mu
  lock split. A request that finds a reload in flight now skips this
  cycle; reload stays throttled and best-effort.
- users_file.go: mtime check changed from ModTime().After(lastModTime)
  to ModTime().Equal(lastModTime), so a backward mtime move (restore
  from backup, cp -p) still triggers a reload instead of being
  skipped. Added a regression test with os.Chtimes into the past.
- llmgateway.go: attachUsersFile now stats the file before loading it,
  matching maybeReload's ordering, so a write landing between stat and
  load is caught on the next check instead of being lost. Documented
  why the ignored statErr is safe (falls back to zero lastModTime,
  causing one harmless extra reload).
This commit is contained in:
2026-08-20 01:16:13 +01:00
parent eef33b8232
commit 15bbf51deb
3 changed files with 58 additions and 10 deletions
+13 -1
View File
@@ -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()
+23 -9
View File
@@ -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
}
+22
View File
@@ -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"}})