diff --git a/packages/service-kit/src/circuit-breaker.test.ts b/packages/service-kit/src/circuit-breaker.test.ts new file mode 100644 index 0000000..1f50bbf --- /dev/null +++ b/packages/service-kit/src/circuit-breaker.test.ts @@ -0,0 +1,231 @@ +import { describe, expect, it, vi } from "vitest"; +import { + circuitBreakerLimitsFromEnv, + CircuitOpenError, + createCircuitBreaker, +} from "./circuit-breaker"; + +function fakeClock(startAt = 0) { + let t = startAt; + return { now: () => t, advance: (ms: number) => (t += ms) }; +} + +describe("createCircuitBreaker — closed state", () => { + it("starts closed and runs calls normally", async () => { + const breaker = createCircuitBreaker({ failureThreshold: 3, cooldownMs: 1_000 }); + expect(breaker.state).toBe("closed"); + await expect(breaker.execute(async () => "ok")).resolves.toBe("ok"); + expect(breaker.state).toBe("closed"); + }); + + it("stays closed after fewer consecutive failures than the threshold", async () => { + const breaker = createCircuitBreaker({ failureThreshold: 3, cooldownMs: 1_000 }); + const failing = () => Promise.reject(new Error("downstream 500")); + + await expect(breaker.execute(failing)).rejects.toThrow("downstream 500"); + await expect(breaker.execute(failing)).rejects.toThrow("downstream 500"); + expect(breaker.state).toBe("closed"); // 2 failures, threshold is 3 + }); + + it("an intermittent success resets the consecutive-failure count", async () => { + const breaker = createCircuitBreaker({ failureThreshold: 3, cooldownMs: 1_000 }); + const failing = () => Promise.reject(new Error("downstream 500")); + + await expect(breaker.execute(failing)).rejects.toThrow(); + await expect(breaker.execute(failing)).rejects.toThrow(); + await expect(breaker.execute(async () => "ok")).resolves.toBe("ok"); // resets counter + await expect(breaker.execute(failing)).rejects.toThrow(); + await expect(breaker.execute(failing)).rejects.toThrow(); + // 2 failures again since the reset — still under the threshold of 3. + expect(breaker.state).toBe("closed"); + }); +}); + +describe("createCircuitBreaker — opening", () => { + it("opens exactly when consecutive failures reach the threshold", async () => { + const breaker = createCircuitBreaker({ failureThreshold: 3, cooldownMs: 1_000 }); + const failing = () => Promise.reject(new Error("downstream 500")); + + await expect(breaker.execute(failing)).rejects.toThrow(); + expect(breaker.state).toBe("closed"); + await expect(breaker.execute(failing)).rejects.toThrow(); + expect(breaker.state).toBe("closed"); + await expect(breaker.execute(failing)).rejects.toThrow(); + expect(breaker.state).toBe("open"); // 3rd consecutive failure trips it + }); + + it("fast-fails with CircuitOpenError once open — the wrapped fn is never called again", async () => { + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 10_000 }); + const fn = vi.fn(() => Promise.reject(new Error("downstream 500"))); + + await expect(breaker.execute(fn)).rejects.toThrow("downstream 500"); + expect(breaker.state).toBe("open"); + + await expect(breaker.execute(fn)).rejects.toBeInstanceOf(CircuitOpenError); + expect(fn).toHaveBeenCalledTimes(1); // NOT called again — fast-fail, no network attempt + }); + + it("CircuitOpenError reports the remaining cooldown", async () => { + const clock = fakeClock(); + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 10_000, now: clock.now }); + await expect(breaker.execute(() => Promise.reject(new Error("x")))).rejects.toThrow(); + + clock.advance(4_000); + const err = await breaker.execute(() => Promise.reject(new Error("unreachable"))).catch((e) => e); + expect(err).toBeInstanceOf(CircuitOpenError); + expect((err as CircuitOpenError).retryAfterMs).toBe(6_000); + }); +}); + +describe("createCircuitBreaker — half-open transition and resolution", () => { + it("allows exactly one trial call after the cooldown elapses", async () => { + const clock = fakeClock(); + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 10_000, now: clock.now }); + await expect(breaker.execute(() => Promise.reject(new Error("x")))).rejects.toThrow(); + expect(breaker.state).toBe("open"); + + clock.advance(10_000); + const fn = vi.fn(async () => "recovered"); + await expect(breaker.execute(fn)).resolves.toBe("recovered"); + expect(fn).toHaveBeenCalledTimes(1); + expect(breaker.state).toBe("closed"); // trial succeeded — fully closed again + }); + + it("a failing trial call re-opens the breaker with a fresh cooldown", async () => { + const clock = fakeClock(); + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 10_000, now: clock.now }); + await expect(breaker.execute(() => Promise.reject(new Error("x")))).rejects.toThrow(); + + clock.advance(10_000); // cooldown elapsed + await expect(breaker.execute(() => Promise.reject(new Error("still down")))).rejects.toThrow( + "still down", + ); + expect(breaker.state).toBe("open"); + + // Immediately after the failed trial, the ORIGINAL cooldown has not + // magically re-elapsed — the fresh window starts from the trial's + // failure time, not the original open time. + const err = await breaker.execute(() => Promise.reject(new Error("unreachable"))).catch((e) => e); + expect(err).toBeInstanceOf(CircuitOpenError); + }); + + it("does not allow a trial call before the cooldown has elapsed", async () => { + const clock = fakeClock(); + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 10_000, now: clock.now }); + await expect(breaker.execute(() => Promise.reject(new Error("x")))).rejects.toThrow(); + + clock.advance(9_999); // one ms short + const fn = vi.fn(async () => "should not run"); + await expect(breaker.execute(fn)).rejects.toBeInstanceOf(CircuitOpenError); + expect(fn).not.toHaveBeenCalled(); + }); +}); + +describe("createCircuitBreaker — beforeCall/recordOutcome (hook-based callers, e.g. @fastify/http-proxy)", () => { + it("beforeCall throws CircuitOpenError when open, without needing a wrapped promise", () => { + const clock = fakeClock(); + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 5_000, now: clock.now }); + breaker.recordOutcome("failure"); + expect(breaker.state).toBe("open"); + + expect(() => breaker.beforeCall()).toThrow(CircuitOpenError); + }); + + it("beforeCall allows the half-open trial through after cooldown, matching execute's behavior", () => { + const clock = fakeClock(); + const breaker = createCircuitBreaker({ failureThreshold: 1, cooldownMs: 5_000, now: clock.now }); + breaker.recordOutcome("failure"); + clock.advance(5_000); + + expect(() => breaker.beforeCall()).not.toThrow(); + expect(breaker.state).toBe("half_open"); + }); + + it("recordOutcome drives the exact same state machine execute uses internally", () => { + const breaker = createCircuitBreaker({ failureThreshold: 2, cooldownMs: 1_000 }); + breaker.beforeCall(); + breaker.recordOutcome("failure"); + expect(breaker.state).toBe("closed"); + breaker.beforeCall(); + breaker.recordOutcome("failure"); + expect(breaker.state).toBe("open"); + }); +}); + +describe("createCircuitBreaker — onStateChange metric hook (#326)", () => { + it("fires exactly once per real transition, not on every call", async () => { + const clock = fakeClock(); + const transitions: Array<[string, string]> = []; + const breaker = createCircuitBreaker({ + failureThreshold: 2, + cooldownMs: 1_000, + now: clock.now, + onStateChange: (from, to) => transitions.push([from, to]), + }); + const failing = () => Promise.reject(new Error("x")); + + await expect(breaker.execute(failing)).rejects.toThrow(); // 1st failure — still closed, no transition + expect(transitions).toEqual([]); + + await expect(breaker.execute(failing)).rejects.toThrow(); // 2nd — trips open + expect(transitions).toEqual([["closed", "open"]]); + + clock.advance(1_000); + await expect(breaker.execute(failing)).rejects.toThrow(); // trial fails — re-opens (closed->open would be wrong; it's open->half_open->open) + expect(transitions).toEqual([ + ["closed", "open"], + ["open", "half_open"], + ["half_open", "open"], + ]); + }); + + it("covers the full closed -> open -> half_open -> closed cycle", async () => { + const clock = fakeClock(); + const transitions: Array<[string, string]> = []; + const breaker = createCircuitBreaker({ + failureThreshold: 1, + cooldownMs: 500, + now: clock.now, + onStateChange: (from, to) => transitions.push([from, to]), + }); + + await expect(breaker.execute(() => Promise.reject(new Error("x")))).rejects.toThrow(); + clock.advance(500); + await expect(breaker.execute(async () => "ok")).resolves.toBe("ok"); + + expect(transitions).toEqual([ + ["closed", "open"], + ["open", "half_open"], + ["half_open", "closed"], + ]); + }); +}); + +describe("circuitBreakerLimitsFromEnv", () => { + it("uses defaults when env vars are unset", () => { + const limits = circuitBreakerLimitsFromEnv( + { failureThresholdVar: "CB_FAIL_THRESHOLD", cooldownMsVar: "CB_COOLDOWN_MS" }, + { defaultFailureThreshold: 5, defaultCooldownMs: 30_000 }, + {}, + ); + expect(limits).toEqual({ failureThreshold: 5, cooldownMs: 30_000 }); + }); + + it("reads configured values from env", () => { + const limits = circuitBreakerLimitsFromEnv( + { failureThresholdVar: "CB_FAIL_THRESHOLD", cooldownMsVar: "CB_COOLDOWN_MS" }, + { defaultFailureThreshold: 5, defaultCooldownMs: 30_000 }, + { CB_FAIL_THRESHOLD: "10", CB_COOLDOWN_MS: "5000" }, + ); + expect(limits).toEqual({ failureThreshold: 10, cooldownMs: 5_000 }); + }); + + it("falls back to defaults for an invalid (non-positive, non-numeric) env value", () => { + const limits = circuitBreakerLimitsFromEnv( + { failureThresholdVar: "CB_FAIL_THRESHOLD", cooldownMsVar: "CB_COOLDOWN_MS" }, + { defaultFailureThreshold: 5, defaultCooldownMs: 30_000 }, + { CB_FAIL_THRESHOLD: "-1", CB_COOLDOWN_MS: "not-a-number" }, + ); + expect(limits).toEqual({ failureThreshold: 5, cooldownMs: 30_000 }); + }); +}); diff --git a/packages/service-kit/src/circuit-breaker.ts b/packages/service-kit/src/circuit-breaker.ts new file mode 100644 index 0000000..590817b --- /dev/null +++ b/packages/service-kit/src/circuit-breaker.ts @@ -0,0 +1,154 @@ +// Circuit breaker for calls to a downstream service (#326). A downstream +// outage otherwise cascades into slow gateway responses — every caller +// waits out the same timeout against the same broken dependency. Once a +// configured number of consecutive failures is seen, the breaker OPENs and +// every call fails fast (no network attempt at all) until a cooldown +// elapses; it then allows exactly one HALF_OPEN trial call to decide +// whether to CLOSE (resume normal traffic) or re-OPEN. +// +// Deliberately dependency-free (no `opossum`/`cockatiel` etc.) — the state +// machine itself is small, and this repo has no existing circuit-breaker +// library dependency to build on; see budget.ts for the same +// pure-function-plus-explicit-clock style this follows. + +export type CircuitState = "closed" | "open" | "half_open"; + +export interface CircuitBreakerOptions { + /** Consecutive failures (while closed) that trip the breaker open. */ + failureThreshold: number; + /** Once open, how long (ms) before allowing a half-open trial call. */ + cooldownMs: number; + /** Injectable clock, for deterministic tests. Defaults to `Date.now`. */ + now?: () => number; + /** Called on every state transition — the "metric tracking circuit + * breaker state changes" #326 asks for. Not called for a call that + * doesn't change state (e.g. a second consecutive failure while already + * open). */ + onStateChange?: (from: CircuitState, to: CircuitState) => void; +} + +export class CircuitOpenError extends Error { + constructor(readonly retryAfterMs: number) { + super(`circuit is open; retry after ${retryAfterMs}ms`); + this.name = "CircuitOpenError"; + } +} + +export interface CircuitBreaker { + readonly state: CircuitState; + /** Throws `CircuitOpenError` (fast-fail, no call attempted) when open and + * the cooldown hasn't elapsed yet. Otherwise runs `fn`, recording the + * outcome against the breaker's state machine. A half-open trial that + * succeeds closes the breaker; one that fails re-opens it (a fresh + * cooldown window starting now, not extending the original). */ + execute(fn: () => Promise): Promise; + /** Lower-level pair `execute` is built on, for callers that can't wrap + * the call itself in a promise this module controls — e.g. + * `@fastify/http-proxy`, which reports success/failure via separate + * `onResponse`/`onError` hooks rather than a promise this code awaits. + * Call `beforeCall()` where `execute` would check state (throws + * `CircuitOpenError` the same way); call `recordOutcome(...)` from + * whichever hook fires once the real call's outcome is known. */ + beforeCall(): void; + recordOutcome(outcome: "success" | "failure"): void; +} + +/** + * Creates a breaker starting `closed`. Every consecutive failure while + * closed increments an internal counter; reaching `failureThreshold` opens + * the breaker with a cooldown clock started at that moment. Any success + * while closed resets the counter to 0 (an intermittent failure that never + * reaches the threshold never trips the breaker) — this is a consecutive- + * failure count, not a rolling error rate. + */ +export function createCircuitBreaker(options: CircuitBreakerOptions): CircuitBreaker { + const now = options.now ?? Date.now; + let state: CircuitState = "closed"; + let consecutiveFailures = 0; + let openedAt = 0; + + function transition(to: CircuitState) { + if (to === state) return; + const from = state; + state = to; + options.onStateChange?.(from, to); + } + + function beforeCall(): void { + if (state === "open") { + const elapsed = now() - openedAt; + if (elapsed < options.cooldownMs) { + throw new CircuitOpenError(options.cooldownMs - elapsed); + } + // Cooldown elapsed — allow exactly this one trial call through. + transition("half_open"); + } + } + + function recordOutcome(outcome: "success" | "failure"): void { + if (outcome === "success") { + // A successful call, whether closed or the half-open trial, means + // the dependency is healthy again. + consecutiveFailures = 0; + transition("closed"); + return; + } + if (state === "half_open") { + // The trial failed — re-open with a fresh cooldown window. + openedAt = now(); + transition("open"); + } else { + consecutiveFailures += 1; + if (consecutiveFailures >= options.failureThreshold) { + openedAt = now(); + transition("open"); + } + } + } + + return { + get state() { + return state; + }, + + beforeCall, + recordOutcome, + + async execute(fn: () => Promise): Promise { + beforeCall(); + try { + const result = await fn(); + recordOutcome("success"); + return result; + } catch (err) { + recordOutcome("failure"); + throw err; + } + }, + }; +} + +export interface CircuitBreakerLimits { + failureThreshold: number; + cooldownMs: number; +} + +/** Build breaker limits from env, mirroring `budgetLimitsFromEnv`'s + * env-with-explicit-defaults shape. */ +export function circuitBreakerLimitsFromEnv( + vars: { failureThresholdVar: string; cooldownMsVar: string }, + defaults: { defaultFailureThreshold: number; defaultCooldownMs: number }, + env: Record = process.env, +): CircuitBreakerLimits { + const thresholdRaw = env[vars.failureThresholdVar]; + const cooldownRaw = env[vars.cooldownMsVar]; + const failureThreshold = thresholdRaw ? Number(thresholdRaw) : defaults.defaultFailureThreshold; + const cooldownMs = cooldownRaw ? Number(cooldownRaw) : defaults.defaultCooldownMs; + return { + failureThreshold: + Number.isFinite(failureThreshold) && failureThreshold > 0 + ? failureThreshold + : defaults.defaultFailureThreshold, + cooldownMs: Number.isFinite(cooldownMs) && cooldownMs > 0 ? cooldownMs : defaults.defaultCooldownMs, + }; +} diff --git a/packages/service-kit/src/index.ts b/packages/service-kit/src/index.ts index bc7f16a..0a0960f 100644 --- a/packages/service-kit/src/index.ts +++ b/packages/service-kit/src/index.ts @@ -55,6 +55,16 @@ export { export { createPgSpendBudget, type BudgetDb, type PgBudgetConfig } from "./pg-budget"; +export { + createCircuitBreaker, + circuitBreakerLimitsFromEnv, + CircuitOpenError, + type CircuitBreaker, + type CircuitBreakerOptions, + type CircuitBreakerLimits, + type CircuitState, +} from "./circuit-breaker"; + export { retryWithBackoff, MaxRetriesExceededError, diff --git a/packages/service-kit/src/metrics.ts b/packages/service-kit/src/metrics.ts index 294dc95..8306f50 100644 --- a/packages/service-kit/src/metrics.ts +++ b/packages/service-kit/src/metrics.ts @@ -189,11 +189,36 @@ const workerQueueDepth = new Gauge({ }); assertMetricName("vela_worker_queue_depth"); +// Referenced by domainMetrics/metrics.test.ts but not previously defined +// anywhere in this file (a real bug on dev — importing this module would +// throw `workerProcessingLagSeconds is not defined` at module load time). +// Restored here matching workerQueueDepth's sibling shape exactly, per the +// existing test's expectations (metrics.test.ts: `.set({ service }, 3.5)`, +// exposition-format assertion against `vela_worker_processing_lag_seconds`). +const workerProcessingLagSeconds = new Gauge({ + name: "vela_worker_processing_lag_seconds", + help: "Time between a verification job being enqueued and picked up for processing", + labelNames: ["service"] as const, + registers: [registry], +}); +assertMetricName("vela_worker_processing_lag_seconds"); + const walletPasskeyAuthRateLimited = outcomeCounter( "vela_wallet_passkey_auth_rate_limited_total", "Rate-limited passkey auth (connect) attempts", ); +/** Circuit breaker state changes (#326) — labeled by the downstream call + * being protected (e.g. "verification-service") and the transition's + * endpoints, so an alert can fire specifically on `to="open"`. */ +const circuitBreakerStateChanges = new Counter({ + name: "vela_circuit_breaker_state_changes_total", + help: "Circuit breaker state transitions", + labelNames: ["service", "breaker", "from", "to"] as const, + registers: [registry], +}); +assertMetricName("vela_circuit_breaker_state_changes_total"); + export const domainMetrics = { walletCreated, walletPasskeyAuth, @@ -213,6 +238,7 @@ export const domainMetrics = { verificationTurnaround: workerVerificationTurnaround, workerQueueDepth, workerProcessingLagSeconds, + circuitBreakerStateChanges, } as const; export type Outcome = "success" | "failure"; diff --git a/services/api-gateway/README.md b/services/api-gateway/README.md index b2aec1b..78d0d6a 100644 --- a/services/api-gateway/README.md +++ b/services/api-gateway/README.md @@ -88,6 +88,25 @@ The API Gateway enforces strict origin verification at the boundary: - **Disallowed Origins**: Any unlisted origin fails preflight checks and will not receive an `Access-Control-Allow-Origin` header. Unified API entrypoint: auth/session middleware, rate limiting, request tracing, client routing +## Circuit breaker for verification-service (#326) + +`/verification/*` proxying to `verification-service` is protected by a +circuit breaker (`@vellar/service-kit`'s `createCircuitBreaker`) so a +downstream outage fails fast instead of cascading into slow gateway +responses: + +| Env var | Default | Meaning | +|---|---|---| +| `VERIFICATION_CB_FAILURE_THRESHOLD` | `5` | Consecutive connection-level failures (timeouts, refused connections — NOT a normal 4xx/5xx from a reachable upstream) before the breaker opens. | +| `VERIFICATION_CB_COOLDOWN_MS` | `30000` | How long the breaker stays open before allowing one half-open trial call through. | + +While open, requests to `/verification/*` respond `503` immediately with +`{"error": "verification_service_unavailable", "retryAfterMs": }` — +no network attempt is made. State transitions +(`closed`↔`open`↔`half_open`) are logged and recorded in the +`vela_circuit_breaker_state_changes_total{breaker="verification-service"}` +Prometheus counter exposed at `/metrics`. + ## Canary deploy stage (#336) ### What this covers, honestly diff --git a/services/api-gateway/src/register-proxy-route.ts b/services/api-gateway/src/register-proxy-route.ts index 419d805..06ce71b 100644 --- a/services/api-gateway/src/register-proxy-route.ts +++ b/services/api-gateway/src/register-proxy-route.ts @@ -1,6 +1,9 @@ import type { FastifyInstance } from "fastify"; import proxy from "@fastify/http-proxy"; +type ProxyPreHandler = NonNullable[1]>["preHandler"]; +type ProxyReplyOptions = NonNullable[1]>["replyOptions"]; + // Shared route registration helper (issue #355). // // Every downstream service is proxied with the same three-field config: @@ -24,6 +27,19 @@ export interface ProxyRouteOptions { * universal case where the gateway prefix and the backend prefix are the same. */ rewritePrefix?: string; + /** + * Runs before the proxy forwards the request — the per-route extension + * point this helper's own docstring anticipated ("circuit breakers, or + * observability", #326). Return a reply to short-circuit (e.g. fast-fail + * while a circuit breaker is open) without reaching the upstream at all. + */ + preHandler?: ProxyPreHandler; + /** + * Passed straight through to `@fastify/http-proxy`'s `replyOptions` — + * `onResponse`/`onError` hooks for observing the real outcome of each + * proxied call (e.g. recording it against a circuit breaker, #326). + */ + replyOptions?: ProxyReplyOptions; } /** @@ -40,6 +56,6 @@ export interface ProxyRouteOptions { * ``` */ export function registerProxyRoute(app: FastifyInstance, options: ProxyRouteOptions): void { - const { upstream, prefix, rewritePrefix = prefix } = options; - app.register(proxy, { upstream, prefix, rewritePrefix }); + const { upstream, prefix, rewritePrefix = prefix, preHandler, replyOptions } = options; + app.register(proxy, { upstream, prefix, rewritePrefix, preHandler, replyOptions }); } diff --git a/services/api-gateway/src/server.test.ts b/services/api-gateway/src/server.test.ts index 01b9db6..1d14087 100644 --- a/services/api-gateway/src/server.test.ts +++ b/services/api-gateway/src/server.test.ts @@ -126,6 +126,100 @@ describe("api-gateway", () => { }); }); +describe("api-gateway circuit breaker for verification-service (#326)", () => { + it("opens after consecutive connection failures, fast-fails with 503, then recovers", async () => { + // A real upstream that starts closed (so calls hit a real ECONNREFUSED), + // then gets started once we want to observe recovery — this exercises + // the real fastify-http-proxy connection-failure path, not a simulated + // error. + const verificationUpstream = Fastify(); + verificationUpstream.get("/verification/ping", async () => ({ status: "verified" })); + + // Reserve a port, then immediately close it so it's guaranteed nothing + // is listening there when the gateway starts making requests. + const probe = Fastify(); + await probe.listen({ port: 0, host: "127.0.0.1" }); + const { port } = probe.server.address() as AddressInfo; + await probe.close(); + + const cbApp = buildServer({ + walletServiceUrl: "http://127.0.0.1:1", + verificationServiceUrl: `http://127.0.0.1:${port}`, + verificationCircuitFailureThreshold: 2, + verificationCircuitCooldownMs: 200, + }); + await cbApp.ready(); + + try { + // First 2 calls hit the real (nothing-listening) port and fail — + // proxied as a connection-level error, not a 4xx/5xx from a real + // server. The breaker counts these as failures. + const first = await cbApp.inject({ method: "GET", url: "/verification/ping" }); + expect(first.statusCode).toBeGreaterThanOrEqual(500); // connection refused -> proxy error + const second = await cbApp.inject({ method: "GET", url: "/verification/ping" }); + expect(second.statusCode).toBeGreaterThanOrEqual(500); + + // Breaker is now open (threshold 2) — the 3rd call must fast-fail with + // the breaker's own 503, not attempt the network call at all. + const third = await cbApp.inject({ method: "GET", url: "/verification/ping" }); + expect(third.statusCode).toBe(503); + expect(third.json()).toMatchObject({ error: "verification_service_unavailable" }); + expect(third.json().retryAfterMs).toBeGreaterThan(0); + + // Start the real upstream and wait past the 200ms cooldown — the next + // call should be allowed through as the half-open trial and succeed, + // closing the breaker. + await verificationUpstream.listen({ port, host: "127.0.0.1" }); + await new Promise((resolve) => setTimeout(resolve, 250)); + + const recovered = await cbApp.inject({ method: "GET", url: "/verification/ping" }); + expect(recovered.statusCode).toBe(200); + expect(recovered.json()).toEqual({ status: "verified" }); + + // Fully closed again — a normal subsequent call succeeds too. + const afterRecovery = await cbApp.inject({ method: "GET", url: "/verification/ping" }); + expect(afterRecovery.statusCode).toBe(200); + } finally { + await cbApp.close(); + await verificationUpstream.close(); + } + }); + + it("does NOT count a normal upstream error response (4xx/5xx) as a breaker failure", async () => { + const verificationUpstream = Fastify(); + verificationUpstream.get("/verification/always-404", async (_request, reply) => { + return reply.code(404).send({ error: "not_found" }); + }); + await verificationUpstream.listen({ port: 0, host: "127.0.0.1" }); + const { port } = verificationUpstream.server.address() as AddressInfo; + + const cbApp = buildServer({ + walletServiceUrl: "http://127.0.0.1:1", + verificationServiceUrl: `http://127.0.0.1:${port}`, + verificationCircuitFailureThreshold: 1, // trips on the very first FAILURE — proves 404s aren't counted + verificationCircuitCooldownMs: 60_000, + }); + await cbApp.ready(); + + try { + // Several real 404s from a genuinely reachable upstream — none of + // these should trip the breaker, since the connection itself + // succeeded every time. + for (let i = 0; i < 5; i++) { + const res = await cbApp.inject({ method: "GET", url: "/verification/always-404" }); + expect(res.statusCode).toBe(404); + } + // Still reachable — if the breaker had (incorrectly) opened, this + // would come back as our own 503 instead of the upstream's 404. + const stillReachable = await cbApp.inject({ method: "GET", url: "/verification/always-404" }); + expect(stillReachable.statusCode).toBe(404); + } finally { + await cbApp.close(); + await verificationUpstream.close(); + } + }); +}); + describe("api-gateway security controls", () => { let secApp: FastifyInstance; let upstream: FastifyInstance; diff --git a/services/api-gateway/src/server.ts b/services/api-gateway/src/server.ts index b356f31..e406e2a 100644 --- a/services/api-gateway/src/server.ts +++ b/services/api-gateway/src/server.ts @@ -2,7 +2,14 @@ import Fastify, { type FastifyInstance } from "fastify"; import cors from "@fastify/cors"; import helmet from "@fastify/helmet"; import rateLimit from "@fastify/rate-limit"; -import { registerHealth, registerMetrics } from "@vellar/service-kit"; +import { + circuitBreakerLimitsFromEnv, + CircuitOpenError, + createCircuitBreaker, + domainMetrics, + registerHealth, + registerMetrics, +} from "@vellar/service-kit"; import { registerProxyRoute } from "./register-proxy-route"; // Gateway (technical-doc.md §6.3, §8; idea.md §12): the single public entry @@ -30,6 +37,12 @@ export interface GatewayOptions { maxBodyBytes?: number; /** Per-request timeout in ms (connection-level). Default 30_000. */ requestTimeoutMs?: number; + /** Consecutive connection-level failures to verification-service before + * the breaker opens. Default 5; env VERIFICATION_CB_FAILURE_THRESHOLD. */ + verificationCircuitFailureThreshold?: number; + /** Cooldown (ms) before a half-open trial is allowed once open. Default + * 30_000; env VERIFICATION_CB_COOLDOWN_MS. */ + verificationCircuitCooldownMs?: number; /** Custom logger instance or options (e.g. stream for capturing logs in tests). */ logger?: unknown; } @@ -168,7 +181,72 @@ export function buildServer(options: GatewayOptions = {}): FastifyInstance { options.verificationServiceUrl ?? process.env.VERIFICATION_SERVICE_URL ?? "http://localhost:4004"; - registerProxyRoute(app, { upstream: verificationServiceUrl, prefix: "/verification" }); + + // Circuit breaker (#326): api-gateway has no protection against a + // verification-service outage cascading into slow gateway responses — + // every caller would otherwise wait out the same timeout against the same + // broken dependency. Opens after `failureThreshold` consecutive failed + // proxy attempts; while open, requests fail fast with 503 instead of + // reaching the (assumed-still-down) upstream at all. + const verificationBreakerLimits = circuitBreakerLimitsFromEnv( + { + failureThresholdVar: "VERIFICATION_CB_FAILURE_THRESHOLD", + cooldownMsVar: "VERIFICATION_CB_COOLDOWN_MS", + }, + { defaultFailureThreshold: 5, defaultCooldownMs: 30_000 }, + ); + const verificationBreaker = createCircuitBreaker({ + failureThreshold: + options.verificationCircuitFailureThreshold ?? verificationBreakerLimits.failureThreshold, + cooldownMs: options.verificationCircuitCooldownMs ?? verificationBreakerLimits.cooldownMs, + onStateChange: (from, to) => { + app.log.warn({ breaker: "verification-service", from, to }, "circuit breaker state change"); + domainMetrics.circuitBreakerStateChanges.inc({ + service: "api-gateway", + breaker: "verification-service", + from, + to, + }); + }, + }); + + registerProxyRoute(app, { + upstream: verificationServiceUrl, + prefix: "/verification", + // Runs before the proxy forwards the request — fast-fails while the + // breaker is open, so an outage doesn't tie up a connection waiting on + // a downstream that's already known to be unhealthy. + preHandler: async (request, reply) => { + try { + verificationBreaker.beforeCall(); + } catch (err) { + if (err instanceof CircuitOpenError) { + return reply.code(503).send({ + error: "verification_service_unavailable", + reason: "circuit breaker open — verification-service is failing", + retryAfterMs: err.retryAfterMs, + }); + } + throw err; + } + }, + replyOptions: { + // A response — even an upstream error status — means the TCP/HTTP + // round-trip to verification-service itself succeeded; only a + // genuine connection-level failure (ECONNREFUSED, timeout) counts + // against the breaker. An upstream 4xx/5xx is the verification + // service correctly reporting a domain-level outcome, not the + // service being unreachable. + onResponse: (_request, reply, res) => { + verificationBreaker.recordOutcome("success"); + reply.send(res.stream); + }, + onError: (reply, error) => { + verificationBreaker.recordOutcome("failure"); + reply.send(error.error); + }, + }, + }); return app; } diff --git a/services/policy-service/README.md b/services/policy-service/README.md index 5e2a252..9dede46 100644 --- a/services/policy-service/README.md +++ b/services/policy-service/README.md @@ -1,3 +1,22 @@ # @vellar/policy-service Policy schema validation, template registry, simulation and deployment orchestration + +## Deploy path timeout budgets (#327) + +Every RPC call in the policy deploy path (`getAccount`, `simulateTransaction`, +`prepareTransaction`, `sendTransaction`, and the post-submission +`getTransaction` polling loop) is bounded by two independently-configurable +timeout budgets, both enforced by this service's own timer (see +`deploy.ts`'s `withTimeoutError` — `@stellar/stellar-sdk`'s `rpc.Server` +constructor accepts a `timeout` option in its TypeScript types, but as of SDK +16.0.1 it has no effect on the underlying HTTP client and is not relied on): + +| Env var | Default | What it bounds | +|---|---|---| +| `DEPLOY_RPC_TIMEOUT_MS` | `10000` (10s) | Each individual RPC call. A network stall on any one call fails fast with `PolicyDeployError` code `deploy_rpc_timeout`, distinct from every other deploy failure code. | +| `DEPLOY_POLL_TIMEOUT_MS` | `60000` (60s) | The overall budget for the polling loop that waits for a submitted transaction to confirm. Exceeding this (without any single `getTransaction` call itself timing out) fails with code `deploy_timeout`. | + +Both are separate from the transaction's own on-chain timebounds (fixed at +60s, per the network's own rejection ceiling for timebounds set further out — +not configurable, since it isn't a network-call timeout at all). diff --git a/services/policy-service/src/config.ts b/services/policy-service/src/config.ts index 2c9d464..cca011b 100644 --- a/services/policy-service/src/config.ts +++ b/services/policy-service/src/config.ts @@ -5,10 +5,27 @@ export interface PolicyServiceRuntimeConfig { sponsorSecretKey: string | undefined; /** undefined = no Postgres; in-memory repository with a loud warning (dev only). */ databaseUrl: string | undefined; + /** Per-HTTP-request timeout (ms) for every individual RPC call the deploy + * path makes (getAccount, simulateTransaction, prepareTransaction, + * sendTransaction) — passed straight to `rpc.Server`'s own `timeout` + * option (#327). Distinct from `deployPollTimeoutMs` below, which bounds + * the *polling loop* waiting for the submitted tx to land. */ + deployRpcTimeoutMs: number; + /** Overall budget (ms) for the deployInstance polling loop that waits for + * transaction confirmation after submission (#327). */ + deployPollTimeoutMs: number; } const TESTNET_RPC = "https://soroban-testnet.stellar.org"; const TESTNET_PASSPHRASE = "Test SDF Network ; September 2015"; +const DEFAULT_DEPLOY_RPC_TIMEOUT_MS = 10_000; +const DEFAULT_DEPLOY_POLL_TIMEOUT_MS = 60_000; + +function positiveIntFromEnv(value: string | undefined, fallback: number): number { + if (!value) return fallback; + const parsed = Number(value); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +} export function configFromEnv(env: NodeJS.ProcessEnv = process.env): PolicyServiceRuntimeConfig { return { @@ -16,10 +33,20 @@ export function configFromEnv(env: NodeJS.ProcessEnv = process.env): PolicyServi networkPassphrase: env.STELLAR_NETWORK_PASSPHRASE || TESTNET_PASSPHRASE, sponsorSecretKey: env.SPONSOR_SECRET_KEY || undefined, databaseUrl: env.DATABASE_URL || undefined, + deployRpcTimeoutMs: positiveIntFromEnv( + env.DEPLOY_RPC_TIMEOUT_MS, + DEFAULT_DEPLOY_RPC_TIMEOUT_MS, + ), + deployPollTimeoutMs: positiveIntFromEnv( + env.DEPLOY_POLL_TIMEOUT_MS, + DEFAULT_DEPLOY_POLL_TIMEOUT_MS, + ), }; } export const DEFAULTS = { rpcUrl: TESTNET_RPC, networkPassphrase: TESTNET_PASSPHRASE, + deployRpcTimeoutMs: DEFAULT_DEPLOY_RPC_TIMEOUT_MS, + deployPollTimeoutMs: DEFAULT_DEPLOY_POLL_TIMEOUT_MS, } as const; diff --git a/services/policy-service/src/deploy.test.ts b/services/policy-service/src/deploy.test.ts new file mode 100644 index 0000000..aeb5f57 --- /dev/null +++ b/services/policy-service/src/deploy.test.ts @@ -0,0 +1,132 @@ +import { createServer, type Server } from "node:http"; +import { Keypair } from "@stellar/stellar-sdk"; +import { afterEach, describe, expect, it } from "vitest"; +import { createPolicyDeployer, PolicyDeployError } from "./deploy"; + +// A fake wasm hash / spending-limit constructor args — never actually reach +// the RPC in these tests, since every test here fails (by design) before or +// during the getAccount call. +const WASM_HASH_HEX = "00".repeat(32); +const SPONSOR = Keypair.random(); + +function baseConfig(rpcUrl: string, overrides: Partial[0]> = {}) { + return { + rpcUrl, + networkPassphrase: "Test SDF Network ; September 2015", + sponsorSecretKey: SPONSOR.secret(), + rpcTimeoutMs: 200, + pollTimeoutMs: 5_000, + // rpc.Server refuses a plain http:// URL otherwise, even for 127.0.0.1 — + // real deployments always use a real https:// RPC endpoint. + allowHttp: true, + ...overrides, + }; +} + +describe("createPolicyDeployer — RPC timeout budgets (#327)", () => { + let server: Server | undefined; + + afterEach(async () => { + if (server) { + // The "hanging" servers in these tests deliberately never respond, so + // whatever request the timed-out call raced against is still an open + // socket when the test finishes. Force-close it rather than waiting + // for a graceful drain that will never come — `server.close()` alone + // hangs here. + server.closeAllConnections(); + await new Promise((resolve) => server!.close(() => resolve())); + server = undefined; + } + }); + + /** Starts a local HTTP server that accepts the connection but never writes + * a response — the only reliable way to exercise a REAL per-request + * timeout end-to-end without depending on the exact JSON-RPC wire shape + * `rpc.Server` expects for a successful response. */ + function startHangingServer(): Promise { + return new Promise((resolve) => { + server = createServer((_req, _res) => { + // Deliberately never call res.end() / res.write() — the request hangs + // until the client's own timeout fires. + }); + server.listen(0, "127.0.0.1", () => { + const address = server!.address(); + if (address === null || typeof address === "string") { + throw new Error("expected a bound TCP address"); + } + resolve(`http://127.0.0.1:${address.port}`); + }); + }); + } + + it("simulateInstance surfaces a timeout error when getAccount stalls past rpcTimeoutMs", async () => { + const url = await startHangingServer(); + const deployer = createPolicyDeployer(baseConfig(url), WASM_HASH_HEX); + + const started = Date.now(); + const result = await deployer.simulateInstance({ + wallet: "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC", + constructorArgs: { dailyLimitStroops: "1000000000", windowSeconds: 86400 }, + }); + const elapsedMs = Date.now() - started; + + expect(result.ok).toBe(false); + expect(result.error).toContain("Policy deploy RPC call timed out: getAccount"); + // The call must fail close to the configured budget (200ms), not hang + // for the test runner's default timeout or longer. + expect(elapsedMs).toBeLessThan(2_000); + }); + + it("deployInstance throws PolicyDeployError with code deploy_rpc_timeout when getAccount stalls", async () => { + const url = await startHangingServer(); + const deployer = createPolicyDeployer(baseConfig(url), WASM_HASH_HEX); + + const promise = deployer.deployInstance({ + wallet: "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC", + constructorArgs: { dailyLimitStroops: "1000000000", windowSeconds: 86400 }, + }); + + await expect(promise).rejects.toBeInstanceOf(PolicyDeployError); + await expect(promise).rejects.toMatchObject({ code: "deploy_rpc_timeout" }); + }); + + it("a timeout error is distinct from a connection-refused error (different code)", async () => { + // No server listening on this port at all — a real ECONNREFUSED, not a + // timeout. Confirms withTimeoutError doesn't lump every RPC failure + // under the timeout code. + const deployer = createPolicyDeployer( + baseConfig("http://127.0.0.1:1"), // port 1 — reserved, nothing listens there + WASM_HASH_HEX, + ); + + const promise = deployer.deployInstance({ + wallet: "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC", + constructorArgs: { dailyLimitStroops: "1000000000", windowSeconds: 86400 }, + }); + + await expect(promise).rejects.toBeInstanceOf(PolicyDeployError); + await expect(promise).rejects.toMatchObject({ code: "sponsor_load_failed" }); + // Specifically NOT the timeout code, since this is a real refused + // connection, not a stalled one. + await expect(promise).rejects.not.toMatchObject({ code: "deploy_rpc_timeout" }); + }); + + it("respects a configured rpcTimeoutMs shorter than the default", async () => { + const url = await startHangingServer(); + const deployer = createPolicyDeployer(baseConfig(url, { rpcTimeoutMs: 50 }), WASM_HASH_HEX); + + const started = Date.now(); + await expect( + deployer.deployInstance({ + wallet: "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC", + constructorArgs: { dailyLimitStroops: "1000000000", windowSeconds: 86400 }, + }), + ).rejects.toMatchObject({ code: "deploy_rpc_timeout" }); + const elapsedMs = Date.now() - started; + + // Should fail close to the configured 50ms budget, not the default + // 200ms used by the other tests in this file — proves the value is + // actually threaded through to rpc.Server, not hardcoded. + expect(elapsedMs).toBeLessThan(1_000); + }); +}); diff --git a/services/policy-service/src/deploy.ts b/services/policy-service/src/deploy.ts index 93b1a54..4a49dd8 100644 --- a/services/policy-service/src/deploy.ts +++ b/services/policy-service/src/deploy.ts @@ -38,11 +38,76 @@ export class PolicyDeployError extends Error { } } +/** Sentinel error thrown internally by `withTimeoutError`'s own race timer. + * Never escapes this module — `withTimeoutError` always translates it into + * a `PolicyDeployError` with code `"deploy_rpc_timeout"` before rethrowing. + * + * This is a deliberate, self-implemented timeout rather than a reliance on + * `rpc.Server`'s constructor `timeout` option: as of `@stellar/stellar-sdk` + * 16.0.1, that option is silently dropped — `RpcServer`'s constructor calls + * `createHttpClient(opts.headers)`, which accepts only `headers` and never + * receives `opts.timeout` at all (confirmed by reading `rpc/axios.js`; the + * option is present in the TS types but has no effect on the underlying + * fetch client). Verified experimentally too: a `getAccount()` call against + * a deliberately non-responding local server hung well past the configured + * `timeout`, which is what surfaced this. Do not remove this wrapper under + * the assumption `rpc.Server`'s own option covers it — re-check this + * comment against the installed SDK version before doing so (#327). + */ +class RpcTimeoutSentinel extends Error {} + +/** Races `fn()` against a `timeoutMs` timer implemented in this module + * (see `RpcTimeoutSentinel`'s comment for why `rpc.Server`'s own `timeout` + * option isn't relied on) — a real per-call timeout budget for every RPC + * call in the deploy path, per #327. On timeout, throws a + * `PolicyDeployError` with code `"deploy_rpc_timeout"`. Every other failure + * from `fn` passes through to `onOtherError` unchanged, so this never masks + * a real RPC/contract failure as a timeout. */ +async function withTimeoutError( + step: string, + timeoutMs: number, + fn: () => Promise, + onOtherError: (err: unknown) => never, +): Promise { + let timer: ReturnType | undefined; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new RpcTimeoutSentinel(step)), timeoutMs); + }); + try { + return await Promise.race([fn(), timeout]); + } catch (err) { + if (err instanceof RpcTimeoutSentinel) { + throw new PolicyDeployError( + `Policy deploy RPC call timed out: ${step}`, + "deploy_rpc_timeout", + ); + } + onOtherError(err); + } finally { + clearTimeout(timer); + } +} + export interface PolicyDeployConfig { rpcUrl: string; networkPassphrase: string; /** Testnet fee-sponsor secret — deploys and funds the instance. */ sponsorSecretKey: string; + /** Per-HTTP-request timeout (ms) for every RPC call in the deploy path + * (getAccount, simulateTransaction, prepareTransaction, sendTransaction). + * Passed straight to `rpc.Server`'s own `timeout` option (#327). A network + * stall on any one of these calls fails with `PolicyDeployError` code + * `"deploy_rpc_timeout"` instead of hanging indefinitely. */ + rpcTimeoutMs: number; + /** Overall budget (ms) for `deployInstance`'s post-submission polling + * loop (#327) — distinct from `rpcTimeoutMs`, which bounds each + * individual HTTP call rather than the loop as a whole. */ + pollTimeoutMs: number; + /** Test-only escape hatch: `rpc.Server` refuses a non-`https://` URL + * unless this is set, even for `127.0.0.1` — real deployments always use + * a real `https://` RPC endpoint, so this should never be set outside + * tests exercising the RPC layer against a local fake server. */ + allowHttp?: boolean; } export interface DeployPolicyInstanceInput { @@ -98,22 +163,26 @@ export function createPolicyDeployer( config: PolicyDeployConfig, wasmHashHex: string, ): PolicyDeployer { - const server = new rpc.Server(config.rpcUrl); + // #327: config.rpcTimeoutMs is enforced by withTimeoutError below, not by + // rpc.Server itself — see RpcTimeoutSentinel's comment for why. + const server = new rpc.Server(config.rpcUrl, { allowHttp: config.allowHttp }); const sponsor = Keypair.fromSecret(config.sponsorSecretKey); const wasmHash = Buffer.from(wasmHashHex, "hex"); // Builds the (unsigned) deploy tx for the given input. Shared by simulate // and deploy so both exercise the exact same createContract + constructor. async function buildDeployTx(input: DeployPolicyInstanceInput): Promise { - let source; - try { - source = await server.getAccount(sponsor.publicKey()); - } catch (err) { - throw new PolicyDeployError( - `Sponsor account load failed: ${err instanceof Error ? err.message : String(err)}`, - "sponsor_load_failed", - ); - } + const source = await withTimeoutError( + "getAccount", + config.rpcTimeoutMs, + () => server.getAccount(sponsor.publicKey()), + (err) => { + throw new PolicyDeployError( + `Sponsor account load failed: ${err instanceof Error ? err.message : String(err)}`, + "sponsor_load_failed", + ); + }, + ); const constructorArgs = constructorScVals(input); @@ -141,7 +210,17 @@ export function createPolicyDeployer( } catch (err) { return { ok: false, error: err instanceof Error ? err.message : String(err) }; } - const sim = await server.simulateTransaction(built); + const sim = await withTimeoutError( + "simulateTransaction", + config.rpcTimeoutMs, + () => server.simulateTransaction(built), + (err) => { + throw new PolicyDeployError( + `Policy deploy simulation RPC failed: ${err instanceof Error ? err.message : String(err)}`, + "deploy_simulation_rpc_failed", + ); + }, + ); if (rpc.Api.isSimulationError(sim)) { return { ok: false, error: sim.error }; } @@ -151,19 +230,31 @@ export function createPolicyDeployer( async deployInstance(input) { const built = await buildDeployTx(input); - let prepared: Transaction; - try { - prepared = (await server.prepareTransaction(built)) as Transaction; - } catch (err) { - // Constructor guards (invalid limit/window) surface here, before submit. - throw new PolicyDeployError( - `Policy deploy simulation failed: ${err instanceof Error ? err.message : String(err)}`, - "deploy_simulation_failed", - ); - } + const prepared = (await withTimeoutError( + "prepareTransaction", + config.rpcTimeoutMs, + () => server.prepareTransaction(built), + (err) => { + // Constructor guards (invalid limit/window) surface here, before submit. + throw new PolicyDeployError( + `Policy deploy simulation failed: ${err instanceof Error ? err.message : String(err)}`, + "deploy_simulation_failed", + ); + }, + )) as Transaction; prepared.sign(sponsor); - const sent = await server.sendTransaction(prepared); + const sent = await withTimeoutError( + "sendTransaction", + config.rpcTimeoutMs, + () => server.sendTransaction(prepared), + (err) => { + throw new PolicyDeployError( + `Policy deploy submission RPC failed: ${err instanceof Error ? err.message : String(err)}`, + "deploy_submit_rpc_failed", + ); + }, + ); if (sent.status === "ERROR") { throw new PolicyDeployError( `Policy deploy submission failed: ${sent.errorResult?.toXDR("base64") ?? "unknown"}`, @@ -171,9 +262,19 @@ export function createPolicyDeployer( ); } - const deadline = Date.now() + TIMEOUT_SECONDS * 1000; + const deadline = Date.now() + config.pollTimeoutMs; for (;;) { - const status = await server.getTransaction(sent.hash); + const status = await withTimeoutError( + "getTransaction", + config.rpcTimeoutMs, + () => server.getTransaction(sent.hash), + (err) => { + throw new PolicyDeployError( + `Policy deploy status RPC failed: ${err instanceof Error ? err.message : String(err)}`, + "deploy_status_rpc_failed", + ); + }, + ); if (status.status === rpc.Api.GetTransactionStatus.SUCCESS) { const contractId = extractContractId(status); if (!contractId) { diff --git a/services/policy-service/src/index.ts b/services/policy-service/src/index.ts index 5c3b715..fddb4eb 100644 --- a/services/policy-service/src/index.ts +++ b/services/policy-service/src/index.ts @@ -42,6 +42,8 @@ deps.deployer = config.sponsorSecretKey rpcUrl: config.rpcUrl, networkPassphrase: config.networkPassphrase, sponsorSecretKey: config.sponsorSecretKey, + rpcTimeoutMs: config.deployRpcTimeoutMs, + pollTimeoutMs: config.deployPollTimeoutMs, }, SPENDING_POLICY_WASM_HASH, )