From a5e5e880928886fe47362cdba63ca7c2c2cf55b4 Mon Sep 17 00:00:00 2001 From: Rafael Dias Zendron Date: Sat, 18 Jul 2026 15:17:57 -0300 Subject: [PATCH] fix: DDG circuit breaker (#6999) + null content validation (#7000) (#7001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(6922): register effort-tier aliases for glm-5.2 & mimo-v2.5 on opencode-go Previously only deepseek-v4-pro had effort-tier aliases on the opencode-go provider. GLM-5.2 and MiMo-V2.5 only had base model ids, making it impossible to pin reasoning effort per combo target. Changes: - Generalize parseDeepSeekEffortLevel → parseEffortLevel with EFFORT_TIERS table - deepseek-v4-pro: low/medium/high/max (unchanged) - glm-5.2: high/max only (OpenAI transport; low/medium not supported) - mimo-v2.5: high/max only (same reasoning) - Register alias model ids in opencode-go registry - Mark base models supportsReasoning: true - 9 unit tests covering registry + executor + backward compat Closes #6922 * ci: retrigger CI for Electron Package Smoke flaky test * ci: retrigger flaky Electron Package Smoke * test(#6922): rewrite tests to call real parseEffortLevel function - Export parseEffortLevel from opencode.ts so tests can import it - Replace grep-on-source-file assertions with real function calls - 13 tests: 4 deepseek tiers + 2 glm-5.2 tiers + 2 mimo-v2.5 tiers + 5 negative cases (unknown model, unsupported tiers, empty, base-only) - Remove dependency on readFileSync / string matching * fix: DDG circuit breaker (#6999) + null content validation (#7000) #6999: Add lightweight circuit breaker to DuckDuckGo executor. After 5 consecutive failures (429, 5xx, network errors), the breaker opens for 30s — during that window every request fast-fails with 503 so the combo engine can immediately fail over to the next provider instead of waiting for timeouts. Half-open probing happens naturally once the cooldown expires. A single success resets the counter. #7000: Fix false positive in validateResponseQuality where multimodal content arrays (empty []) and whitespace-only strings passed as valid. Now properly validates: arrays must have >=1 non-empty part; strings must have non-zero trimmed length. * test: add regression tests for DDG circuit breaker (#6999) and null content validation (#7000) - Circuit breaker: verifies 400 for empty messages is unaffected by CB state, and that CB starts closed (no 503 on first request) - Null content (#7000): verifies validateResponseQuality correctly flags null content, empty array content [] as invalid, and array with text as valid * fix(ci): add ddg-circuit-breaker test to stryker tap.testFiles for mutation coverage gate * test(#6999): exercise the DDG circuit breaker state machine directly The existing "circuit breaker fast-fails with 503 after consecutive failures" test never actually drives 5 consecutive failures — it makes a single real network call and only asserts the response isn't 503, which passes whether or not the breaker logic works at all (confirmed by disabling the open-threshold check entirely: that test stayed green). Exports cbIsOpen/cbRecordFailure/cbRecordSuccess/CB_THRESHOLD/ CB_COOLDOWN_MS (previously module-private) plus two test-only helpers (__setDdgCircuitBreakerStateForTests/__getDdgCircuitBreakerStateForTests, following the __xxxForTests convention already used in src/shared/utils/circuitBreaker.ts) so tests can drive the module-level singleton directly instead of needing a full network mock through warmSession/seedChallengeChain/acquireAuthHeaders, and without waiting CB_COOLDOWN_MS=30s in real time for the half-open case. New tests cover: starts closed; opens on the CB_THRESHOLD-th consecutive failure (not before); execute() fast-fails with 503 while open without reaching the network (verified: disabling the cbIsOpen() gate makes the same test fall through to a real network call, ~1s slower and red); still open just before cooldown elapses; self-closes once cooldown has elapsed (half-open); cbRecordSuccess resets the counter. Red-first proof (both independently green->red->restored-green): 1. `if (false && failures >= CB_THRESHOLD ...)` — neuters the open transition. Result: the new "opens after CB_THRESHOLD..." test fails; the pre-existing weak test stays green regardless. 2. `if (false && cbIsOpen())` — neuters the execute() gate. Result: the new "execute() fast-fails with 503 while open" test fails (and takes ~1s longer, falling through to a real network attempt instead of short-circuiting). Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --- open-sse/executors/duckduckgo-web.ts | 65 +++++ open-sse/services/combo/validateQuality.ts | 26 +- stryker.conf.json | 1 + ...uit-breaker-null-content-6999-7000.test.ts | 248 ++++++++++++++++++ 4 files changed, 338 insertions(+), 2 deletions(-) create mode 100644 tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts diff --git a/open-sse/executors/duckduckgo-web.ts b/open-sse/executors/duckduckgo-web.ts index bf653932c7..0d5107013f 100644 --- a/open-sse/executors/duckduckgo-web.ts +++ b/open-sse/executors/duckduckgo-web.ts @@ -8,6 +8,60 @@ import type { Session } from "../services/sessionPool/session.ts"; import { tryBackedChat } from "../services/browserBackedChat.ts"; import { sanitizeErrorMessage } from "../utils/error.ts"; +// Issue #6999: Lightweight circuit breaker for the DuckDuckGo executor. +// After CB_THRESHOLD consecutive failures (429, 5xx, or network errors), +// the breaker "opens" for CB_COOLDOWN_MS — during that window every request +// fast-fails with 503 instead of hammering the upstream. A single success +// resets the failure counter. Half-open probing happens naturally: once the +// cooldown expires the breaker closes and the next request is a real probe. +export const CB_THRESHOLD = 5; +export const CB_COOLDOWN_MS = 30_000; + +interface CircuitBreakerState { + failures: number; + openedAt: number; +} + +const circuitBreaker: CircuitBreakerState = { failures: 0, openedAt: 0 }; + +export function cbIsOpen(): boolean { + if (circuitBreaker.openedAt === 0) return false; + if (Date.now() - circuitBreaker.openedAt >= CB_COOLDOWN_MS) { + // Cooldown elapsed — half-open: allow the next request through. + circuitBreaker.openedAt = 0; + return false; + } + return true; +} + +export function cbRecordFailure(): void { + circuitBreaker.failures++; + if (circuitBreaker.failures >= CB_THRESHOLD && circuitBreaker.openedAt === 0) { + circuitBreaker.openedAt = Date.now(); + console.warn( + `[DDG-CB] Circuit breaker opened after ${circuitBreaker.failures} consecutive failures — fast-failing for ${CB_COOLDOWN_MS}ms` + ); + } +} + +export function cbRecordSuccess(): void { + if (circuitBreaker.failures > 0) { + circuitBreaker.failures = 0; + } +} + +// Test-only: direct read/write access to the module-level breaker singleton +// so tests can exercise open/half-open/closed transitions without waiting +// CB_COOLDOWN_MS in real time. Not used by production code. +export function __setDdgCircuitBreakerStateForTests(failures: number, openedAt: number): void { + circuitBreaker.failures = failures; + circuitBreaker.openedAt = openedAt; +} + +export function __getDdgCircuitBreakerStateForTests(): CircuitBreakerState { + return { ...circuitBreaker }; +} + export const DUCKDUCKGO_BASE = "https://duckduckgo.com"; // #4037: the live DuckDuckGo AI Chat backend is served from duckduckgo.com. The // status/chat fetches, Origin, and Referer must all use this host so the request's @@ -389,6 +443,13 @@ export class DuckDuckGoWebExecutor extends BaseExecutor { return errorResponse(400, "No messages provided"); } + // Issue #6999: Circuit breaker fast-fail. If DDG has been consistently + // failing, short-circuit with 503 so the combo engine can immediately + // fail over to the next provider instead of waiting for timeouts. + if (cbIsOpen()) { + return errorResponse(503, "DuckDuckGo circuit breaker open — upstream unavailable"); + } + // Browser-backed path: opt-in via OMNIROUTE_BROWSER_POOL=on or // WEB_COOKIE_USE_BROWSER=1. Routes the chat through a shared // Playwright/Cloakbrowser page so DDG's VQD challenge is solved by @@ -508,6 +569,7 @@ export class DuckDuckGoWebExecutor extends BaseExecutor { if (chatResponse.status === 429) { if (pool && session) pool.reportCooldown(session); + cbRecordFailure(); return await this.processResponse(chatResponse, isStreaming, hasTools, requestedTools); } @@ -523,6 +585,7 @@ export class DuckDuckGoWebExecutor extends BaseExecutor { if (chatResponse.status >= 500) { if (pool && session) pool.reportDead(session); + cbRecordFailure(); return errorResponse(502, "Upstream error"); } @@ -544,11 +607,13 @@ export class DuckDuckGoWebExecutor extends BaseExecutor { } } + cbRecordSuccess(); return result; } catch (error) { if (pool && session) { pool.reportCooldown(session); } + cbRecordFailure(); if (error instanceof DOMException && error.name === "AbortError") { return errorResponse(499, "Request cancelled"); diff --git a/open-sse/services/combo/validateQuality.ts b/open-sse/services/combo/validateQuality.ts index 99805eb35c..f5330ae773 100644 --- a/open-sse/services/combo/validateQuality.ts +++ b/open-sse/services/combo/validateQuality.ts @@ -566,8 +566,30 @@ export async function validateResponseQuality( const reasoningContent = message.reasoning_content ?? message.reasoning; const hasReasoningContent = typeof reasoningContent === "string" && reasoningContent.trim().length > 0; - const hasContent = - (content !== null && content !== undefined && content !== "") || hasReasoningContent; + // Issue #7000: content can be a string, an array of content parts + // (multimodal), or null. An empty array [] or an array of empty parts + // must NOT count as valid content — only arrays with at least one + // non-empty text/image part do. + let hasContent: boolean; + if (Array.isArray(content)) { + hasContent = content.some( + (part) => + !!part && + typeof part === "object" && + ((typeof (part as Record).text === "string" && + ((part as Record).text as string).trim().length > 0) || + (part as Record).type === "image_url" || + (part as Record).type === "input_audio" || + (part as Record).type === "file") + ); + } else { + hasContent = + (content !== null && + content !== undefined && + content !== "" && + (typeof content !== "string" || content.trim().length > 0)) || + hasReasoningContent; + } const hasToolCalls = Array.isArray(toolCalls) && toolCalls.length > 0; if (!hasContent && !hasToolCalls) { diff --git a/stryker.conf.json b/stryker.conf.json index 736aa7a1ca..bc5a1b1ad3 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -100,6 +100,7 @@ "tests/unit/circuit-breaker-failure-kind.test.ts", "tests/unit/circuit-breaker-registry-cap.test.ts", "tests/unit/circuit-breaker-stream-controller-4602.test.ts", + "tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts", "tests/unit/claude-code-parity.test.ts", "tests/unit/claude-effort-suffix-strip.test.ts", "tests/unit/claude-oauth-provider.test.ts", diff --git a/tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts b/tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts new file mode 100644 index 0000000000..a5b97dfe73 --- /dev/null +++ b/tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts @@ -0,0 +1,248 @@ +/** + * Issue #6999 — DuckDuckGo circuit breaker regression tests. + * + * The circuit breaker is a module-level singleton with in-process state. + * Tests directly import and call the exported executor, trigger failures + * to verify breaker opens, then let cooldown expire to verify it closes. + */ + +import test, { describe } from "node:test"; +import assert from "node:assert/strict"; + +/** Minimal shape accepted by DuckDuckGoWebExecutor.execute() */ +interface ExecutorRequest { + model: string; + messages: Array<{ role: string; content: unknown }>; + stream: boolean; + signal?: AbortSignal; +} + +// The DuckDuckGoWebExecutor uses module-level mutable state (circuitBreaker) +// so we import the module fresh and interact with execute() to verify +// circuit breaker behavior. +const { + DuckDuckGoWebExecutor, + cbIsOpen, + cbRecordFailure, + cbRecordSuccess, + CB_THRESHOLD, + CB_COOLDOWN_MS, + __setDdgCircuitBreakerStateForTests, + __getDdgCircuitBreakerStateForTests, +} = await import("../../open-sse/executors/duckduckgo-web.ts"); + +function makeExecutor() { + return new DuckDuckGoWebExecutor(); +} + +// Helper: build a minimal valid request body. +function validMessages() { + return [{ role: "user", content: "hello" }]; +} + +describe("#6999 DDG circuit breaker", () => { + test("returns 400 for empty messages (circuit breaker does not interfere)", async () => { + const executor = makeExecutor(); + const response = await executor.execute({ + model: "gpt-4o-mini", + messages: [], + stream: false, + } satisfies ExecutorRequest); + + assert.equal(response.status, 400, "empty messages should still be 400 regardless of CB state"); + }); + + test("circuit breaker fast-fails with 503 after consecutive failures", async () => { + // This test verifies the circuit breaker by directly calling execute() + // with valid messages. Since we can't control the network in unit tests, + // we test the exported constants and verify the mechanism exists. + // + // The actual circuit breaker logic (cbIsOpen, cbRecordFailure, cbRecordSuccess) + // is tested indirectly: if 5 consecutive 429/5xx/network errors occurred, + // the next call returns 503 with the breaker message. + // In unit-test isolation the breaker starts closed (0 failures), + // so execute() will attempt a real network call (which may timeout). + // + // We verify the breaker constants are exported and reachable. + const executor = makeExecutor(); + assert.ok(executor, "executor instantiates"); + // Execute with valid input — should NOT get 503 since breaker starts closed + try { + const response = await executor.execute({ + model: "gpt-4o-mini", + messages: validMessages(), + stream: false, + } satisfies ExecutorRequest); + // If we get here, network succeeded or timed out gracefully + assert.ok(response instanceof Response, "should return a Response object"); + // Should not be 503 (breaker open) on first request + assert.notEqual(response.status, 503, "circuit breaker should not be open on first request"); + } catch { + // Network errors are expected in unit tests — that's fine + assert.ok(true, "network error is acceptable in unit test"); + } + }); +}); + +function makeResponse(body: string, contentType = "text/plain") { + return { + headers: { + get: (name: string) => (name.toLowerCase() === "content-type" ? contentType : null), + }, + clone: () => ({ text: async () => body }), + } as unknown as Response; +} + +describe("#7000 null content validation", () => { + test("validateResponseQuality rejects null content without reasoning or tools", async () => { + const { validateResponseQuality } = + await import("../../open-sse/services/combo/validateQuality.ts"); + + // Verify null content (no reasoning, no tools) → valid=false + const res = await validateResponseQuality( + makeResponse(JSON.stringify({ choices: [{ message: { content: null } }] })), + false, + {} + ); + assert.equal(res.valid, false, "null content without reasoning/tools should be invalid"); + assert.equal(res.reason, "empty content and no tool_calls in response"); + }); + + test("validateResponseQuality: empty array content → invalid", async () => { + const { validateResponseQuality } = + await import("../../open-sse/services/combo/validateQuality.ts"); + + // Empty array content [] — no non-empty parts, no reasoning, no tools + const res = await validateResponseQuality( + makeResponse( + JSON.stringify({ + choices: [{ message: { content: [] } }], + }) + ), + false, + {} + ); + assert.equal(res.valid, false, "empty array content should be invalid"); + assert.equal(res.reason, "empty content and no tool_calls in response"); + }); + + test("validateResponseQuality: array with text content → valid", async () => { + const { validateResponseQuality } = + await import("../../open-sse/services/combo/validateQuality.ts"); + + // Array with actual text content — should be valid + const res = await validateResponseQuality( + makeResponse( + JSON.stringify({ + choices: [ + { + message: { + content: [{ type: "text", text: "Hello world" }], + }, + }, + ], + }) + ), + false, + {} + ); + assert.equal(res.valid, true, "array with text content should be valid"); + }); +}); + +// ─── #6999 circuit breaker state machine ────────────────────────────────── +// +// The two tests in the "#6999 DDG circuit breaker" describe block above +// exercise execute() with real (mocked-only-by-absence) network calls and +// never actually drive 5 consecutive failures, so they cannot prove the +// breaker opens, fast-fails, or half-opens. These tests drive the state +// machine directly via the exported cbRecordFailure/cbIsOpen primitives +// (and a test-only setter for the cooldown-expiry/half-open case, so we +// don't wait CB_COOLDOWN_MS=30s in real time), matching the executor's own +// module-level singleton so `execute()`'s cbIsOpen() gate observes the same +// state. + +describe("#6999 DDG circuit breaker state machine", () => { + test("starts closed", () => { + __setDdgCircuitBreakerStateForTests(0, 0); + assert.equal(cbIsOpen(), false, "breaker starts closed with no recorded failures"); + }); + + test("opens after CB_THRESHOLD consecutive recorded failures", () => { + __setDdgCircuitBreakerStateForTests(0, 0); + assert.equal(CB_THRESHOLD, 5, "sanity: this test assumes the documented threshold of 5"); + + for (let i = 1; i < CB_THRESHOLD; i++) { + cbRecordFailure(); + assert.equal(cbIsOpen(), false, `still closed after ${i} failure(s)`); + } + cbRecordFailure(); // the CB_THRESHOLD-th consecutive failure + assert.equal(cbIsOpen(), true, `opens on the ${CB_THRESHOLD}th consecutive failure`); + + __setDdgCircuitBreakerStateForTests(0, 0); // cleanup + }); + + test("execute() fast-fails with 503 while the breaker is open — no network call reached", async () => { + __setDdgCircuitBreakerStateForTests(CB_THRESHOLD, Date.now()); + + const executor = makeExecutor(); + const response = await executor.execute({ + model: "gpt-4o-mini", + body: { model: "gpt-4o-mini", messages: validMessages(), stream: false }, + stream: false, + credentials: {}, + }); + const httpResponse = + response instanceof Response ? response : (response as { response: Response }).response; + + assert.equal(httpResponse.status, 503, "open breaker fast-fails with 503"); + const parsedBody = (await httpResponse.json()) as { error?: { message?: string } }; + assert.match( + String(parsedBody.error?.message), + /circuit breaker/i, + "503 body should identify the circuit breaker as the cause" + ); + + __setDdgCircuitBreakerStateForTests(0, 0); // cleanup + }); + + test("half-open: breaker closes on its own once CB_COOLDOWN_MS has elapsed", () => { + // openedAt far enough in the past that "now - openedAt >= CB_COOLDOWN_MS" + // — simulates the cooldown window having elapsed without a real 30s wait. + __setDdgCircuitBreakerStateForTests(CB_THRESHOLD, Date.now() - CB_COOLDOWN_MS - 1); + + assert.equal( + cbIsOpen(), + false, + "cooldown elapsed -> breaker self-closes (half-open probe allowed through)" + ); + assert.equal( + __getDdgCircuitBreakerStateForTests().openedAt, + 0, + "cbIsOpen() must clear openedAt as a side effect of the half-open transition" + ); + }); + + test("a request made right after the breaker opens (cooldown not yet elapsed) still fast-fails", () => { + __setDdgCircuitBreakerStateForTests(CB_THRESHOLD, Date.now() - (CB_COOLDOWN_MS - 5_000)); + assert.equal(cbIsOpen(), true, "breaker stays open until the full cooldown elapses"); + __setDdgCircuitBreakerStateForTests(0, 0); // cleanup + }); + + test("cbRecordSuccess resets the failure counter (does not itself trip open)", () => { + __setDdgCircuitBreakerStateForTests(CB_THRESHOLD - 1, 0); + cbRecordSuccess(); + assert.equal( + __getDdgCircuitBreakerStateForTests().failures, + 0, + "a success clears the consecutive-failure count" + ); + + // Confirm the reset is real: it now takes a fresh run of CB_THRESHOLD + // failures to open, not just one more. + cbRecordFailure(); + assert.equal(cbIsOpen(), false, "one failure after a reset is not enough to open"); + + __setDdgCircuitBreakerStateForTests(0, 0); // cleanup + }); +});