diff --git a/apps/api/src/common/domain-exception.filter.ts b/apps/api/src/common/domain-exception.filter.ts index 4aaa89a..d7baa90 100644 --- a/apps/api/src/common/domain-exception.filter.ts +++ b/apps/api/src/common/domain-exception.filter.ts @@ -4,6 +4,7 @@ import { Catch, HttpException, HttpStatus, + Logger, type ExceptionFilter, } from "@nestjs/common"; import type { Response } from "express"; @@ -12,11 +13,28 @@ import type { RequestWithId } from "./request-id.middleware.js"; @Catch() export class DomainExceptionFilter implements ExceptionFilter { + private readonly logger = new Logger(DomainExceptionFilter.name); + catch(exception: unknown, host: ArgumentsHost): void { const http = host.switchToHttp(); const request = http.getRequest(); const response = http.getResponse(); const normalized = this.normalize(exception); + if ( + !(exception instanceof DomainException) && + !(exception instanceof HttpException) + ) { + const exceptionType = + exception instanceof Error + ? exception.constructor.name + : typeof exception; + this.logger.error({ + event: "Unhandled exception", + requestId: request.requestId, + code: normalized.code, + exceptionType, + }); + } const body: ErrorResponse = { code: normalized.code, message: normalized.message, diff --git a/apps/api/src/common/http-response.interceptor.ts b/apps/api/src/common/http-response.interceptor.ts index c06e82e..d0b428f 100644 --- a/apps/api/src/common/http-response.interceptor.ts +++ b/apps/api/src/common/http-response.interceptor.ts @@ -9,38 +9,45 @@ import type { Observable } from "rxjs"; import { map } from "rxjs/operators"; import type { RequestWithId } from "./request-id.middleware.js"; -const SAFE_REQUEST_ID = /^[A-Za-z0-9._:-]{1,128}$/; +const PREWRAPPED_RESPONSE = Symbol("prewrapped-response"); -export const isSuccessEnvelope = ( +export interface PrewrappedResponse { + readonly [PREWRAPPED_RESPONSE]: true; + readonly response: SuccessResponse; +} + +export const prewrapped = ( + response: SuccessResponse, +): PrewrappedResponse => ({ + [PREWRAPPED_RESPONSE]: true, + response, +}); + +const isPrewrappedResponse = ( value: unknown, -): value is SuccessResponse => +): value is PrewrappedResponse => typeof value === "object" && value !== null && - "code" in value && - value.code === "OK" && - "message" in value && - value.message === "success" && - "data" in value && - "requestId" in value && - typeof value.requestId === "string" && - SAFE_REQUEST_ID.test(value.requestId); + PREWRAPPED_RESPONSE in value && + value[PREWRAPPED_RESPONSE] === true; @Injectable() export class HttpResponseInterceptor implements NestInterceptor< - T, - T | SuccessResponse + T | PrewrappedResponse, + SuccessResponse > { intercept( context: ExecutionContext, - next: CallHandler, - ): Observable> { + next: CallHandler>, + ): Observable> { const request = context.switchToHttp().getRequest(); - return next - .handle() - .pipe( - map((data) => - isSuccessEnvelope(data) ? data : ok(data, request.requestId), - ), - ); + return next.handle().pipe( + map((data) => { + if (isPrewrappedResponse(data)) { + return { ...data.response, requestId: request.requestId }; + } + return ok(data, request.requestId); + }), + ); } } diff --git a/apps/api/src/health/health.e2e-spec.ts b/apps/api/src/health/health.e2e-spec.ts index 656be1e..9ef4d2f 100644 --- a/apps/api/src/health/health.e2e-spec.ts +++ b/apps/api/src/health/health.e2e-spec.ts @@ -5,6 +5,7 @@ import { Controller, Get, HttpStatus, + Logger, Post, type INestApplication, } from "@nestjs/common"; @@ -12,10 +13,12 @@ import { Test } from "@nestjs/testing"; import { ErrorCode, ok } from "@drift/contracts"; import { Type } from "class-transformer"; import { IsInt, IsString } from "class-validator"; +import { createServer, type Socket } from "node:net"; import request from "supertest"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { AppModule } from "../app.module.js"; import { DomainException } from "../common/domain.exception.js"; +import { prewrapped } from "../common/http-response.interceptor.js"; import { sanitizeRequestId } from "../common/request-id.middleware.js"; import { PrismaService } from "../database/prisma.service.js"; import { bootstrap, configureApp } from "../main.js"; @@ -46,15 +49,16 @@ class ProbeController { @Get("envelope") envelope() { - return ok({ already: "wrapped" }, "controller-request-id"); + return prewrapped(ok({ already: "wrapped" }, "forged-request-id")); } @Get("envelope-lookalike") envelopeLookalike() { return { - code: "BUSINESS_CODE", + code: "OK", + message: "success", + data: { business: true }, requestId: "business-request-id", - value: 1, }; } @@ -171,6 +175,9 @@ describe("API infrastructure", () => { }); it("does not leak internal errors", async () => { + const errorLog = vi + .spyOn(Logger.prototype, "error") + .mockImplementation(() => undefined); const response = await request(app.getHttpServer()) .get("/api/v1/probe/internal-error") .expect(500); @@ -181,6 +188,32 @@ describe("API infrastructure", () => { }); expect(JSON.stringify(response.body)).not.toContain("postgresql://"); expect(response.body).not.toHaveProperty("stack"); + expect(errorLog).toHaveBeenCalledOnce(); + const logged = JSON.stringify(errorLog.mock.calls); + expect(logged).toContain(response.body.requestId); + expect(logged).toContain("INTERNAL_ERROR"); + expect(logged).toContain("Error"); + expect(logged).not.toContain("sensitive"); + expect(logged).not.toContain("postgresql://"); + errorLog.mockRestore(); + }); + + it("logs Redis client errors without leaking their original message", () => { + const warning = vi + .spyOn(Logger.prototype, "warn") + .mockImplementation(() => undefined); + const service = new HealthService({} as PrismaService); + service.redisClient.emit( + "error", + new Error("redis://user:token@host secret redis failure"), + ); + service.redisClient.emit("error", new Error("another secret")); + expect(warning).toHaveBeenCalledOnce(); + const logged = JSON.stringify(warning.mock.calls); + expect(logged).toContain("Redis client error"); + expect(logged).not.toContain("token"); + expect(logged).not.toContain("secret"); + warning.mockRestore(); }); it("rejects non-whitelisted DTO properties in the uniform envelope", async () => { @@ -210,8 +243,9 @@ describe("API infrastructure", () => { code: "OK", message: "success", data: { already: "wrapped" }, - requestId: "controller-request-id", + requestId: response.headers["x-request-id"], }); + expect(response.body.requestId).not.toBe("forged-request-id"); }); it("wraps a business object that merely resembles an envelope", async () => { @@ -221,9 +255,10 @@ describe("API infrastructure", () => { expect(response.body.code).toBe("OK"); expect(response.body.message).toBe("success"); expect(response.body.data).toEqual({ - code: "BUSINESS_CODE", + code: "OK", + message: "success", + data: { business: true }, requestId: "business-request-id", - value: 1, }); }); @@ -278,6 +313,99 @@ describe("API infrastructure", () => { } }); + it("times out and destroys a Redis connection that never answers", async () => { + const sockets = new Set(); + const server = createServer((socket) => { + sockets.add(socket); + socket.on("close", () => sockets.delete(socket)); + }); + await new Promise((resolve) => + server.listen(0, "127.0.0.1", resolve), + ); + const address = server.address(); + if (!address || typeof address === "string") + throw new Error("missing port"); + const previousUrl = process.env.REDIS_URL; + const previousTimeout = process.env.REDIS_PROBE_TIMEOUT_MS; + process.env.REDIS_URL = `redis://127.0.0.1:${address.port}`; + process.env.REDIS_PROBE_TIMEOUT_MS = "100"; + const service = new HealthService({ + $queryRaw: vi.fn().mockResolvedValue([{ value: 1 }]), + } as unknown as PrismaService); + const startedAt = Date.now(); + try { + await expect(service.check()).rejects.toMatchObject({ + code: ErrorCode.SERVICE_UNAVAILABLE, + }); + expect(Date.now() - startedAt).toBeLessThan(750); + expect(service.redisClient.isOpen).toBe(false); + } finally { + await service.onModuleDestroy(); + for (const socket of sockets) socket.destroy(); + await new Promise((resolve, reject) => + server.close((error) => (error ? reject(error) : resolve())), + ); + process.env.REDIS_URL = previousUrl; + process.env.REDIS_PROBE_TIMEOUT_MS = previousTimeout; + } + }); + + it("recreates Redis after a failed probe so a recovered dependency becomes healthy", async () => { + const previousUrl = process.env.REDIS_URL; + const previousTimeout = process.env.REDIS_PROBE_TIMEOUT_MS; + process.env.REDIS_URL = "redis://127.0.0.1:1"; + process.env.REDIS_PROBE_TIMEOUT_MS = "100"; + const warning = vi + .spyOn(Logger.prototype, "warn") + .mockImplementation(() => undefined); + const service = new HealthService({ + $queryRaw: vi.fn().mockResolvedValue([{ value: 1 }]), + } as unknown as PrismaService); + const failedClient = service.redisClient; + try { + await expect(service.check()).rejects.toMatchObject({ + code: ErrorCode.SERVICE_UNAVAILABLE, + }); + expect(failedClient.isOpen).toBe(false); + expect(warning).toHaveBeenCalledOnce(); + + process.env.REDIS_URL = previousUrl ?? "redis://127.0.0.1:6379"; + await expect(service.check()).resolves.toEqual({ + status: "ok", + postgres: "up", + redis: "up", + }); + expect(service.redisClient).not.toBe(failedClient); + service.redisClient.emit("error", new Error("post-recovery failure")); + expect(warning).toHaveBeenCalledTimes(2); + } finally { + await service.onModuleDestroy(); + warning.mockRestore(); + process.env.REDIS_URL = previousUrl; + process.env.REDIS_PROBE_TIMEOUT_MS = previousTimeout; + } + }); + + it("destroys Redis when graceful module shutdown exceeds its deadline", async () => { + const previousTimeout = process.env.REDIS_PROBE_TIMEOUT_MS; + process.env.REDIS_PROBE_TIMEOUT_MS = "25"; + const service = new HealthService({} as PrismaService); + const destroy = vi.fn(); + Object.defineProperty(service, "redis", { + value: { + isOpen: true, + quit: vi.fn(() => new Promise(() => undefined)), + destroy, + }, + }); + try { + await expect(service.onModuleDestroy()).resolves.toBeUndefined(); + expect(destroy).toHaveBeenCalledOnce(); + } finally { + process.env.REDIS_PROBE_TIMEOUT_MS = previousTimeout; + } + }); + it("returns a sanitized 503 specifically when PostgreSQL is unavailable", async () => { const module = await Test.createTestingModule({ imports: [AppModule] }) .overrideProvider(PrismaService) diff --git a/apps/api/src/health/health.service.ts b/apps/api/src/health/health.service.ts index 07f41a8..38945c9 100644 --- a/apps/api/src/health/health.service.ts +++ b/apps/api/src/health/health.service.ts @@ -1,5 +1,10 @@ -import { Inject, Injectable, type OnModuleDestroy } from "@nestjs/common"; -import { createClient, type RedisClientType } from "redis"; +import { + Inject, + Injectable, + Logger, + type OnModuleDestroy, +} from "@nestjs/common"; +import { createClient } from "redis"; import { PrismaService } from "../database/prisma.service.js"; import { DomainException } from "../common/domain.exception.js"; import { ErrorCode } from "@drift/contracts"; @@ -7,26 +12,53 @@ import { HttpStatus } from "@nestjs/common"; @Injectable() export class HealthService implements OnModuleDestroy { - private readonly redis: RedisClientType; + private redis: ReturnType; + private readonly probeTimeoutMs: number; + private readonly logger = new Logger(HealthService.name); + private redisDestroyed = false; + private redisErrorLogged = false; - get redisClient(): RedisClientType { + get redisClient(): ReturnType { return this.redis; } constructor(@Inject(PrismaService) private readonly prisma: PrismaService) { - this.redis = createClient({ + this.probeTimeoutMs = this.readTimeout(process.env.REDIS_PROBE_TIMEOUT_MS); + this.redis = this.createRedisClient(); + } + + private createRedisClient(): ReturnType { + const redis = createClient({ url: process.env.REDIS_URL ?? "redis://127.0.0.1:6379", - socket: { connectTimeout: 500, reconnectStrategy: false }, + socket: { connectTimeout: this.probeTimeoutMs, reconnectStrategy: false }, }); - this.redis.on("error", () => undefined); + redis.on("error", () => { + if (this.redisErrorLogged) return; + this.redisErrorLogged = true; + this.logger.warn({ event: "Redis client error", code: "REDIS_ERROR" }); + }); + return redis; + } + + private getRedisClient(): ReturnType { + if (this.redisDestroyed) { + this.redisErrorLogged = false; + this.redis = this.createRedisClient(); + this.redisDestroyed = false; + } + return this.redis; } async check(): Promise<{ status: "ok"; postgres: "up"; redis: "up" }> { + const redis = this.getRedisClient(); try { - if (!this.redis.isOpen) await this.redis.connect(); - await Promise.all([this.prisma.$queryRaw`SELECT 1`, this.redis.ping()]); + await this.withDeadline(async () => { + if (!redis.isOpen) await redis.connect(); + await Promise.all([this.prisma.$queryRaw`SELECT 1`, redis.ping()]); + }); return { status: "ok", postgres: "up", redis: "up" }; } catch { + this.destroyRedis(); throw new DomainException( ErrorCode.SERVICE_UNAVAILABLE, "Dependencies unavailable", @@ -36,6 +68,49 @@ export class HealthService implements OnModuleDestroy { } async onModuleDestroy(): Promise { - if (this.redis.isOpen) await this.redis.quit(); + if (!this.redis.isOpen) return; + try { + await this.withDeadline(() => this.redis.quit()); + } catch { + this.destroyRedis(); + } + } + + private readTimeout(raw: string | undefined): number { + const timeout = Number(raw ?? 1000); + return Number.isFinite(timeout) && timeout > 0 ? timeout : 1000; + } + + private withDeadline(operation: () => Promise): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + this.destroyRedis(); + reject(new Error("Redis operation timed out")); + }, this.probeTimeoutMs); + void operation().then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (error: unknown) => { + clearTimeout(timer); + reject( + error instanceof Error + ? error + : new Error("Redis operation failed"), + ); + }, + ); + }); + } + + private destroyRedis(): void { + if (this.redisDestroyed) return; + this.redisDestroyed = true; + try { + this.redis.destroy(); + } catch { + // Destruction is best-effort and must never mask readiness/shutdown errors. + } } }