diff --git a/auth.go b/auth.go index 3c1e97f..3f3d02d 100644 --- a/auth.go +++ b/auth.go @@ -83,11 +83,19 @@ type authEntry struct { // authStore identifies requests by API key and resolves them to a user and // their group. Raw API keys are never retained after construction — only // their SHA-256 digests. +// +// mu guards byDigest for readers (identify) and the final swap in +// replaceFileUsers. buildMu serializes the whole body of replaceFileUsers — +// including buildEntry's potentially slow file-backed secret resolution — +// so concurrent reload attempts never interleave or overwrite each other. +// buildMu is never held at the same time as mu, so a rebuild in progress +// never blocks identify's readers. type authStore struct { groups map[string]*group inline map[[32]byte]*authEntry // built once by newAuthStore; never mutated afterward byDigest map[[32]byte]*authEntry // inline entries plus the current file-sourced set; guarded by mu mu sync.RWMutex + buildMu sync.Mutex } // newAuthStore builds an authStore from cfg's groups and inline users. It @@ -158,14 +166,16 @@ func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) { // On error the store is left exactly as it was before the call — the new // set is validated and built in full before it replaces the old one. // -// mu is held for the whole rebuild, not just the final swap. That serializes -// concurrent callers (a reload timer must never race itself), so one call's -// result can never be partially overwritten or interleaved with another's. -// identify's reads stay fast (RLock) and reloads are rare, so the extra hold -// time is a good trade. +// buildMu is held for the whole rebuild, not just the final swap — that +// serializes concurrent callers (a reload timer must never race itself), so +// one call's result can never be partially overwritten or interleaved with +// another's. mu, which identify's reads use (RLock), is locked only for the +// final map swap below: a slow rebuild (buildEntry's resolveSecret can do +// blocking file I/O for "file:"-prefixed API keys) no longer stalls every +// identify call for its whole duration. func (a *authStore) replaceFileUsers(us []*UserConfig) error { - a.mu.Lock() - defer a.mu.Unlock() + a.buildMu.Lock() + defer a.buildMu.Unlock() next := make(map[[32]byte]*authEntry, len(a.inline)+len(us)) for digest, entry := range a.inline { @@ -182,7 +192,9 @@ func (a *authStore) replaceFileUsers(us []*UserConfig) error { next[entry.digest] = entry } + a.mu.Lock() a.byDigest = next + a.mu.Unlock() return nil } diff --git a/auth_test.go b/auth_test.go index c53d799..d5c6184 100644 --- a/auth_test.go +++ b/auth_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "sync" "testing" + "time" ) // TestIdentify is the brief's Step-1 table: bearer auth resolves the right @@ -374,3 +375,38 @@ func TestAuthStore_ReplaceFileUsers_ConcurrentWritersAndReaders(t *testing.T) { t.Fatalf("want exactly one file-user batch to survive concurrent swaps (size %d), got size %d — a rebuild was lost or merged with another", want, got) } } + +// TestAuthStore_Identify_NotBlockedByBuildMu is the regression for the +// Task-4 lock split: replaceFileUsers's writer-serialization lock (buildMu) +// must be independent from the reader lock (mu) that identify uses. The +// prior implementation held mu for the whole rebuild, which would stall +// every identify call for as long as a reload's build phase (including +// resolveSecret's potentially slow file-backed reads) took. Holding buildMu +// externally here simulates "a rebuild's build phase is in progress" — a +// correct identify must not need buildMu at all. +func TestAuthStore_Identify_NotBlockedByBuildMu(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + a.buildMu.Lock() + defer a.buildMu.Unlock() + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.Header.Set("Authorization", "Bearer sk-secret") + + done := make(chan bool, 1) + go func() { + _, _, ok := a.identify(r) + done <- ok + }() + + select { + case ok := <-done: + if !ok { + t.Fatal("want identify to succeed") + } + case <-time.After(2 * time.Second): + t.Fatal("identify blocked while buildMu is held — reader lock must not depend on the writer-serialization lock") + } +}