diff --git a/core/engine.go b/core/engine.go index 5abc4f597..1f056e96e 100644 --- a/core/engine.go +++ b/core/engine.go @@ -12114,7 +12114,9 @@ func (e *Engine) executeCardAction(cmd, args, sessionKey string) { s.ClearHistory() e.sessions.Save() if e.providerSaveFunc != nil { - _ = e.providerSaveFunc(provName) + if err := e.providerSaveFunc(provName); err != nil { + slog.Error("failed to save provider", "error", err) + } } } diff --git a/core/management.go b/core/management.go index 70bab96a4..103ab798c 100644 --- a/core/management.go +++ b/core/management.go @@ -1154,14 +1154,26 @@ func (m *ManagementServer) handleProjectProviders(w http.ResponseWriter, r *http action = parts[1] } if action == "activate" && r.Method == http.MethodPost { - if !ps.SetActiveProvider(provName) { + found := false + for _, p := range ps.ListProviders() { + if p.Name == provName { + found = true + break + } + } + if !found { mgmtError(w, http.StatusNotFound, fmt.Sprintf("provider not found: %s", provName)) return } - e.resetAllSessions() if e.providerSaveFunc != nil { - _ = e.providerSaveFunc(provName) + if err := e.providerSaveFunc(provName); err != nil { + slog.Error("failed to save active provider", "provider", provName, "error", err) + mgmtError(w, http.StatusInternalServerError, "failed to persist provider: "+err.Error()) + return + } } + ps.SetActiveProvider(provName) + e.resetAllSessions() mgmtJSON(w, http.StatusOK, map[string]any{ "active_provider": provName, "message": "provider activated", @@ -1188,10 +1200,14 @@ func (m *ManagementServer) handleProjectProviders(w http.ResponseWriter, r *http mgmtError(w, http.StatusNotFound, fmt.Sprintf("provider not found: %s", provName)) return } - ps.SetProviders(remaining) if e.providerRemoveSaveFunc != nil { - _ = e.providerRemoveSaveFunc(provName) + if err := e.providerRemoveSaveFunc(provName); err != nil { + slog.Error("failed to persist provider removal", "provider", provName, "error", err) + mgmtError(w, http.StatusInternalServerError, "failed to persist provider removal: "+err.Error()) + return + } } + ps.SetProviders(remaining) mgmtOK(w, "provider removed") return } @@ -1246,12 +1262,16 @@ func (m *ManagementServer) handleProjectProviders(w http.ResponseWriter, r *http Thinking: body.Thinking, Env: body.Env, } + if e.providerAddSaveFunc != nil { + if err := e.providerAddSaveFunc(prov); err != nil { + slog.Error("failed to persist provider", "provider", prov.Name, "error", err) + mgmtError(w, http.StatusInternalServerError, "failed to persist provider: "+err.Error()) + return + } + } providers := ps.ListProviders() providers = append(providers, prov) ps.SetProviders(providers) - if e.providerAddSaveFunc != nil { - _ = e.providerAddSaveFunc(prov) - } mgmtJSON(w, http.StatusOK, map[string]any{ "name": body.Name, "message": "provider added", diff --git a/core/management_provider_persist_test.go b/core/management_provider_persist_test.go new file mode 100644 index 000000000..a14e9fe47 --- /dev/null +++ b/core/management_provider_persist_test.go @@ -0,0 +1,225 @@ +package core + +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" +) + +// stubPersistProvider is a ProviderSwitcher with a mutex so handler tests can +// assert on in-memory state without racing the HTTP handler goroutine. +type stubPersistProvider struct { + stubAgent + mu sync.Mutex + providers []ProviderConfig + active string +} + +func (s *stubPersistProvider) ListProviders() []ProviderConfig { + s.mu.Lock() + defer s.mu.Unlock() + out := make([]ProviderConfig, len(s.providers)) + copy(out, s.providers) + return out +} +func (s *stubPersistProvider) SetProviders(p []ProviderConfig) { + s.mu.Lock() + s.providers = append([]ProviderConfig(nil), p...) + s.mu.Unlock() +} +func (s *stubPersistProvider) GetActiveProvider() *ProviderConfig { + s.mu.Lock() + defer s.mu.Unlock() + for i := range s.providers { + if s.providers[i].Name == s.active { + p := s.providers[i] + return &p + } + } + return nil +} +func (s *stubPersistProvider) SetActiveProvider(name string) bool { + s.mu.Lock() + defer s.mu.Unlock() + if name == "" { + s.active = "" + return true + } + for _, p := range s.providers { + if p.Name == name { + s.active = name + return true + } + } + return false +} +func (s *stubPersistProvider) activeName() string { + s.mu.Lock() + defer s.mu.Unlock() + return s.active +} +func (s *stubPersistProvider) count() int { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.providers) +} + +func newMgmtProviderServer(t *testing.T, token string, save, addSave, removeSave func() error) (*ManagementServer, *httptest.Server, *Engine, *stubPersistProvider) { + t.Helper() + agent := &stubPersistProvider{ + providers: []ProviderConfig{{Name: "prov-a"}, {Name: "prov-b"}}, + } + e := NewEngine("test-project", agent, nil, "", LangEnglish) + e.sessions = NewSessionManager("") + e.providerSaveFunc = func(string) error { return save() } + e.providerAddSaveFunc = func(ProviderConfig) error { return addSave() } + e.providerRemoveSaveFunc = func(string) error { return removeSave() } + + mgmt := NewManagementServer(0, token, nil) + mgmt.RegisterEngine("test-project", e) + + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/projects/", mgmt.wrap(mgmt.handleProjectRoutes)) + ts := httptest.NewServer(mux) + t.Cleanup(ts.Close) + return mgmt, ts, e, agent +} + +// Regression: activate previously returned 200 even when persisting the active +// provider to disk failed, so the in-memory state diverged from what was on +// disk without telling the caller. The handler must now return 500 and leave +// the active provider unchanged when save fails. +func TestMgmt_ActivateProvider_PersistFailure(t *testing.T) { + wantErr := errors.New("disk full") + _, ts, _, agent := newMgmtProviderServer(t, "tok", + func() error { return wantErr }, + func() error { return nil }, + func() error { return nil }, + ) + + req, _ := http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers/prov-a/activate", nil) + req.Header.Set("Authorization", "Bearer tok") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("post activate: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500 on persist failure", resp.StatusCode) + } + if got := agent.activeName(); got != "" { + t.Fatalf("active provider mutated to %q despite persist failure", got) + } +} + +// Regression: DELETE provider must not mutate in-memory state when disk removal +// fails; it must surface a 500 so the user knows the operation didn't take. +func TestMgmt_DeleteProvider_PersistFailure(t *testing.T) { + wantErr := errors.New("permission denied") + _, ts, _, agent := newMgmtProviderServer(t, "tok", + func() error { return nil }, + func() error { return nil }, + func() error { return wantErr }, + ) + + req, _ := http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/projects/test-project/providers/prov-a", nil) + req.Header.Set("Authorization", "Bearer tok") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("delete: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500 on persist failure", resp.StatusCode) + } + if got := agent.count(); got != 2 { + t.Fatalf("provider count = %d after failed delete, want 2 (unchanged)", got) + } +} + +// Regression: POST provider must surface persistence failures as 500 and must +// not add the provider to in-memory state when disk write fails. +func TestMgmt_AddProvider_PersistFailure(t *testing.T) { + wantErr := errors.New("config read-only") + _, ts, _, agent := newMgmtProviderServer(t, "tok", + func() error { return nil }, + func() error { return wantErr }, + func() error { return nil }, + ) + + body := strings.NewReader(`{"name":"prov-c","api_key":"x"}`) + req, _ := http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers", body) + req.Header.Set("Authorization", "Bearer tok") + req.Header.Set("Content-Type", "application/json") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("post add: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500 on persist failure", resp.StatusCode) + } + if got := agent.count(); got != 2 { + t.Fatalf("provider count = %d after failed add, want 2 (unchanged)", got) + } +} + +// Sanity: when persistence succeeds, the happy paths still do the in-memory +// mutation and return 200. +func TestMgmt_ProviderLifecycle_HappyPath(t *testing.T) { + _, ts, _, agent := newMgmtProviderServer(t, "tok", + func() error { return nil }, + func() error { return nil }, + func() error { return nil }, + ) + + // activate + req, _ := http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers/prov-a/activate", nil) + req.Header.Set("Authorization", "Bearer tok") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("activate: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("activate status = %d, want 200", resp.StatusCode) + } + if got := agent.activeName(); got != "prov-a" { + t.Fatalf("active = %q, want prov-a", got) + } + + // add + addBody := strings.NewReader(`{"name":"prov-c"}`) + req, _ = http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers", addBody) + req.Header.Set("Authorization", "Bearer tok") + req.Header.Set("Content-Type", "application/json") + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("add: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("add status = %d, want 200", resp.StatusCode) + } + if got := agent.count(); got != 3 { + t.Fatalf("count after add = %d, want 3", got) + } + + // delete + req, _ = http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/projects/test-project/providers/prov-c", nil) + req.Header.Set("Authorization", "Bearer tok") + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("delete: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("delete status = %d, want 200", resp.StatusCode) + } + if got := agent.count(); got != 2 { + t.Fatalf("count after delete = %d, want 2", got) + } +}