From 482425487c0265fa2a80ad13940600fd0aeb2b1e Mon Sep 17 00:00:00 2001 From: balisdev <294588434+balisdev@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:21:57 +0100 Subject: [PATCH 1/3] fix(services): prevent dangling retry-processor loop on restart startRetryProcessor fired its first tick without awaiting it and used a plain boolean flag to gate the recursive setTimeout loop. If the processor was restarted while an earlier tick was still in flight (e.g. stop immediately followed by start), the stale tick would see the flag flipped back to true and reschedule itself, running two overlapping processing loops. Await the first tick and gate scheduling with a generation counter instead of a boolean, so a stale in-flight tick can never reschedule itself after a restart. --- src/services/retry-queue.ts | 9 ++++-- tests/unit/services/retry-queue.test.ts | 40 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) 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/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 = { From c89b1dd4fd6b91ae78c8d9e1c8d5e0d4156e9273 Mon Sep 17 00:00:00 2001 From: balisdev <294588434+balisdev@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:22:06 +0100 Subject: [PATCH 2/3] refactor(resilience): extract reusable retry/circuit-breaker factory Pull the retry policy and circuit breaker logic out of src/stellar/resilience.ts into a generic src/utils/resilience.ts factory, so other external service clients can get the same retry+breaker+timeout protection without sharing Stellar's circuit breaker state. src/stellar/resilience.ts keeps its exact public API, now backed by the shared factory. Also treat AbortError/TimeoutError and "timed out" messages as transient, so a hanging dependency actually gets retried and trips the breaker instead of silently bypassing both. --- src/stellar/resilience.ts | 149 ++++----------------------------- src/utils/resilience.ts | 169 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 133 deletions(-) create mode 100644 src/utils/resilience.ts 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); + }, + ); + }); +} From f2c549f37ff0c3a0d7674ac319011c8dfaa23f93 Mon Sep 17 00:00:00 2001 From: balisdev <294588434+balisdev@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:22:15 +0100 Subject: [PATCH 3/3] feat(quizzes): add resilience and response validation to AI client The chainlearn-ai client had a request timeout but no retry or circuit breaker (unlike Stellar calls), so a persistent AI service outage hung every quiz request for the full timeout duration. It also cast the parsed response straight to AiQuizResponse with no runtime check, so a malformed body would crash instead of falling back gracefully. - Wrap the AI service call with the same retry + circuit breaker pattern used for Stellar calls, bounded to 3 attempts per request so a call still fails predictably rather than retrying forever. - Validate the response body with a zod schema and reject malformed shapes with a descriptive error instead of an unchecked cast. Both failure modes are still caught by quizService.generateQuiz's existing fallback to placeholder questions. Closes #139, Closes #140, Closes #141, Closes #142 --- src/modules/quizzes/ai-client.ts | 52 +++++++--- tests/unit/quizzes/ai-client.test.ts | 140 +++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 12 deletions(-) create mode 100644 tests/unit/quizzes/ai-client.test.ts 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/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); +});