diff --git a/ui/litellm-dashboard/e2e_tests/tests/settings/routerSettings.spec.ts b/ui/litellm-dashboard/e2e_tests/tests/settings/routerSettings.spec.ts index 98b86ec9b11..3e140b9ab56 100644 --- a/ui/litellm-dashboard/e2e_tests/tests/settings/routerSettings.spec.ts +++ b/ui/litellm-dashboard/e2e_tests/tests/settings/routerSettings.spec.ts @@ -3,6 +3,14 @@ import { ADMIN_STORAGE_PATH } from "../../constants"; import { navigateToPage } from "../../helpers/navigation"; import { Page } from "../../fixtures/pages"; import { Role, users } from "../../fixtures/users"; +// Type-only import of the OpenAPI-generated backend schema, erased at runtime by +// esbuild. It types the round-trips below so mistakes surface in the editor; the live +// test against the real proxy is what actually enforces the contract. +import type { components } from "../../../src/lib/http/schema"; + +// These tests mutate the proxy's shared router_settings, and the Loadbalancing save +// echoes the whole settings object, so they must not run concurrently. +test.describe.configure({ mode: "serial" }); const PRIMARY = "fake-openai-gpt-4"; const FALLBACK = "fake-anthropic-claude"; @@ -99,3 +107,84 @@ test.describe("Router Settings - Fallbacks", () => { await expect(newRow).toHaveCount(1, { timeout: 10_000 }); }); }); + +type ConfigYAML = components["schemas"]["ConfigYAML"]; +type RouterSettingsResponse = components["schemas"]["RouterSettingsResponse"]; + +const BASE_URL = "http://localhost:4000"; +const ADMIN_AUTH = { Authorization: `Bearer ${users[Role.ProxyAdmin].password}` }; + +/** + * Apply a router_settings patch through the typed /config/update contract. The + * server merges it over existing settings (request wins), so only the passed keys + * change. Fails loudly if the write is rejected instead of leaving a silent bad seed. + */ +async function patchRouterSettings( + request: import("@playwright/test").APIRequestContext, + patch: Partial>, +) { + const res = await request.post(`${BASE_URL}/config/update`, { + headers: ADMIN_AUTH, + data: { router_settings: patch }, + }); + expect(res.ok(), `seed /config/update failed: ${res.status()} ${await res.text()}`).toBeTruthy(); +} + +test.describe("Router Settings - Loadbalancing", () => { + test.use({ storageState: ADMIN_STORAGE_PATH }); + + // Pin num_retries and an empty routing_groups so the assertions are deterministic. + // Empty already reproduces LIT-4057: the old tab serialized [] to the string "[]" + // and the save 422'd. + test.beforeEach(async ({ request }) => { + await patchRouterSettings(request, { num_retries: 3, routing_groups: [] }); + }); + + test.afterEach(async ({ request }) => { + await patchRouterSettings(request, { num_retries: 3 }); + }); + + test("saves the Loadbalancing tab without a 422 when routing_groups is present, and persists", async ({ + page, + request, + }) => { + await navigateToPage(page, Page.RouterSettings); + await page.getByRole("tab", { name: "Loadbalancing" }).click(); + + const numRetries = page.locator('input[name="num_retries"]'); + await expect(numRetries).toHaveValue("3", { timeout: 15_000 }); + // routing_groups belongs to its own tab and must not leak into this form. + await expect(page.locator('input[name="routing_groups"]')).toHaveCount(0); + + await numRetries.fill("5"); + + // LIT-4057: the tab used to serialize routing_groups as the string "[]", + // which the backend rejects with 422 while the UI still claimed success. + // Assert the save actually succeeds at the network level. + const saveResponse = page.waitForResponse( + (res) => res.url().includes("/config/update") && res.request().method() === "POST", + { timeout: 15_000 }, + ); + await page.getByRole("button", { name: /save changes/i }).click(); + expect((await saveResponse).status()).toBe(200); + + await expect(page.getByText(/router settings updated successfully/i).first()).toBeVisible({ timeout: 10_000 }); + + // The ticket's core symptom was that a refresh showed the old value. + await navigateToPage(page, Page.RouterSettings); + await page.getByRole("tab", { name: "Loadbalancing" }).click(); + await expect(page.locator('input[name="num_retries"]')).toHaveValue("5", { timeout: 15_000 }); + + // The typed backend read agrees the change persisted. + await expect + .poll( + async () => { + const res = await request.get(`${BASE_URL}/router/settings`, { headers: ADMIN_AUTH }); + const data = (await res.json()) as RouterSettingsResponse; + return data.current_values?.num_retries; + }, + { timeout: 10_000 }, + ) + .toBe(5); + }); +}); diff --git a/ui/litellm-dashboard/src/components/router_settings/ReliabilityRetriesSection.tsx b/ui/litellm-dashboard/src/components/router_settings/ReliabilityRetriesSection.tsx index fa48c1c97b9..da089552b11 100644 --- a/ui/litellm-dashboard/src/components/router_settings/ReliabilityRetriesSection.tsx +++ b/ui/litellm-dashboard/src/components/router_settings/ReliabilityRetriesSection.tsx @@ -20,14 +20,15 @@ const ReliabilityRetriesSection: React.FC = ({
{Object.entries(routerSettings) .filter( - ([param, value]) => + ([param]) => param != "fallbacks" && param != "context_window_fallbacks" && param != "routing_strategy_args" && param != "routing_strategy" && param != "enable_tag_filtering" && param != "retry_policy" && - param != "model_group_retry_policy", + param != "model_group_retry_policy" && + param != "routing_groups", ) .map(([param, value]) => (
diff --git a/ui/litellm-dashboard/src/components/router_settings/index.test.tsx b/ui/litellm-dashboard/src/components/router_settings/index.test.tsx index 78a0b4b0dff..94cbb94d164 100644 --- a/ui/litellm-dashboard/src/components/router_settings/index.test.tsx +++ b/ui/litellm-dashboard/src/components/router_settings/index.test.tsx @@ -146,4 +146,45 @@ describe("RouterSettings", () => { expect(NotificationsManager.success).toHaveBeenCalledWith("router settings updated successfully"); }); + + it("should not render or save routing_groups (owned by the Routing Groups tab)", async () => { + const user = userEvent.setup(); + vi.mocked(getCallbacksCall).mockResolvedValue({ + router_settings: { + routing_strategy: "simple-shuffle", + num_retries: 3, + routing_groups: [{ group_name: "g1", models: ["gpt-4"], routing_strategy: "simple-shuffle" }], + }, + }); + renderWithProviders(); + + await waitFor(() => { + expect(screen.getByTestId("strategy-select")).toBeInTheDocument(); + }); + expect(document.querySelector('input[name="routing_groups"]')).toBeNull(); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => + expect(setCallbacksCall).toHaveBeenCalledWith("test-token", { + router_settings: expect.not.objectContaining({ routing_groups: expect.anything() }), + }), + ); + }); + + it("should surface an error and not claim success when saving fails", async () => { + const user = userEvent.setup(); + vi.mocked(setCallbacksCall).mockRejectedValue(new Error("422 Unprocessable Entity")); + renderWithProviders(); + + await waitFor(() => { + expect(screen.getByTestId("strategy-select")).toBeInTheDocument(); + }); + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(NotificationsManager.fromBackend).toHaveBeenCalled(); + }); + expect(NotificationsManager.success).not.toHaveBeenCalled(); + }); }); diff --git a/ui/litellm-dashboard/src/components/router_settings/index.tsx b/ui/litellm-dashboard/src/components/router_settings/index.tsx index d3753529058..360c7f41138 100644 --- a/ui/litellm-dashboard/src/components/router_settings/index.tsx +++ b/ui/litellm-dashboard/src/components/router_settings/index.tsx @@ -81,7 +81,7 @@ const RouterSettings: React.FC = ({ accessToken, userRole, }); }, [accessToken, userRole, userID]); - const handleSaveChanges = () => { + const handleSaveChanges = async () => { if (!accessToken) { return; } @@ -91,9 +91,9 @@ const RouterSettings: React.FC = ({ accessToken, userRole, const numberKeys = new Set(["allowed_fails", "cooldown_time", "num_retries", "timeout", "retry_after"]); const jsonKeys = new Set(["model_group_alias"]); - // retry_policy and model_group_retry_policy are owned exclusively by the - // Model Retry Settings tab; this page must not read or write them. - const tabOwnedKeys = new Set(["retry_policy", "model_group_retry_policy"]); + // retry_policy and model_group_retry_policy are owned by the Model Retry Settings tab; + // routing_groups is owned by the Routing Groups tab. This page must not read or write them. + const tabOwnedKeys = new Set(["retry_policy", "model_group_retry_policy", "routing_groups"]); const parseInputValue = (key: string, raw: string | undefined, fallback: unknown) => { if (raw === undefined) return fallback; @@ -172,12 +172,11 @@ const RouterSettings: React.FC = ({ accessToken, userRole, }; try { - setCallbacksCall(accessToken, payload); + await setCallbacksCall(accessToken, payload); + NotificationsManager.success("router settings updated successfully"); } catch (error) { NotificationsManager.fromBackend("Failed to update router settings: " + error); } - - NotificationsManager.success("router settings updated successfully"); }; if (!accessToken) {