diff --git a/crates/aisix-proxy/src/chat.rs b/crates/aisix-proxy/src/chat.rs index 4c1ba37f..1696ee96 100644 --- a/crates/aisix-proxy/src/chat.rs +++ b/crates/aisix-proxy/src/chat.rs @@ -1653,6 +1653,18 @@ async fn dispatch( if attempt_idx == retries { break; } + // #788 P2: exponential backoff + jitter before retrying + // the SAME target, so a transiently-failing upstream gets + // a pause instead of being hammered. Cross-target fallover + // (the outer loop) stays immediate. + let backoff = crate::routing::retry_backoff((attempt_idx + 1) as u32); + tracing::debug!( + target_model = %model.display_name, + next_attempt = attempt_idx + 2, + backoff_ms = backoff.as_millis() as u64, + "backing off before same-target retry", + ); + tokio::time::sleep(backoff).await; } } } diff --git a/crates/aisix-proxy/src/routing.rs b/crates/aisix-proxy/src/routing.rs index fb72802b..5bf594cf 100644 --- a/crates/aisix-proxy/src/routing.rs +++ b/crates/aisix-proxy/src/routing.rs @@ -56,6 +56,38 @@ pub fn is_retryable(err: &BridgeError, retry_on_429: bool) -> bool { } } +/// Base delay before the first same-target retry. Each subsequent retry +/// doubles it, capped at [`RETRY_BACKOFF_MAX_MS`]. +const RETRY_BACKOFF_BASE_MS: u64 = 250; +/// Ceiling for the exponential term — bounds the worst-case added latency. +const RETRY_BACKOFF_MAX_MS: u64 = 2_000; +/// Additive jitter ceiling, sampled uniformly in `[0, this]` and added on +/// top of the exponential term. +const RETRY_BACKOFF_JITTER_MS: u64 = 250; + +/// Backoff before retrying the **same** target, for 1-based retry number +/// `retry` (`retry == 0` → no wait). Exponential term `base * 2^(retry-1)` +/// capped at [`RETRY_BACKOFF_MAX_MS`], plus uniform additive jitter in +/// `[0, RETRY_BACKOFF_JITTER_MS]`. +/// +/// Same strategy as LiteLLM's router (`_calculate_retry_after`: capped +/// exponential floor + additive jitter — not full-jitter-to-zero, so a +/// struggling upstream always gets a real pause), with bounds tightened +/// from LiteLLM's library defaults (0.5s base / 8s cap) to suit an inline +/// proxy where the retry runs inside a single request's latency budget. +/// Cross-target fallover is deliberately NOT backed off — a different, +/// presumably healthy target should be tried immediately (LiteLLM's +/// healthy-deployment fast-path). +pub fn retry_backoff(retry: u32) -> Duration { + if retry == 0 { + return Duration::ZERO; + } + let exp = RETRY_BACKOFF_BASE_MS.saturating_mul(1u64 << (retry - 1).min(20)); + let base = exp.min(RETRY_BACKOFF_MAX_MS); + let jitter = rand::thread_rng().gen_range(0..=RETRY_BACKOFF_JITTER_MS); + Duration::from_millis(base + jitter) +} + #[derive(Default)] pub struct RoutingRegistry { // virtual model name → atomic round-robin cursor @@ -602,6 +634,41 @@ mod tests { )); } + // ── retry_backoff ───────────────────────────────────────────── + #[test] + fn retry_backoff_zero_is_no_wait() { + assert_eq!(retry_backoff(0), Duration::ZERO); + } + + #[test] + fn retry_backoff_grows_exponentially_and_caps() { + // The exponential FLOOR (delay minus the additive jitter) must be + // base*2^(retry-1), capped. Sample many times: the minimum observed + // delay tracks the floor and never exceeds floor + jitter ceiling. + let cases = [ + (1u32, 250u64), // 250 * 2^0 + (2, 500), // 250 * 2^1 + (3, 1000), // 250 * 2^2 + (4, 2000), // 250 * 2^3 = 2000 (== cap) + (5, 2000), // capped + (50, 2000), // capped, no overflow + ]; + for (retry, floor) in cases { + let mut min = u64::MAX; + let mut max = 0u64; + for _ in 0..2000 { + let ms = retry_backoff(retry).as_millis() as u64; + min = min.min(ms); + max = max.max(ms); + } + assert!(min >= floor, "retry {retry}: min {min} < floor {floor}"); + assert!( + max <= floor + 250, + "retry {retry}: max {max} > floor {floor} + jitter 250", + ); + } + } + // ── filter_attempt_models ───────────────────────────────────── fn am(id: &str) -> AttemptModel { let model: Model = serde_json::from_str(&format!( diff --git a/tests/e2e/src/cases/retry-backoff-e2e.test.ts b/tests/e2e/src/cases/retry-backoff-e2e.test.ts new file mode 100644 index 00000000..07d3dae1 --- /dev/null +++ b/tests/e2e/src/cases/retry-backoff-e2e.test.ts @@ -0,0 +1,132 @@ +import { createHash } from "node:crypto"; +import OpenAI, { APIError } from "openai"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + AdminClient, + EtcdClient, + spawnApp, + startOpenAiUpstream, + waitConfigPropagation, + type OpenAiUpstream, + type SpawnedApp, +} from "../harness/index.js"; + +// E2E: same-target retries back off before re-hitting the upstream instead +// of hammering it immediately (issue 788 P2). A routing model with a single +// target and `retries: 2` against an always-503 upstream makes three +// attempts; the two inter-retry backoffs have a guaranteed exponential floor +// (250ms then 500ms — additive jitter only, never full-jitter-to-zero), so +// the whole request must take at least ~750ms. Without backoff the three +// attempts complete in single-digit milliseconds. +// +// The assertion is a LOWER bound on elapsed time, which the exponential +// floor makes non-flaky. + +const CALLER_PLAINTEXT = "sk-retry-backoff-caller"; +const CALLER_KEY_HASH = createHash("sha256") + .update(CALLER_PLAINTEXT) + .digest("hex"); + +// 250ms (retry 1) + 500ms (retry 2) exponential floor, minus a safety margin +// so jitter/scheduling noise can't push a correct run under the threshold. +const MIN_EXPECTED_MS = 600; + +describe("retry backoff e2e: same-target retries wait before re-hitting upstream", () => { + let app: SpawnedApp | undefined; + let upstream: OpenAiUpstream | undefined; + let admin: AdminClient | undefined; + let etcdReachable = false; + + beforeAll(async () => { + etcdReachable = await new EtcdClient().ping(); + if (!etcdReachable) return; + + // Every request to this upstream returns a retryable 503. + upstream = await startOpenAiUpstream({ + status: 503, + errorBody: { error: { message: "always down", type: "server_error" } }, + }); + app = await spawnApp(); + admin = new AdminClient(app.adminUrl, app.adminKey); + + const pk = await admin.createProviderKey({ + display_name: "retry-backoff-pk", + secret: "sk-mock", + api_base: `${upstream.baseUrl}/v1`, + }); + await admin.createModel({ + display_name: "retry-backoff-target", + provider: "openai", + model_name: "gpt-4o-mini", + provider_key_id: pk.id, + }); + // Single-target router with retries=2: the same target is attempted + // three times, so the two inter-retry backoffs are the only delay. + await admin.createModel({ + display_name: "retry-backoff-router", + routing: { + strategy: "failover", + targets: [{ model: "retry-backoff-target" }], + retries: 2, + }, + }); + await admin.createApiKey({ + key_hash: CALLER_KEY_HASH, + allowed_models: ["retry-backoff-router"], + }); + }); + + afterAll(async () => { + await app?.exit(); + await upstream?.close(); + }); + + test("retries=2 against an always-503 upstream waits for the backoff floor", async (ctx) => { + if (!etcdReachable || !app || !upstream) { + ctx.skip(); + return; + } + + const client = new OpenAI({ + apiKey: CALLER_PLAINTEXT, + baseURL: `${app.proxyUrl}/v1`, + maxRetries: 0, + }); + + // Wait until the router is active: a probe actually reaches the upstream + // (status-agnostic — before the router/model/key propagate, the probe + // 404s at the gateway and never hits the upstream). + await waitConfigPropagation(async () => { + const before = upstream!.receivedRequests.length; + try { + await client.chat.completions.create({ + model: "retry-backoff-router", + messages: [{ role: "user", content: "probe" }], + }); + } catch { + // expected: upstream is always 503. + } + return upstream!.receivedRequests.length > before; + }); + + const hitsBefore = upstream.receivedRequests.length; + const start = Date.now(); + let caught: unknown; + try { + await client.chat.completions.create({ + model: "retry-backoff-router", + messages: [{ role: "user", content: "drive the retries" }], + }); + } catch (e) { + caught = e; + } + const elapsed = Date.now() - start; + + // The request exhausts its retries and fails. + expect(caught).toBeInstanceOf(APIError); + // Three attempts to the single target (initial + 2 retries). + expect(upstream.receivedRequests.length - hitsBefore).toBe(3); + // ...and the two inter-retry backoffs make it take at least the floor. + expect(elapsed).toBeGreaterThanOrEqual(MIN_EXPECTED_MS); + }); +});