fix(api): fail open on audit log writes so login/register survive audit DB hiccups (closes #530)

api/src/audit/audit.interceptor.ts

@@ -61,21 +61,12 @@ export class AuditInterceptor implements NestInterceptor {
     const userId = req.auth?.userId ?? null
 
     return next.handle().pipe(
-      // Runs only on success. The interceptor has no access to the
-      // post-validation request body, so metadata here is limited to
-      // `req.params` (e.g. the deleted stream id). Actions that need
-      // richer metadata (e.g. AuthService.login) call auditService.log()
-      // directly at the service layer.
-      tap(() =>
-        this.auditService.log(
-          userId,
-          action,
-          action === AuditAction.STREAM_DELETE
-            ? { streamId: Number(req.params?.id) }
-            : {},
-          ip,
-        ),
-      ),
+      // metadata is empty here because the interceptor doesn't have access to
+      // the request body post-processing. Callers that need richer metadata
+      // (e.g. AuthService) call auditService.logSafely() directly.
+      // Fail-open policy (issue #530): logSafely never throws, so a failed
+      // audit INSERT cannot surface as a post-response error either.
+      tap(() => this.auditService.logSafely(userId, action, {}, ip)),
     )
   }
 }

api/src/audit/audit.module.ts

@@ -1,10 +1,26 @@
 import { Module } from "@nestjs/common"
 import { APP_INTERCEPTOR } from "@nestjs/core"
 
+import { AdminAuditController } from "./admin-audit.controller"
 import { AuditInterceptor } from "./audit.interceptor"
 import { AuditService } from "./audit.service"
+import { MetricsModule } from "../metrics/metrics.module"
 
+/**
+ * Audit logging module.
+ *
+ * Fail-open policy (issue #530): audit log writes must never fail the
+ * primary request they are auditing. A DB hiccup on the audit table
+ * must not turn a valid login into a 503. `AuditService.logSafely`
+ * absorbs write failures (structured log + `audit_log_write_failures_total`
+ * metric) so security-relevant events stay observable while the
+ * audited action proceeds — a deliberate trade-off between audit
+ * coverage and product availability. The interceptor and the auth
+ * service both route through the safe path.
+ */
 @Module({
+  imports: [MetricsModule],
+  controllers: [AdminAuditController],
   providers: [
     AuditService,
     { provide: APP_INTERCEPTOR, useClass: AuditInterceptor },

api/src/audit/audit.service.spec.ts

@@ -0,0 +1,142 @@
+import { Logger } from "@nestjs/common"
+
+import { AuditAction } from "./audit-action.enum"
+import { AuditService } from "./audit.service"
+
+interface MockPool {
+  query: jest.Mock
+}
+
+interface MockMetricsService {
+  auditLogWriteFailuresTotal: { inc: jest.Mock }
+}
+
+function makeService(
+  pool: MockPool,
+  metrics?: MockMetricsService,
+): AuditService {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any -- the pool only needs `query` for these tests
+  return new AuditService(pool as any, metrics as any)
+}
+
+describe("AuditService", () => {
+  let pool: MockPool
+  let metrics: MockMetricsService
+  let service: AuditService
+
+  beforeEach(() => {
+    pool = { query: jest.fn() }
+    metrics = {
+      auditLogWriteFailuresTotal: { inc: jest.fn() },
+    }
+    service = makeService(pool, metrics)
+    jest.clearAllMocks()
+  })
+
+  describe("log (fail-closed primitive)", () => {
+    it("inserts the audit row with stringified metadata", async () => {
+      pool.query.mockResolvedValue({ rows: [] })
+
+      await service.log(
+        7,
+        AuditAction.AUTH_LOGIN_FAILURE,
+        { reason: "invalid_password" },
+        "1.2.3.4",
+      )
+
+      expect(pool.query).toHaveBeenCalledWith(
+        "INSERT INTO audit_logs (user_id, action, metadata, ip) VALUES ($1, $2, $3, $4)",
+        [
+          7,
+          AuditAction.AUTH_LOGIN_FAILURE,
+          JSON.stringify({ reason: "invalid_password" }),
+          "1.2.3.4",
+        ],
+      )
+    })
+
+    it("throws on database errors — callers on primary paths must use logSafely", async () => {
+      pool.query.mockRejectedValue(new Error("connection refused"))
+
+      await expect(
+        service.log(null, AuditAction.AUTH_LOGIN_FAILURE, {}, "1.2.3.4"),
+      ).rejects.toThrow("connection refused")
+    })
+  })
+
+  describe("logSafely (fail-open policy)", () => {
+    it("resolves without throwing when the audit INSERT fails", async () => {
+      pool.query.mockRejectedValue(new Error("connection refused"))
+
+      await expect(
+        service.logSafely(
+          1,
+          AuditAction.AUTH_LOGIN_SUCCESS,
+          { email: "a@b.c" },
+          "203.0.113.7",
+        ),
+      ).resolves.toBeUndefined()
+    })
+
+    it("logs the failed write with the action, user, and IP that would have been recorded", async () => {
+      pool.query.mockRejectedValue(new Error("connection refused"))
+      const errorSpy = jest
+        .spyOn(Logger.prototype, "error")
+        .mockImplementation(() => undefined)
+
+      try {
+        await service.logSafely(
+          42,
+          AuditAction.AUTH_LOGIN_SUCCESS,
+          { email: "a@b.c" },
+          "203.0.113.7",
+        )
+
+        const [message] = errorSpy.mock.calls[0]
+        expect(String(message)).toContain("action=AUTH_LOGIN_SUCCESS")
+        expect(String(message)).toContain("userId=42")
+        expect(String(message)).toContain("ip=203.0.113.7")
+      } finally {
+        errorSpy.mockRestore()
+      }
+    })
+
+    it("increments the audit write failure counter with the action label", async () => {
+      pool.query.mockRejectedValue(new Error("timeout"))
+
+      await service.logSafely(
+        1,
+        AuditAction.AUTH_REGISTER_SUCCESS,
+        {},
+        "1.2.3.4",
+      )
+
+      expect(metrics.auditLogWriteFailuresTotal.inc).toHaveBeenCalledWith({
+        action: AuditAction.AUTH_REGISTER_SUCCESS,
+      })
+    })
+
+    it("performs a single write on success — no retry, no double-write, no counter bump", async () => {
+      pool.query.mockResolvedValue({ rows: [] })
+
+      await service.logSafely(
+        1,
+        AuditAction.AUTH_LOGIN_SUCCESS,
+        { email: "a@b.c" },
+        "1.2.3.4",
+      )
+
+      expect(pool.query).toHaveBeenCalledTimes(1)
+      expect(metrics.auditLogWriteFailuresTotal.inc).not.toHaveBeenCalled()
+    })
+
+    it("tolerates a missing metrics service (unit-test construction)", async () => {
+      const bare = makeService(pool)
+      pool.query.mockRejectedValue(new Error("down"))
+
+      await expect(
+        bare.logSafely(1, AuditAction.AUTH_LOGIN_SUCCESS, {}, "1.2.3.4"),
+      ).resolves.toBeUndefined()
+    })
+  })
+})

api/src/audit/audit.service.ts

@@ -1,15 +1,41 @@
-import { Inject, Injectable } from "@nestjs/common"
+import { Inject, Injectable, Logger, Optional } from "@nestjs/common"
 import { Pool } from "pg"
-import { PG_POOL } from "../database/database.module"
+
 import { AuditAction } from "./audit-action.enum"
+import { PG_POOL } from "../database/database.module"
+import { MetricsService } from "../metrics/metrics.service"
 
+/**
+ * Audit logging for security-relevant actions.
+ *
+ * Fail-open policy (issue #530): audit writes are an observability
+ * concern, not a trust boundary for the audited action. A failed
+ * INSERT must never fail the request being audited — a DB hiccup on
+ * the audit table must not turn a valid login into a 503. Primary
+ * request paths MUST go through {@link logSafely}, which absorbs the
+ * failure (structured log + `audit_log_write_failures_total` metric)
+ * and lets the audited action proceed. The raw {@link log} method
+ * stays fail-closed (it throws) so callers can still observe write
+ * failures directly; it is intentionally not awaited from request
+ * handlers.
+ */
 @Injectable()
 export class AuditService {
-  constructor(@Inject(PG_POOL) private readonly pool: Pool) {}
+  private readonly logger = new Logger(AuditService.name)
+
+  constructor(
+    @Inject(PG_POOL) private readonly pool: Pool,
+    @Optional() private readonly metricsService?: MetricsService,
+  ) {}
 
   /**
    * Persist an audit log entry.
    *
+   * Fail-closed primitive: throws on any database error. Do NOT await
+   * this from a primary request path — use {@link logSafely} instead
+   * so an audit write failure cannot fail the audited action
+   * (fail-open policy, issue #530).
+   *
    * @param userId  - the authenticated user, or `null` for anonymous events.
    * @param action  - a value from {@link AuditAction}; never a free-form string.
    * @param metadata - structured context for the action (e.g. `{ email, reason }`).
@@ -28,6 +54,37 @@ export class AuditService {
     )
   }
 
+  /**
+   * Fail-open wrapper around {@link log} for primary request paths.
+   *
+   * Deliberate trade-off (see module comment): if the audit INSERT
+   * fails (DB down, constraint, timeout), the failure is logged with
+   * the action, user, and IP that would have been recorded and counted
+   * in `audit_log_write_failures_total`, then swallowed — the audited
+   * action proceeds. Audit coverage degrades visibly during a DB
+   * outage instead of taking login/register down with it.
+   *
+   * Never throws. Safe to await anywhere. On success behaves exactly
+   * like {@link log} — a single write, no retry, no duplicate row.
+   */
+  async logSafely(
+    userId: number | null,
+    action: AuditAction,
+    metadata: Record<string, unknown>,
+    ip: string,
+  ): Promise<void> {
+    try {
+      await this.log(userId, action, metadata, ip)
+    } catch (err) {
+      const message = err instanceof Error ? err.message : String(err)
+      this.logger.error(
+        `audit log write failed (action=${action}, userId=${userId}, ip=${ip}): ${message}`,
+        err instanceof Error ? err.stack : undefined,
+      )
+      this.metricsService?.auditLogWriteFailuresTotal.inc({ action })
+    }
+  }
+
   async findAll(page = 1, limit = 20) {
     const offset = (page - 1) * limit
     const totalResult = await this.pool.query(

api/src/auth/auth-rate-limit.integration.spec.ts

@@ -13,12 +13,12 @@ import { Test, TestingModule } from "@nestjs/testing"
 import { ThrottlerGuard, ThrottlerModule } from "@nestjs/throttler"
 import request from "supertest"
 
-import { AuditService } from "../audit/audit.service"
 import { AuthController } from "./auth.controller"
 import { AuthService } from "./auth.service"
 import { PasswordResetService } from "./password-reset.service"
 import { TokenDenylistService } from "./token-denylist.service"
 import { UsersRepository } from "./users.repository"
+import { AuditService } from "../audit/audit.service"
 
 describe("Auth Rate Limiting (Integration)", () => {
   let app: INestApplication
@@ -51,6 +51,7 @@ describe("Auth Rate Limiting (Integration)", () => {
 
   const mockAuditService = {
     log: jest.fn().mockResolvedValue(undefined),
+    logSafely: jest.fn().mockResolvedValue(undefined),
   }
 
   beforeEach(async () => {