diff --git a/src/modules/quizzes/ai-client.ts b/src/modules/quizzes/ai-client.ts index f01be64..ad74b6d 100644 --- a/src/modules/quizzes/ai-client.ts +++ b/src/modules/quizzes/ai-client.ts @@ -1,16 +1,20 @@ +import { z } from "zod"; import { config } from "../../config/index.js"; import { logger } from "../../utils/logger.js"; +import { createTransientRetryPolicy, createCircuitBreaker } from "../../utils/resilience.js"; -interface AiQuizQuestion { - prompt: string; - options: string[]; - correct_index: number; -} +const aiQuizQuestionSchema = z.object({ + prompt: z.string(), + options: z.array(z.string()), + correct_index: z.number().int(), +}); -interface AiQuizResponse { - quiz_id: string; - questions: AiQuizQuestion[]; -} +const aiQuizResponseSchema = z.object({ + quiz_id: z.string(), + questions: z.array(aiQuizQuestionSchema), +}); + +export type AiQuizQuestion = z.infer; export type AiDifficulty = "beginner" | "intermediate" | "advanced"; @@ -22,7 +26,10 @@ export interface GenerateQuizFromAIParams { numQuestions: number; } -export async function generateQuizFromAI( +const aiRetry = createTransientRetryPolicy("AI service", { maxAttempts: 3 }); +const aiBreaker = createCircuitBreaker({ label: "AI service" }); + +async function requestQuiz( params: GenerateQuizFromAIParams ): Promise { const controller = new AbortController(); @@ -50,8 +57,17 @@ export async function generateQuizFromAI( throw new Error(`AI service returned ${response.status}`); } - const data = (await response.json()) as AiQuizResponse; - return data.questions; + const raw = await response.json(); + const parsed = aiQuizResponseSchema.safeParse(raw); + if (!parsed.success) { + logger.error( + { issues: parsed.error.issues }, + "AI service returned a malformed response" + ); + throw new Error("AI service returned a malformed response"); + } + + return parsed.data.questions; } catch (err) { if (err instanceof Error && err.name === "AbortError") { logger.error({ timeout: config.AI_TIMEOUT_MS }, "AI service request timed out"); @@ -62,3 +78,15 @@ export async function generateQuizFromAI( clearTimeout(timeout); } } + +/** + * Requests a generated quiz from the chainlearn-ai service. Wrapped with the + * same retry + circuit breaker pattern used for Stellar calls: transient + * failures are retried with backoff, and a persistent outage trips the + * breaker so requests fail fast instead of blocking on every quiz request. + */ +export async function generateQuizFromAI( + params: GenerateQuizFromAIParams +): Promise { + return aiBreaker.execute(() => aiRetry.execute(() => requestQuiz(params))); +} diff --git a/src/services/retry-queue.ts b/src/services/retry-queue.ts index 4a7ab2e..d7758a4 100644 --- a/src/services/retry-queue.ts +++ b/src/services/retry-queue.ts @@ -50,15 +50,17 @@ export async function requeueReward(job: RetryJob): Promise { let processorRunning = false; let processorTimer: ReturnType | null = null; +let processorGeneration = 0; export async function startRetryProcessor( processFn: (job: RetryJob) => Promise ): Promise { if (processorRunning) return; processorRunning = true; + const generation = ++processorGeneration; const tick = async () => { - if (!processorRunning) return; + if (generation !== processorGeneration) return; try { const job = await dequeueReward(); if (job) { @@ -70,17 +72,18 @@ export async function startRetryProcessor( } catch (err) { logger.error({ err }, "Retry processor tick failed"); } - if (processorRunning) { + if (generation === processorGeneration) { processorTimer = setTimeout(tick, RETRY_INTERVAL_MS); } }; - tick(); + await tick(); logger.info("Retry processor started"); } export function stopRetryProcessor(): void { processorRunning = false; + processorGeneration++; if (processorTimer) { clearTimeout(processorTimer); processorTimer = null; diff --git a/src/stellar/resilience.ts b/src/stellar/resilience.ts index 7f70612..0cc2afd 100644 --- a/src/stellar/resilience.ts +++ b/src/stellar/resilience.ts @@ -1,144 +1,27 @@ -import { retry, handleType, ExponentialBackoff } from "cockatiel"; -import { logger } from "../utils/logger.js"; +import { + createTransientRetryPolicy, + createCircuitBreaker, + withTimeout, + isCircuitBreakerError, + CircuitState, + CircuitBreakerOpenError, + TimeoutError, +} from "../utils/resilience.js"; -function isTransientError(err: Error): boolean { - const name = err.name ?? ""; - const msg = err.message ?? ""; +export const stellarRetry = createTransientRetryPolicy("Stellar"); - if (name === "FetchError" || name === "HttpError") return true; - if ( - msg.includes("ECONNREFUSED") || - msg.includes("ETIMEDOUT") || - msg.includes("ECONNRESET") || - msg.includes("ENOTFOUND") || - msg.includes("socket hang up") - ) { - return true; - } - - const statusMatch = msg.match(/\b(502|503|504)\b/); - if (statusMatch) return true; - - return false; -} - -export const stellarRetry = retry( - handleType(Error, (err) => { - if (isTransientError(err)) { - logger.warn({ error: err.message }, "Stellar call retrying after transient error"); - return true; - } - return false; - }), - { backoff: new ExponentialBackoff() } -); - -// Circuit breaker implementation -export enum CircuitState { - Closed = "Closed", - Open = "Open", - HalfOpen = "HalfOpen", -} - -let circuitState = CircuitState.Closed; -let failureCount = 0; -let lastFailureTime = 0; -const THRESHOLD = 5; -const HALF_OPEN_AFTER = 30_000; - -function recordSuccess(): void { - failureCount = 0; - if (circuitState !== CircuitState.Closed) { - logger.info("Circuit breaker reset to closed"); - circuitState = CircuitState.Closed; - } -} - -function recordFailure(): void { - failureCount++; - lastFailureTime = Date.now(); - - // Transition to Open from Closed state when threshold is reached - if (failureCount >= THRESHOLD && circuitState === CircuitState.Closed) { - circuitState = CircuitState.Open; - logger.warn("Circuit breaker opened after consecutive failures"); - } - - // If probe fails in HalfOpen state, re-open the circuit - if (circuitState === CircuitState.HalfOpen) { - circuitState = CircuitState.Open; - logger.warn("Circuit breaker re-opened after probe failure in HalfOpen state"); - } -} +const stellarBreaker = createCircuitBreaker({ label: "Stellar" }); export function getCircuitState(): CircuitState { - if (circuitState === CircuitState.Open) { - if (Date.now() - lastFailureTime > HALF_OPEN_AFTER) { - circuitState = CircuitState.HalfOpen; - logger.info("Circuit breaker half-open — allowing probe request"); - } - } - return circuitState; + return stellarBreaker.getState(); } export function resetCircuitBreaker(): void { - circuitState = CircuitState.Closed; - failureCount = 0; - lastFailureTime = 0; + stellarBreaker.reset(); } -export async function circuitBreakerExecute(fn: () => Promise): Promise { - const state = getCircuitState(); - - if (state === CircuitState.Open) { - throw new CircuitBreakerOpenError("Circuit breaker is open"); - } - - try { - const result = await fn(); - recordSuccess(); - return result; - } catch (err) { - if (err instanceof Error && isTransientError(err)) { - recordFailure(); - } - throw err; - } -} - -export function withTimeout(promise: Promise, ms: number): Promise { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - reject(new TimeoutError(`Operation timed out after ${ms}ms`)); - }, ms); - - promise.then( - (value) => { - clearTimeout(timer); - resolve(value); - }, - (err) => { - clearTimeout(timer); - reject(err); - }, - ); - }); +export function circuitBreakerExecute(fn: () => Promise): Promise { + return stellarBreaker.execute(fn); } -export class CircuitBreakerOpenError extends Error { - constructor(message = "Circuit breaker is open") { - super(message); - this.name = "CircuitBreakerOpenError"; - } -} - -export class TimeoutError extends Error { - constructor(message = "Operation timed out") { - super(message); - this.name = "TimeoutError"; - } -} - -export function isCircuitBreakerError(err: unknown): boolean { - return err instanceof CircuitBreakerOpenError; -} +export { withTimeout, isCircuitBreakerError, CircuitState, CircuitBreakerOpenError, TimeoutError }; diff --git a/src/utils/resilience.ts b/src/utils/resilience.ts new file mode 100644 index 0000000..d8e5977 --- /dev/null +++ b/src/utils/resilience.ts @@ -0,0 +1,169 @@ +import { retry, handleType, ExponentialBackoff, type RetryPolicy } from "cockatiel"; +import { logger } from "./logger.js"; + +export function isTransientError(err: Error): boolean { + const name = err.name ?? ""; + const msg = err.message ?? ""; + + if (name === "FetchError" || name === "HttpError" || name === "AbortError" || name === "TimeoutError") { + return true; + } + if ( + msg.includes("ECONNREFUSED") || + msg.includes("ETIMEDOUT") || + msg.includes("ECONNRESET") || + msg.includes("ENOTFOUND") || + msg.includes("socket hang up") || + msg.includes("timed out") + ) { + return true; + } + + const statusMatch = msg.match(/\b(502|503|504)\b/); + if (statusMatch) return true; + + return false; +} + +export interface RetryPolicyOptions { + maxAttempts?: number; +} + +export function createTransientRetryPolicy( + label: string, + options: RetryPolicyOptions = {} +): RetryPolicy { + return retry( + handleType(Error, (err) => { + if (isTransientError(err)) { + logger.warn({ error: err.message }, `${label} call retrying after transient error`); + return true; + } + return false; + }), + { backoff: new ExponentialBackoff(), maxAttempts: options.maxAttempts } + ); +} + +export enum CircuitState { + Closed = "Closed", + Open = "Open", + HalfOpen = "HalfOpen", +} + +export class CircuitBreakerOpenError extends Error { + constructor(message = "Circuit breaker is open") { + super(message); + this.name = "CircuitBreakerOpenError"; + } +} + +export class TimeoutError extends Error { + constructor(message = "Operation timed out") { + super(message); + this.name = "TimeoutError"; + } +} + +export function isCircuitBreakerError(err: unknown): boolean { + return err instanceof CircuitBreakerOpenError; +} + +export interface CircuitBreaker { + execute(fn: () => Promise): Promise; + getState(): CircuitState; + reset(): void; +} + +export interface CircuitBreakerOptions { + label: string; + threshold?: number; + halfOpenAfterMs?: number; +} + +export function createCircuitBreaker(options: CircuitBreakerOptions): CircuitBreaker { + const { label, threshold = 5, halfOpenAfterMs = 30_000 } = options; + + let circuitState = CircuitState.Closed; + let failureCount = 0; + let lastFailureTime = 0; + + function recordSuccess(): void { + failureCount = 0; + if (circuitState !== CircuitState.Closed) { + logger.info({ label }, "Circuit breaker reset to closed"); + circuitState = CircuitState.Closed; + } + } + + function recordFailure(): void { + failureCount++; + lastFailureTime = Date.now(); + + if (failureCount >= threshold && circuitState === CircuitState.Closed) { + circuitState = CircuitState.Open; + logger.warn({ label }, "Circuit breaker opened after consecutive failures"); + } + + if (circuitState === CircuitState.HalfOpen) { + circuitState = CircuitState.Open; + logger.warn({ label }, "Circuit breaker re-opened after probe failure in HalfOpen state"); + } + } + + function getState(): CircuitState { + if (circuitState === CircuitState.Open) { + if (Date.now() - lastFailureTime > halfOpenAfterMs) { + circuitState = CircuitState.HalfOpen; + logger.info({ label }, "Circuit breaker half-open — allowing probe request"); + } + } + return circuitState; + } + + function reset(): void { + circuitState = CircuitState.Closed; + failureCount = 0; + lastFailureTime = 0; + } + + async function execute(fn: () => Promise): Promise { + const state = getState(); + + if (state === CircuitState.Open) { + throw new CircuitBreakerOpenError(`Circuit breaker is open for ${label}`); + } + + try { + const result = await fn(); + recordSuccess(); + return result; + } catch (err) { + if (err instanceof Error && isTransientError(err)) { + recordFailure(); + } + throw err; + } + } + + return { execute, getState, reset }; +} + +export function withTimeout(promise: Promise, ms: number): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new TimeoutError(`Operation timed out after ${ms}ms`)); + }, ms); + + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (err) => { + clearTimeout(timer); + reject(err); + }, + ); + }); +} diff --git a/tests/unit/quizzes/ai-client.test.ts b/tests/unit/quizzes/ai-client.test.ts new file mode 100644 index 0000000..9f89216 --- /dev/null +++ b/tests/unit/quizzes/ai-client.test.ts @@ -0,0 +1,140 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("../../../src/config/index.js", () => ({ + config: { + AI_SERVICE_URL: "http://ai.test", + AI_TIMEOUT_MS: 200, + }, +})); + +vi.mock("../../../src/utils/logger.js", () => ({ + logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), fatal: vi.fn() }, +})); + +const baseParams = { + userId: "user-1", + courseId: "course-1", + moduleId: "module-1", + difficulty: "beginner" as const, + numQuestions: 2, +}; + +async function loadClient() { + vi.resetModules(); + return import("../../../src/modules/quizzes/ai-client.js"); +} + +function jsonResponse(status: number, body: unknown) { + return { ok: status >= 200 && status < 300, status, json: async () => body }; +} + +describe("generateQuizFromAI", () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("returns parsed questions on a valid response", async () => { + const validBody = { + quiz_id: "quiz-1", + questions: [{ prompt: "Q1", options: ["a", "b"], correct_index: 0 }], + }; + vi.stubGlobal("fetch", vi.fn().mockResolvedValue(jsonResponse(200, validBody))); + + const { generateQuizFromAI } = await loadClient(); + const result = await generateQuizFromAI(baseParams); + + expect(result).toEqual(validBody.questions); + }); + + it("throws when the AI response shape is malformed instead of crashing (#142)", async () => { + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue(jsonResponse(200, { quiz_id: 123, questions: "not-an-array" })) + ); + + const { generateQuizFromAI } = await loadClient(); + + await expect(generateQuizFromAI(baseParams)).rejects.toThrow( + "AI service returned a malformed response" + ); + }); + + it("throws on a response missing the questions field entirely (#142)", async () => { + vi.stubGlobal("fetch", vi.fn().mockResolvedValue(jsonResponse(200, { quiz_id: "quiz-1" }))); + + const { generateQuizFromAI } = await loadClient(); + + await expect(generateQuizFromAI(baseParams)).rejects.toThrow( + "AI service returned a malformed response" + ); + }); + + it("retries transient failures and eventually succeeds (#141)", async () => { + const validBody = { + quiz_id: "quiz-1", + questions: [{ prompt: "Q1", options: ["a", "b"], correct_index: 1 }], + }; + const fetchMock = vi + .fn() + .mockRejectedValueOnce(new Error("connect ECONNREFUSED 127.0.0.1:8000")) + .mockResolvedValueOnce(jsonResponse(200, validBody)); + vi.stubGlobal("fetch", fetchMock); + + const { generateQuizFromAI } = await loadClient(); + const result = await generateQuizFromAI(baseParams); + + expect(result).toEqual(validBody.questions); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("does not retry non-transient (4xx) failures", async () => { + const fetchMock = vi.fn().mockResolvedValue(jsonResponse(400, {})); + vi.stubGlobal("fetch", fetchMock); + + const { generateQuizFromAI } = await loadClient(); + + await expect(generateQuizFromAI(baseParams)).rejects.toThrow("AI service returned 400"); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("opens the circuit after repeated failures and fails fast without calling fetch (#141)", async () => { + const fetchMock = vi.fn().mockRejectedValue(new Error("connect ECONNREFUSED 127.0.0.1:8000")); + vi.stubGlobal("fetch", fetchMock); + + const { generateQuizFromAI } = await loadClient(); + + for (let i = 0; i < 5; i++) { + await expect(generateQuizFromAI(baseParams)).rejects.toThrow(); + } + + fetchMock.mockClear(); + + await expect(generateQuizFromAI(baseParams)).rejects.toThrow("Circuit breaker is open"); + expect(fetchMock).not.toHaveBeenCalled(); + }, 10_000); + + it("aborts and reports a timeout instead of hanging when the AI service never responds (#140 regression)", async () => { + vi.stubGlobal( + "fetch", + vi.fn().mockImplementation((_url: string, init: RequestInit) => { + return new Promise((_resolve, reject) => { + init.signal?.addEventListener("abort", () => { + const err = new Error("This operation was aborted"); + err.name = "AbortError"; + reject(err); + }); + }); + }) + ); + + const { generateQuizFromAI } = await loadClient(); + + await expect(generateQuizFromAI(baseParams)).rejects.toThrow( + "AI service request timed out after 200ms" + ); + }, 10_000); +}); diff --git a/tests/unit/services/retry-queue.test.ts b/tests/unit/services/retry-queue.test.ts index f319af3..37eca9f 100644 --- a/tests/unit/services/retry-queue.test.ts +++ b/tests/unit/services/retry-queue.test.ts @@ -166,6 +166,46 @@ describe("Retry Queue", () => { expect(processFn).toHaveBeenCalledWith(job); }); + it("should not spawn a duplicate loop when restarted while the previous tick is still in flight", async () => { + vi.useFakeTimers(); + try { + let resolveStaleDequeue: (value: string | null) => void = () => {}; + const staleDequeue = new Promise((resolve) => { + resolveStaleDequeue = resolve; + }); + mockRedis.rpop.mockReturnValueOnce(staleDequeue as ReturnType); + + const staleProcessFn = vi.fn().mockResolvedValue(true); + const stalePromise = startRetryProcessor(staleProcessFn); + + // Let the stale tick reach its `await dequeueReward()` point without resolving it. + await Promise.resolve(); + await Promise.resolve(); + + // Stop before the stale tick completes, then immediately restart. + stopRetryProcessor(); + + mockRedis.rpop.mockResolvedValue(null); + const freshProcessFn = vi.fn().mockResolvedValue(true); + await startRetryProcessor(freshProcessFn); + + // Now let the stale tick's dequeue finally resolve. + resolveStaleDequeue(null); + await stalePromise; + await Promise.resolve(); + + mockRedis.rpop.mockClear(); + await vi.advanceTimersByTimeAsync(30_000); + + // Only the fresh loop should still be ticking — a dangling stale loop + // would double this call count. + expect(mockRedis.rpop).toHaveBeenCalledTimes(1); + } finally { + stopRetryProcessor(); + vi.useRealTimers(); + } + }); + it("should requeue failed jobs", async () => { const processFn = vi.fn().mockResolvedValue(false); const job = {