diff --git a/internal/pluginhost/adapters_executors.go b/internal/pluginhost/adapters_executors.go index 3f43edf0c6b..c3aacc88caa 100644 --- a/internal/pluginhost/adapters_executors.go +++ b/internal/pluginhost/adapters_executors.go @@ -179,6 +179,9 @@ func (h *Host) commitExecutorState(snap *Snapshot, manager executorManager, mode } h.executorModelClientIDs = nextModelClients + h.mu.Unlock() + + // The manager takes its own lock, so it is entered only after h.mu is released. for _, registration := range registrations { if registration.adapter == nil || registration.provider == "" { continue @@ -192,7 +195,6 @@ func (h *Host) commitExecutorState(snap *Snapshot, manager executorManager, mode } manager.UnregisterExecutor(provider) } - h.mu.Unlock() if modelRegistry == nil { return diff --git a/internal/pluginhost/executor_lock_order_test.go b/internal/pluginhost/executor_lock_order_test.go new file mode 100644 index 00000000000..fc3fc36ea56 --- /dev/null +++ b/internal/pluginhost/executor_lock_order_test.go @@ -0,0 +1,72 @@ +package pluginhost + +import ( + "testing" + + coreauth "github.com/router-for-me/CLIProxyAPI/v8/sdk/cliproxy/auth" + "github.com/router-for-me/CLIProxyAPI/v8/sdk/pluginapi" +) + +// hostLockProbeManager reports whether Host.mu is held by the caller while the +// executor manager is entered. coreauth.Manager takes its own lock in these +// methods, and its credential selection calls back into the Host, so holding +// Host.mu here produces an ABBA deadlock. +type hostLockProbeManager struct { + *fakeExecutorManager + host *Host + held []string +} + +func (m *hostLockProbeManager) probe(op string) { + if m.host.mu.TryLock() { + m.host.mu.Unlock() + return + } + m.held = append(m.held, op) +} + +func (m *hostLockProbeManager) Executor(provider string) (coreauth.ProviderExecutor, bool) { + m.probe("Executor") + return m.fakeExecutorManager.Executor(provider) +} + +func (m *hostLockProbeManager) RegisterExecutor(executor coreauth.ProviderExecutor) { + m.probe("RegisterExecutor") + m.fakeExecutorManager.RegisterExecutor(executor) +} + +func (m *hostLockProbeManager) UnregisterExecutor(provider string) { + m.probe("UnregisterExecutor") + m.fakeExecutorManager.UnregisterExecutor(provider) +} + +func TestRegisterExecutorsDoesNotHoldHostLockWhileCallingManager(t *testing.T) { + host := newHostWithRecords(capabilityRecord{ + id: "alpha", + plugin: pluginapi.Plugin{Capabilities: pluginapi.Capabilities{ + Executor: &fakeExecutor{identifier: "provider-a"}, + }}, + }) + manager := &hostLockProbeManager{fakeExecutorManager: newFakeExecutorManager(), host: host} + + host.RegisterExecutors(manager, nil) + if manager.registerCalls != 1 { + t.Fatalf("RegisterExecutor calls = %d, want 1", manager.registerCalls) + } + + // Replace the snapshot so provider-a becomes stale and is unregistered. + setHostSnapshotForTest(host, true, capabilityRecord{ + id: "bravo", + plugin: pluginapi.Plugin{Capabilities: pluginapi.Capabilities{ + Executor: &fakeExecutor{identifier: "provider-b"}, + }}, + }) + host.RegisterExecutors(manager, nil) + if len(manager.unregisters) != 1 { + t.Fatalf("UnregisterExecutor calls = %v, want provider-a", manager.unregisters) + } + + if len(manager.held) != 0 { + t.Fatalf("Host.mu was held during manager calls: %v", manager.held) + } +} diff --git a/sdk/cliproxy/auth/conductor_selection.go b/sdk/cliproxy/auth/conductor_selection.go index e2eac43a75c..76da8506578 100644 --- a/sdk/cliproxy/auth/conductor_selection.go +++ b/sdk/cliproxy/auth/conductor_selection.go @@ -43,25 +43,22 @@ func (m *Manager) hasPluginScheduler() bool { return true } -func (m *Manager) pluginSchedulerWantsAcrossPrioritiesLocked() bool { - if m == nil || m.pluginScheduler == nil { - return false - } - if opt, ok := m.pluginScheduler.(PluginSchedulerAcrossPriorities); ok && opt != nil { - return opt.SchedulerWantsAcrossPriorities() - } - return false -} - // PluginSchedulerWantsAcrossPriorities reports whether the configured plugin scheduler // opted into receiving candidates across all priority tiers. +// +// The scheduler is read under the manager lock but consulted after releasing it, so the manager +// lock is never held across the scheduler call. func (m *Manager) PluginSchedulerWantsAcrossPriorities() bool { if m == nil { return false } m.mu.RLock() - defer m.mu.RUnlock() - return m.pluginSchedulerWantsAcrossPrioritiesLocked() + scheduler := m.pluginScheduler + m.mu.RUnlock() + if opt, ok := scheduler.(PluginSchedulerAcrossPriorities); ok && opt != nil { + return opt.SchedulerWantsAcrossPriorities() + } + return false } func isBuiltInSelector(selector Selector) bool { @@ -620,10 +617,10 @@ func (m *Manager) availableAuthsForRouteModelWithPriorityMode(auths []*Auth, pro // availableAuthsForSelector reports the candidates handed to priority-scoped consumers such as // the plugin scheduler, plus the candidates handed to the configured selector. Both are equal // unless session affinity or an across-priorities scheduler is active, in which case the selector -// or scheduler additionally receives lower priority tiers. -func (m *Manager) availableAuthsForSelector(selector Selector, auths []*Auth, provider, routeModel string, now time.Time) (priorityAuths, selectorAuths []*Auth, err error) { +// or scheduler additionally receives lower priority tiers. schedulerAcross is resolved by the +// caller before it takes the manager lock. +func (m *Manager) availableAuthsForSelector(selector Selector, auths []*Auth, provider, routeModel string, now time.Time, schedulerAcross bool) (priorityAuths, selectorAuths []*Auth, err error) { _, sessionAffinity := selector.(*SessionAffinitySelector) - schedulerAcross := m.pluginSchedulerWantsAcrossPrioritiesLocked() if !sessionAffinity && !schedulerAcross { priorityAuths, err = m.availableAuthsForRouteModel(auths, provider, routeModel, now) @@ -1822,6 +1819,7 @@ func (m *Manager) pickNextLegacy(ctx context.Context, provider, model string, op pinnedAuthID := pinnedAuthIDFromMetadata(opts.Metadata) eligibility := authSelectionEligibilityForRequest(ctx, opts) + schedulerAcross := m.PluginSchedulerWantsAcrossPriorities() m.mu.RLock() selector := m.selector opts.Metadata[cliproxyexecutor.SessionAffinityModelMetadataKey] = selectionArgForSelector(selector, model) @@ -1874,7 +1872,7 @@ func (m *Manager) pickNextLegacy(ctx context.Context, provider, model string, op return nil, nil, &Error{Code: "auth_not_found", Message: "no auth available"} } now := time.Now() - available, selectorAuths, errAvailable := m.availableAuthsForSelector(selector, candidates, provider, model, now) + available, selectorAuths, errAvailable := m.availableAuthsForSelector(selector, candidates, provider, model, now, schedulerAcross) if errAvailable != nil { m.mu.RUnlock() m.warnLogAuthUnavailable(ctx, []string{provider}, model, opts, tried, errAvailable) @@ -2164,6 +2162,7 @@ func (m *Manager) pickNextMixedLegacy(ctx context.Context, providers []string, m return nil, nil, "", &Error{Code: "provider_not_found", Message: "no provider supplied"} } + schedulerAcross := m.PluginSchedulerWantsAcrossPriorities() m.mu.RLock() selector := m.selector opts.Metadata[cliproxyexecutor.SessionAffinityModelMetadataKey] = selectionArgForSelector(selector, model) @@ -2220,7 +2219,7 @@ func (m *Manager) pickNextMixedLegacy(ctx context.Context, providers []string, m return nil, nil, "", &Error{Code: "auth_not_found", Message: "no auth available"} } now := time.Now() - available, selectorAuths, errAvailable := m.availableAuthsForSelector(selector, candidates, "mixed", model, now) + available, selectorAuths, errAvailable := m.availableAuthsForSelector(selector, candidates, "mixed", model, now, schedulerAcross) if errAvailable != nil { m.mu.RUnlock() m.warnLogAuthUnavailable(ctx, providers, model, opts, tried, errAvailable) diff --git a/sdk/cliproxy/auth/scheduler_lock_order_test.go b/sdk/cliproxy/auth/scheduler_lock_order_test.go new file mode 100644 index 00000000000..198d8bb1ec1 --- /dev/null +++ b/sdk/cliproxy/auth/scheduler_lock_order_test.go @@ -0,0 +1,75 @@ +package auth + +import ( + "context" + "testing" + + cliproxyexecutor "github.com/router-for-me/CLIProxyAPI/v8/sdk/cliproxy/executor" +) + +// lockProbeScheduler records whether Manager.mu was free for writers while +// SchedulerWantsAcrossPriorities ran. A real plugin host takes its own mutex +// there, and the same mutex is held across Manager.RegisterExecutor (which +// needs Manager.mu for writing), so the call must not run under Manager.mu.RLock. +type lockProbeScheduler struct { + fakePluginScheduler + manager *Manager + probed int + blocked int +} + +func (s *lockProbeScheduler) SchedulerWantsAcrossPriorities() bool { + s.probed++ + if s.manager.mu.TryLock() { + s.manager.mu.Unlock() + } else { + s.blocked++ + } + return false +} + +func newLockProbeManager(t *testing.T) (*Manager, *lockProbeScheduler) { + t.Helper() + manager := NewManager(nil, &RoundRobinSelector{}, nil) + manager.executors["gemini"] = schedulerTestExecutor{} + if _, err := manager.Register(context.Background(), &Auth{ID: "auth-a", Provider: "gemini"}); err != nil { + t.Fatalf("Register() error = %v", err) + } + scheduler := &lockProbeScheduler{manager: manager} + manager.SetPluginScheduler(scheduler) + return manager, scheduler +} + +func TestPickNextDoesNotCallSchedulerAcrossPrioritiesUnderManagerLock(t *testing.T) { + manager, scheduler := newLockProbeManager(t) + if _, _, err := manager.pickNextLegacy(context.Background(), "gemini", "", cliproxyexecutor.Options{}, nil); err != nil { + t.Fatalf("pickNextLegacy() error = %v", err) + } + if scheduler.probed == 0 { + t.Fatal("SchedulerWantsAcrossPriorities was never consulted") + } + if scheduler.blocked != 0 { + t.Fatalf("SchedulerWantsAcrossPriorities ran under Manager.mu %d time(s)", scheduler.blocked) + } +} + +func TestPickNextMixedDoesNotCallSchedulerAcrossPrioritiesUnderManagerLock(t *testing.T) { + manager, scheduler := newLockProbeManager(t) + if _, _, _, err := manager.pickNextMixedLegacy(context.Background(), []string{"gemini"}, "", cliproxyexecutor.Options{}, nil); err != nil { + t.Fatalf("pickNextMixedLegacy() error = %v", err) + } + if scheduler.probed == 0 { + t.Fatal("SchedulerWantsAcrossPriorities was never consulted") + } + if scheduler.blocked != 0 { + t.Fatalf("SchedulerWantsAcrossPriorities ran under Manager.mu %d time(s)", scheduler.blocked) + } +} + +func TestPluginSchedulerWantsAcrossPrioritiesDoesNotHoldManagerLock(t *testing.T) { + manager, scheduler := newLockProbeManager(t) + manager.PluginSchedulerWantsAcrossPriorities() + if scheduler.probed != 1 || scheduler.blocked != 0 { + t.Fatalf("probed=%d blocked=%d, want 1 and 0", scheduler.probed, scheduler.blocked) + } +}