Skip to content

Commit b8fbe70

Browse files
committed
fix(auth): stop old selectors when replacing manager selector and harden cache stop concurrency
- Add `isSameSelector` using type-aware comparable checks to avoid unnecessary selector replacement. - Update `Manager.SetSelector` to: - serialize swaps with a dedicated selector mutex, - no-op when replacing with the same selector instance/type, - stop the previous selector when it implements `StoppableSelector`. - Protect `SessionCache.Stop()` with `sync.Once` and nil-check to make repeated/concurrent stops safe and idempotent. Closes: router-for-me#5018
1 parent 75e2454 commit b8fbe70

5 files changed

Lines changed: 202 additions & 8 deletions

File tree

sdk/cliproxy/auth/conductor.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ type Manager struct {
113113
selector Selector
114114
hook Hook
115115
mu sync.RWMutex
116+
selectorMu sync.Mutex
116117
configCooldownMu sync.Mutex
117118
auths map[string]*Auth
118119
scheduler *authScheduler

sdk/cliproxy/auth/conductor_selection.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"math/rand/v2"
77
"net/http"
8+
"reflect"
89
"sort"
910
"strings"
1011
"time"
@@ -234,16 +235,44 @@ func (m *Manager) ReconcileRegistryModelStates(ctx context.Context, authID strin
234235
}
235236
}
236237

238+
func isSameSelector(a, b Selector) bool {
239+
if a == nil || b == nil {
240+
return a == nil && b == nil
241+
}
242+
ta, tb := reflect.TypeOf(a), reflect.TypeOf(b)
243+
if ta != tb {
244+
return false
245+
}
246+
if ta.Comparable() {
247+
return a == b
248+
}
249+
return false
250+
}
251+
237252
func (m *Manager) SetSelector(selector Selector) {
238253
if m == nil {
239254
return
240255
}
241256
if selector == nil {
242257
selector = &RoundRobinSelector{}
243258
}
259+
m.selectorMu.Lock()
260+
defer m.selectorMu.Unlock()
261+
244262
m.mu.Lock()
263+
oldSelector := m.selector
264+
if isSameSelector(oldSelector, selector) {
265+
m.mu.Unlock()
266+
return
267+
}
245268
m.selector = selector
246269
m.mu.Unlock()
270+
271+
if oldSelector != nil {
272+
if stoppable, ok := oldSelector.(StoppableSelector); ok {
273+
stoppable.Stop()
274+
}
275+
}
247276
if m.scheduler != nil {
248277
m.scheduler.setSelector(selector)
249278
m.syncScheduler()

sdk/cliproxy/auth/selector_test.go

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2200,3 +2200,130 @@ func TestSessionAffinitySelectorUsesRequestPayloadWhenOriginalRequestMissing(t *
22002200
t.Fatalf("request-only conversation changed auth from %q to %q", first.ID, second.ID)
22012201
}
22022202
}
2203+
2204+
func TestSessionCache_StopConcurrent(t *testing.T) {
2205+
t.Parallel()
2206+
for iter := 0; iter < 100; iter++ {
2207+
cache := NewSessionCache(time.Minute)
2208+
var wg sync.WaitGroup
2209+
for i := 0; i < 20; i++ {
2210+
wg.Add(1)
2211+
go func() {
2212+
defer wg.Done()
2213+
cache.Stop()
2214+
}()
2215+
}
2216+
wg.Wait()
2217+
}
2218+
}
2219+
2220+
type mockStoppableSelector struct {
2221+
stopped bool
2222+
}
2223+
2224+
func (m *mockStoppableSelector) Pick(ctx context.Context, provider, model string, opts cliproxyexecutor.Options, auths []*Auth) (*Auth, error) {
2225+
return nil, nil
2226+
}
2227+
2228+
func (m *mockStoppableSelector) Stop() {
2229+
m.stopped = true
2230+
}
2231+
2232+
func TestManagerSetSelectorStopsReplacedStoppableSelector(t *testing.T) {
2233+
t.Parallel()
2234+
mockSelector := &mockStoppableSelector{}
2235+
manager := NewManager(nil, mockSelector, nil)
2236+
2237+
manager.SetSelector(&RoundRobinSelector{})
2238+
2239+
if !mockSelector.stopped {
2240+
t.Fatal("expected previous StoppableSelector to be stopped when replaced via SetSelector")
2241+
}
2242+
}
2243+
2244+
type zeroSizeSelectorA struct {
2245+
stopped *bool
2246+
}
2247+
2248+
func (z zeroSizeSelectorA) Pick(ctx context.Context, provider, model string, opts cliproxyexecutor.Options, auths []*Auth) (*Auth, error) {
2249+
return nil, nil
2250+
}
2251+
2252+
func (z zeroSizeSelectorA) Stop() {
2253+
if z.stopped != nil {
2254+
*z.stopped = true
2255+
}
2256+
}
2257+
2258+
type zeroSizeSelectorB struct{}
2259+
2260+
func (z zeroSizeSelectorB) Pick(ctx context.Context, provider, model string, opts cliproxyexecutor.Options, auths []*Auth) (*Auth, error) {
2261+
return nil, nil
2262+
}
2263+
2264+
func TestManagerSetSelectorDifferentZeroSizedSelectors(t *testing.T) {
2265+
t.Parallel()
2266+
stoppedA := false
2267+
selA := zeroSizeSelectorA{stopped: &stoppedA}
2268+
selB := zeroSizeSelectorB{}
2269+
2270+
manager := NewManager(nil, selA, nil)
2271+
manager.SetSelector(selB)
2272+
2273+
if !stoppedA {
2274+
t.Fatal("expected zeroSizeSelectorA to be stopped when replaced by zeroSizeSelectorB")
2275+
}
2276+
if manager.Selector() != selB {
2277+
t.Fatalf("expected manager selector to be selB, got %#v", manager.Selector())
2278+
}
2279+
}
2280+
2281+
type uncomparableSelector struct {
2282+
fn func()
2283+
}
2284+
2285+
func (u uncomparableSelector) Pick(ctx context.Context, provider, model string, opts cliproxyexecutor.Options, auths []*Auth) (*Auth, error) {
2286+
return nil, nil
2287+
}
2288+
2289+
func TestManagerSetSelectorUncomparableTypes(t *testing.T) {
2290+
t.Parallel()
2291+
manager := NewManager(nil, nil, nil)
2292+
2293+
sel1 := uncomparableSelector{fn: func() {}}
2294+
sel2 := uncomparableSelector{fn: func() {}}
2295+
2296+
// Setting uncomparable types must not panic
2297+
manager.SetSelector(sel1)
2298+
manager.SetSelector(sel2)
2299+
manager.SetSelector(nil)
2300+
}
2301+
2302+
func TestManagerSetSelectorSameInstanceDoesNotStop(t *testing.T) {
2303+
t.Parallel()
2304+
mockSelector := &mockStoppableSelector{}
2305+
manager := NewManager(nil, mockSelector, nil)
2306+
2307+
// Setting the same instance should be a no-op and not call Stop
2308+
manager.SetSelector(mockSelector)
2309+
if mockSelector.stopped {
2310+
t.Fatal("setting the same selector instance unexpectedly called Stop")
2311+
}
2312+
}
2313+
2314+
func TestManagerSetSelectorConcurrent(t *testing.T) {
2315+
t.Parallel()
2316+
manager := NewManager(nil, nil, nil)
2317+
var wg sync.WaitGroup
2318+
for i := 0; i < 20; i++ {
2319+
wg.Add(1)
2320+
go func() {
2321+
defer wg.Done()
2322+
for j := 0; j < 10; j++ {
2323+
sel := &mockStoppableSelector{}
2324+
manager.SetSelector(sel)
2325+
}
2326+
}()
2327+
}
2328+
wg.Wait()
2329+
}

sdk/cliproxy/auth/session_cache.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,11 @@ type sessionEntry struct {
1717

1818
// SessionCache provides TTL-based session to auth mapping with automatic cleanup.
1919
type SessionCache struct {
20-
mu sync.RWMutex
21-
entries map[string]sessionEntry
22-
ttl time.Duration
23-
stopCh chan struct{}
20+
mu sync.RWMutex
21+
entries map[string]sessionEntry
22+
ttl time.Duration
23+
stopCh chan struct{}
24+
stopOnce sync.Once
2425
}
2526

2627
// NewSessionCache creates a cache with the specified TTL.
@@ -319,11 +320,12 @@ func (c *SessionCache) InvalidateAuth(authID string) {
319320

320321
// Stop terminates the background cleanup goroutine.
321322
func (c *SessionCache) Stop() {
322-
select {
323-
case <-c.stopCh:
324-
default:
325-
close(c.stopCh)
323+
if c == nil {
324+
return
326325
}
326+
c.stopOnce.Do(func() {
327+
close(c.stopCh)
328+
})
327329
}
328330

329331
func (c *SessionCache) cleanupLoop() {

sdk/cliproxy/service_config_weight_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package cliproxy
22

33
import (
4+
"context"
45
"testing"
56

67
internalconfig "github.com/router-for-me/CLIProxyAPI/v7/internal/config"
78
coreauth "github.com/router-for-me/CLIProxyAPI/v7/sdk/cliproxy/auth"
9+
cliproxyexecutor "github.com/router-for-me/CLIProxyAPI/v7/sdk/cliproxy/executor"
810
)
911

1012
func TestWeightedRoundRobinRoutingSelector(t *testing.T) {
@@ -40,3 +42,36 @@ func TestServiceRejectsInvalidCredentialWeightConfigCommit(t *testing.T) {
4042
t.Fatalf("config sequence = %d, want 0", service.configSequence)
4143
}
4244
}
45+
46+
type trackingStoppableSelector struct {
47+
stopped bool
48+
}
49+
50+
func (s *trackingStoppableSelector) Pick(ctx context.Context, provider, model string, opts cliproxyexecutor.Options, auths []*coreauth.Auth) (*coreauth.Auth, error) {
51+
return nil, nil
52+
}
53+
54+
func (s *trackingStoppableSelector) Stop() {
55+
s.stopped = true
56+
}
57+
58+
func TestApplyManagerConfigStopsReplacedServiceAffinitySelector(t *testing.T) {
59+
tracking := &trackingStoppableSelector{}
60+
service := &Service{
61+
coreManager: coreauth.NewManager(nil, tracking, nil),
62+
}
63+
64+
newCfg := &internalconfig.Config{
65+
Routing: internalconfig.RoutingConfig{
66+
Strategy: "round-robin",
67+
},
68+
}
69+
commit := configCommit{cfg: newCfg, sequence: 1}
70+
if !service.applyManagerConfig(context.Background(), commit) {
71+
t.Fatal("applyManagerConfig failed")
72+
}
73+
74+
if !tracking.stopped {
75+
t.Fatal("expected replaced selector to be stopped during routing config apply")
76+
}
77+
}

0 commit comments

Comments
 (0)