From b18ccbbfaba234a705c98faa7b062aa9e89937af Mon Sep 17 00:00:00 2001 From: MehrshadFb Date: Wed, 2 Sep 2026 12:00:17 -0400 Subject: [PATCH 1/3] feat(api): make photo storage quota reservation race-safe Check-then-insert was two statements outside a transaction, so two concurrent upload-urls calls for the same uploader (or one client spread across two API instances) could both read usage below the cap and both insert, overshooting PHOTO_STORAGE_LIMIT_BYTES. - Add PhotoStorageService.reserveUploadBytes: SUM(sizeBytes) over the caller's PENDING/READY photos and photo.createMany now run in one Prisma transaction at Serializable isolation, so Postgres commits one overlapping reservation per round and aborts the rest, which re-read usage on retry - Retry aborted reservations up to 5 times with jittered linear backoff, then fail with 409 STORAGE_RESERVATION_CONFLICT; recognise both shapes Prisma produces for SQLSTATE 40001 (P2034 when a statement in the callback fails, the raw driver adapter error when COMMIT fails) - Mint presigned S3 URLs only after the transaction commits so no DB transaction is held open across S3 calls - Keep assertCanUpload as a read-only pre-check; GET /users/me/storage is unchanged - Cover the transaction path, retries, and 409 in unit and e2e tests; document the design, the measured retry budget, and the advisory lock alternative in photos-architecture.md --- api/docs/photos-architecture.md | 43 +++- api/src/photos/photo-storage.service.spec.ts | 219 ++++++++++++++++++- api/src/photos/photo-storage.service.ts | 113 +++++++++- api/src/photos/photos.constants.ts | 15 ++ api/src/photos/photos.service.spec.ts | 65 ++++-- api/src/photos/photos.service.ts | 12 +- api/test/photos.e2e-spec.ts | 32 ++- 7 files changed, 451 insertions(+), 48 deletions(-) diff --git a/api/docs/photos-architecture.md b/api/docs/photos-architecture.md index 1c15ef4..7c4c6c1 100644 --- a/api/docs/photos-architecture.md +++ b/api/docs/photos-architecture.md @@ -18,8 +18,8 @@ An event has many photos. Photos live in S3; metadata lives in Postgres. The API For each file, API: - validates contentType allowlist + size cap - generates a `photoId` (uuid) and `s3Key = photos/{userId}/{eventId}/{photoId}` (one bucket, one prefix per uploader) - - inserts a `Photo` row with `status: PENDING` - - signs an S3 PUT URL (TTL ~1 hour, long enough to survive a backgrounded upload on flaky cellular) + - reserves the batch against the uploader's storage quota and inserts a `Photo` row with `status: PENDING` per file — both in one Serializable transaction (see §9) + - signs an S3 PUT URL (TTL ~1 hour, long enough to survive a backgrounded upload on flaky cellular), after the transaction has committed 3. **API responds** with `[{ photoId, uploadUrl }, ...]`. 4. **Mobile uploads bytes directly to S3.** Uses the OS background uploader (iOS `URLSession` background config, Android `WorkManager`). Each PUT goes straight to S3 — API is not involved. Survives app being backgrounded or killed. @@ -159,6 +159,7 @@ Order matters: if step 2 fails, row stays — operation is retry-safe. If step 3 | Event deleted with pending uploads | `onDelete: Cascade` removes rows. S3 objects orphaned (cleanup later). | | App killed mid-upload | OS background uploader resumes. Presigned URL TTL is 1h to give it room. | | Two devices upload simultaneously | Each has its own photoId. No conflict. | +| Two `upload-urls` calls for the same uploader race near the cap | Quota check + insert run in one Serializable transaction; Postgres aborts the loser, which retries and eventually gets 409 (see §9). | | Presigned URL leaked | TTL 1h, limited to one specific key + contentType. Worst case: attacker uploads junk to one key. | --- @@ -174,6 +175,7 @@ These are explicitly **not** being built now. Listed so we know what we're skipp - **S3 lifecycle rule** to auto-delete `pending/*` keys after 24h. One-time bucket config, no code. (Could ship as part of v1 if we add a `pending/` prefix.) - **Idempotency keys** on `/upload-urls` so retried requests don't mint duplicate rows. - **Per-event quota** checks (count and total bytes) before issuing upload slots. +- **Denormalized storage counter** (`storageUsedBytes` on the user row, bumped under `SELECT … FOR UPDATE`) once billing needs per-user limits or cheaper reads than `SUM(sizeBytes)`. Same transaction shape as today, more places to keep in sync (delete, cleanup). - **Rate limiting** on `/upload-urls` to prevent abuse. ### Reads / performance @@ -207,6 +209,7 @@ In order of implementation: - [x] **`GET /photos/:photoId`** — single photo with presigned GET URL. Non-READY photos 404, matching list invisibility. - [x] **`DELETE /photos/:photoId`** — S3 delete then row delete. - [x] **Per-user storage quota** — 5 GiB free tier enforced at upload-urls; `GET /users/me/storage` for usage. +- [x] **Race-safe quota reservation** — usage check + PENDING insert in one Serializable transaction, retried on serialization failure. - [x] **Unit tests** — service-level, mock `S3Service` and `PrismaService`. - [x] **E2E tests** — controller-level, with auth + CASL. - [x] **OpenAPI regen** — `npm run openapi:generate` so mobile picks up the new contract. (Regenerated alongside each endpoint; request DTOs need explicit `@ApiProperty` — the swagger CLI plugin does not run under the ts-node openapi script.) @@ -225,11 +228,41 @@ In order of implementation: ## 9. Per-user storage quota (free tier) -Each uploader has a storage cap (default **5 GiB**) enforced before upload slots are minted. +Each uploader has a storage cap (default **5 GiB**) enforced when upload slots are minted. - **Usage:** `SUM(sizeBytes)` over the caller's photos with `status IN (PENDING, READY)`. Pending rows count so clients cannot bypass the cap by minting slots without confirming. -- **Enforcement:** `PhotoStorageService.assertCanUpload()` in `PhotosService.createUploadSlots()`, after CASL authorization. -- **Read API:** `GET /users/me/storage` returns `usedBytes`, `limitBytes`, and `remainingBytes` as strings (bigint-safe JSON). +- **Enforcement:** `PhotoStorageService.reserveUploadBytes()` in `PhotosService.createUploadSlots()`, after CASL authorization. The usage query and the `createMany` of the batch's PENDING rows run in **one Prisma transaction at `Serializable` isolation**; presigned URLs are minted only after it commits. +- **Read API:** `GET /users/me/storage` returns `usedBytes`, `limitBytes`, and `remainingBytes` as strings (bigint-safe JSON). It runs the same usage query outside a transaction, so `remainingBytes` is the room left before the next reservation. - **Config:** `PHOTO_STORAGE_LIMIT_BYTES` overrides the default limit for all users until billing ships per-user limits. Over-quota uploads return **413 Payload Too Large** with message `Storage quota exceeded`. + +### Why the check and the insert share a transaction + +A plain check-then-insert is two statements, and nothing stops two requests from interleaving them: + +``` +limit 5 GiB, used 4.9 GiB, each request wants 200 MiB + +A: SUM → 4.9 GiB → ok → INSERT 200 MiB +B: SUM → 4.9 GiB → ok → INSERT 200 MiB (A's rows are not visible yet) +→ 5.3 GiB used, ~300 MiB over quota +``` + +The same interleaving happens across two API instances behind a load balancer. Under `Serializable` isolation Postgres tracks the read/write dependencies between the two transactions (each reads the uploader's usage, each inserts rows the other's read should have seen) and aborts one of them with a serialization failure (SQLSTATE `40001`). The survivor commits; the loser re-runs the whole transaction, re-reads usage, and is rejected with 413 if the survivor used up the room. + +- **Error shapes:** Prisma maps `40001` to `P2034` when a statement inside the callback fails, but a failure raised at `COMMIT` is rethrown as the driver adapter's own error (`{ cause: { kind: "TransactionWriteConflict", originalCode: "40001" } }`). Against Postgres 16 roughly a third of conflicts came back in the second shape, so `PhotoStorageService` recognises both (walking the `cause` chain) before deciding to retry. +- **Retries:** up to `STORAGE_RESERVATION_MAX_ATTEMPTS` (5) attempts with a jittered linear backoff (`STORAGE_RESERVATION_RETRY_DELAY_MS` × attempt, plus up to one delay of jitter). Every lost conflict logs `photo.storage.reservation_conflict` at `warn` with the attempt number. +- **Giving up:** after the last attempt the request fails with **409 Conflict** (`Storage reservation conflicted with a concurrent upload, please retry`). SSI lets roughly one same-user reservation commit per round, so a burst of N parallel in-quota batches needs about N attempts for the last one; measured against Postgres 16, five attempts cleared bursts of eight without a 409, while three started giving up at four. Beyond that the client can retry the same request. +- **Cost:** no blocking locks. SSI only adds predicate tracking, and the `addedById` index keeps the tracked range narrow. Serializable transactions can also abort spuriously (unrelated rows on a shared index page); the same retry absorbs that. +- **Scope:** only the usage query and the insert are inside the transaction. Event lookup and CASL run before it; S3 presigning runs after commit, so a slow S3 call never holds a database transaction open. +- **Alternative if bursts grow:** a per-uploader `pg_advisory_xact_lock` taken as the first statement of a `READ COMMITTED` transaction makes same-user reservations queue instead of abort — deterministic, no retries, still no schema change. It must not be combined with `Serializable`: that level takes its snapshot before the lock wait ends, so the waiter reads stale usage and aborts anyway. + +`PhotoStorageService.assertCanUpload()` remains as a read-only pre-check. It must never gate an insert on its own. + +### What it does not cover + +- A client under-reporting `sizeBytes` — caught at confirm, where `HeadObject` compares the real object size. +- Orphaned S3 objects and stuck PENDING rows — cleanup sweeper (§7). +- Multi-region deployments without a shared database — there is no cross-region serialization. +- Per-user paid limits — the limit is global config. When billing needs cheaper reads or per-user caps, the follow-up is a denormalized `storageUsedBytes` counter on the user row updated under `SELECT … FOR UPDATE` (§7), which replaces the `SUM` inside the same transaction shape. diff --git a/api/src/photos/photo-storage.service.spec.ts b/api/src/photos/photo-storage.service.spec.ts index d30b457..e0d5a67 100644 --- a/api/src/photos/photo-storage.service.spec.ts +++ b/api/src/photos/photo-storage.service.spec.ts @@ -1,20 +1,56 @@ -import { PayloadTooLargeException } from "@nestjs/common"; +import { randomUUID } from "node:crypto"; +import { ConflictException, PayloadTooLargeException } from "@nestjs/common"; import { ConfigService } from "@nestjs/config"; import { Test, TestingModule } from "@nestjs/testing"; -import { PhotoStatus, PrismaClient } from "generated/prisma/client"; +import { PhotoStatus, Prisma, PrismaClient } from "generated/prisma/client"; import { DeepMockProxy, mockDeep } from "jest-mock-extended"; +import { PinoLogger } from "nestjs-pino"; import { PrismaService } from "src/prisma/prisma.service"; -import { FREE_TIER_STORAGE_LIMIT_BYTES, PHOTO_SERVICE_ERRORS, STORAGE_QUOTA_EXCEEDED_CODE } from "./photos.constants"; -import { PhotoStorageService } from "./photo-storage.service"; +import { + buildPhotoS3Key, + FREE_TIER_STORAGE_LIMIT_BYTES, + PHOTO_SERVICE_ERRORS, + STORAGE_QUOTA_EXCEEDED_CODE, + STORAGE_RESERVATION_CONFLICT_CODE, + STORAGE_RESERVATION_MAX_ATTEMPTS, +} from "./photos.constants"; +import { PhotoStorageService, UploadReservationRow } from "./photo-storage.service"; describe("PhotoStorageService", () => { let service: PhotoStorageService; let prisma: DeepMockProxy; + let logger: { setContext: jest.Mock; info: jest.Mock; warn: jest.Mock; error: jest.Mock; debug: jest.Mock }; const userId = "11111111-1111-1111-1111-111111111111"; + const eventId = "66666666-6666-6666-6666-666666666666"; + + const usageWhere = { + addedById: userId, + status: { in: [PhotoStatus.PENDING, PhotoStatus.READY] }, + }; + + const buildRow = (sizeBytes: number): UploadReservationRow => { + const id = randomUUID(); + return { + id, + eventId, + addedById: userId, + s3Key: buildPhotoS3Key(userId, eventId, id), + contentType: "image/jpeg", + sizeBytes, + status: PhotoStatus.PENDING, + }; + }; + + const serializationFailure = () => + new Prisma.PrismaClientKnownRequestError("Transaction failed due to a write conflict or a deadlock.", { + code: "P2034", + clientVersion: "7.8.0", + }); beforeEach(async () => { prisma = mockDeep(); + logger = { setContext: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn() }; const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -26,6 +62,7 @@ describe("PhotoStorageService", () => { getOrThrow: jest.fn(() => FREE_TIER_STORAGE_LIMIT_BYTES), }, }, + { provide: PinoLogger, useValue: logger }, ], }).compile(); @@ -42,10 +79,7 @@ describe("PhotoStorageService", () => { }); expect(prisma.photo.aggregate).toHaveBeenCalledWith({ - where: { - addedById: userId, - status: { in: [PhotoStatus.PENDING, PhotoStatus.READY] }, - }, + where: usageWhere, _sum: { sizeBytes: true }, }); }); @@ -71,4 +105,173 @@ describe("PhotoStorageService", () => { }); await expect(service.assertCanUpload(userId, 200)).rejects.toBeInstanceOf(PayloadTooLargeException); }); + + describe("reserveUploadBytes", () => { + // The transaction client is a distinct mock so the tests can prove that + // both the usage query and the insert go through it, not the root client. + let tx: DeepMockProxy; + + beforeEach(() => { + tx = mockDeep(); + prisma.$transaction.mockImplementation(async (fn) => fn(tx)); + }); + + it("checks usage and inserts the rows inside one serializable transaction", async () => { + tx.photo.aggregate.mockResolvedValue({ _sum: { sizeBytes: 100 } } as never); + tx.photo.createMany.mockResolvedValue({ count: 2 }); + const rows = [buildRow(1024), buildRow(2048)]; + + await expect(service.reserveUploadBytes(userId, rows)).resolves.toBeUndefined(); + + expect(prisma.$transaction).toHaveBeenCalledTimes(1); + expect(prisma.$transaction).toHaveBeenCalledWith(expect.any(Function), { + isolationLevel: Prisma.TransactionIsolationLevel.Serializable, + }); + expect(tx.photo.aggregate).toHaveBeenCalledWith({ where: usageWhere, _sum: { sizeBytes: true } }); + expect(tx.photo.createMany).toHaveBeenCalledWith({ data: rows }); + expect(tx.photo.aggregate.mock.invocationCallOrder[0]).toBeLessThan( + tx.photo.createMany.mock.invocationCallOrder[0], + ); + expect(prisma.photo.aggregate).not.toHaveBeenCalled(); + expect(prisma.photo.createMany).not.toHaveBeenCalled(); + }); + + it("reserves exactly up to the limit", async () => { + tx.photo.aggregate.mockResolvedValue({ + _sum: { sizeBytes: Number(FREE_TIER_STORAGE_LIMIT_BYTES - 300n) }, + } as never); + tx.photo.createMany.mockResolvedValue({ count: 2 }); + + await expect(service.reserveUploadBytes(userId, [buildRow(100), buildRow(200)])).resolves.toBeUndefined(); + + expect(tx.photo.createMany).toHaveBeenCalledTimes(1); + }); + + it("rejects with 413 and inserts nothing when the batch would exceed the quota", async () => { + const usedBytes = FREE_TIER_STORAGE_LIMIT_BYTES - 100n; + tx.photo.aggregate.mockResolvedValue({ _sum: { sizeBytes: Number(usedBytes) } } as never); + const rows = [buildRow(50), buildRow(51)]; + + await expect(service.reserveUploadBytes(userId, rows)).rejects.toMatchObject({ + response: { + code: STORAGE_QUOTA_EXCEEDED_CODE, + message: PHOTO_SERVICE_ERRORS.STORAGE_QUOTA_EXCEEDED, + usedBytes: usedBytes.toString(), + limitBytes: FREE_TIER_STORAGE_LIMIT_BYTES.toString(), + requestedBytes: "101", + }, + }); + expect(tx.photo.createMany).not.toHaveBeenCalled(); + // Over quota is a verdict, not a conflict: no retry. + expect(prisma.$transaction).toHaveBeenCalledTimes(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it("treats a null usage sum (no photos yet) as zero", async () => { + tx.photo.aggregate.mockResolvedValue({ _sum: { sizeBytes: null } } as never); + tx.photo.createMany.mockResolvedValue({ count: 1 }); + + await expect(service.reserveUploadBytes(userId, [buildRow(1)])).resolves.toBeUndefined(); + + expect(tx.photo.createMany).toHaveBeenCalledTimes(1); + }); + + it("retries the whole transaction after a serialization failure and succeeds", async () => { + tx.photo.aggregate.mockResolvedValue({ _sum: { sizeBytes: 0 } } as never); + tx.photo.createMany.mockRejectedValueOnce(serializationFailure()).mockResolvedValue({ count: 1 }); + const rows = [buildRow(1024)]; + + await expect(service.reserveUploadBytes(userId, rows)).resolves.toBeUndefined(); + + expect(prisma.$transaction).toHaveBeenCalledTimes(2); + // The retry re-reads usage instead of reusing the stale sum. + expect(tx.photo.aggregate).toHaveBeenCalledTimes(2); + expect(tx.photo.createMany).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( + expect.objectContaining({ + event: "photo.storage.reservation_conflict", + userId, + attempt: 1, + maxAttempts: STORAGE_RESERVATION_MAX_ATTEMPTS, + willRetry: true, + }), + expect.any(String), + ); + }); + + it.each([ + [ + "the driver adapter error Prisma rethrows unmapped when COMMIT fails", + Object.assign(new Error("TransactionWriteConflict"), { + name: "DriverAdapterError", + cause: { + kind: "TransactionWriteConflict", + originalCode: "40001", + originalMessage: "could not serialize access due to read/write dependencies among transactions", + }, + }), + ], + [ + "a raw Postgres error carrying SQLSTATE 40001", + Object.assign(new Error("could not serialize access"), { code: "40001" }), + ], + [ + "a wrapped error whose cause chain ends in a serialization failure", + new Error("transaction failed", { cause: new Error("inner", { cause: { originalCode: "40001" } }) }), + ], + ])("also retries on %s", async (_shape, failure) => { + tx.photo.aggregate.mockResolvedValue({ _sum: { sizeBytes: 0 } } as never); + tx.photo.createMany.mockRejectedValueOnce(failure).mockResolvedValue({ count: 1 }); + + await expect(service.reserveUploadBytes(userId, [buildRow(1024)])).resolves.toBeUndefined(); + + expect(prisma.$transaction).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledWith( + expect.objectContaining({ event: "photo.storage.reservation_conflict", attempt: 1, willRetry: true }), + expect.any(String), + ); + }); + + it("gives up with 409 once the retry budget is exhausted", async () => { + prisma.$transaction.mockRejectedValue(serializationFailure()); + + const reservation = service.reserveUploadBytes(userId, [buildRow(1024)]); + + await expect(reservation).rejects.toBeInstanceOf(ConflictException); + await expect(reservation).rejects.toMatchObject({ + response: { + code: STORAGE_RESERVATION_CONFLICT_CODE, + message: PHOTO_SERVICE_ERRORS.STORAGE_RESERVATION_CONFLICT, + }, + }); + expect(prisma.$transaction).toHaveBeenCalledTimes(STORAGE_RESERVATION_MAX_ATTEMPTS); + expect(logger.warn).toHaveBeenCalledTimes(STORAGE_RESERVATION_MAX_ATTEMPTS); + expect(logger.warn).toHaveBeenLastCalledWith( + expect.objectContaining({ attempt: STORAGE_RESERVATION_MAX_ATTEMPTS, willRetry: false }), + expect.any(String), + ); + }); + + it("does not retry errors that are not serialization failures", async () => { + prisma.$transaction.mockRejectedValue(new Error("connection reset")); + + await expect(service.reserveUploadBytes(userId, [buildRow(1024)])).rejects.toThrow("connection reset"); + + expect(prisma.$transaction).toHaveBeenCalledTimes(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it("does not retry other known Prisma errors", async () => { + const uniqueViolation = new Prisma.PrismaClientKnownRequestError("Unique constraint failed", { + code: "P2002", + clientVersion: "7.8.0", + }); + prisma.$transaction.mockRejectedValue(uniqueViolation); + + await expect(service.reserveUploadBytes(userId, [buildRow(1024)])).rejects.toBe(uniqueViolation); + + expect(prisma.$transaction).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/api/src/photos/photo-storage.service.ts b/api/src/photos/photo-storage.service.ts index 49ebe3b..e5c3769 100644 --- a/api/src/photos/photo-storage.service.ts +++ b/api/src/photos/photo-storage.service.ts @@ -1,8 +1,15 @@ -import { Injectable, PayloadTooLargeException } from "@nestjs/common"; +import { ConflictException, Injectable, PayloadTooLargeException } from "@nestjs/common"; import { ConfigService } from "@nestjs/config"; -import { PhotoStatus } from "generated/prisma/client"; +import { PhotoStatus, Prisma } from "generated/prisma/client"; +import { PinoLogger } from "nestjs-pino"; import { PrismaService } from "src/prisma/prisma.service"; -import { PHOTO_SERVICE_ERRORS, STORAGE_QUOTA_EXCEEDED_CODE } from "./photos.constants"; +import { + PHOTO_SERVICE_ERRORS, + STORAGE_QUOTA_EXCEEDED_CODE, + STORAGE_RESERVATION_CONFLICT_CODE, + STORAGE_RESERVATION_MAX_ATTEMPTS, + STORAGE_RESERVATION_RETRY_DELAY_MS, +} from "./photos.constants"; export interface UserStorageSnapshot { usedBytes: string; @@ -10,19 +17,53 @@ export interface UserStorageSnapshot { remainingBytes: string; } +/** A PENDING photo row to insert once its bytes are reserved against the uploader's quota. */ +export type UploadReservationRow = Prisma.PhotoCreateManyInput; + +// Postgres reports a serialization failure as SQLSTATE 40001. Prisma maps it to +// P2034 when a statement inside the callback fails, but when the failure surfaces +// at COMMIT the driver adapter's own error ({ cause: { kind, originalCode } }) is +// rethrown unmapped, so both shapes have to be recognised. +const PRISMA_TRANSACTION_WRITE_CONFLICT = "P2034"; +const DRIVER_TRANSACTION_WRITE_CONFLICT = "TransactionWriteConflict"; +const POSTGRES_SERIALIZATION_FAILURE = "40001"; +const MAX_CAUSE_DEPTH = 5; + +type ErrorLike = { code?: unknown; kind?: unknown; originalCode?: unknown; cause?: unknown }; + +const isSerializationFailure = (error: unknown): boolean => { + let current: unknown = error; + for (let depth = 0; depth < MAX_CAUSE_DEPTH && typeof current === "object" && current !== null; depth++) { + const { code, kind, originalCode, cause } = current as ErrorLike; + if (code === PRISMA_TRANSACTION_WRITE_CONFLICT || code === POSTGRES_SERIALIZATION_FAILURE) return true; + if (kind === DRIVER_TRANSACTION_WRITE_CONFLICT || originalCode === POSTGRES_SERIALIZATION_FAILURE) return true; + current = cause; + } + return false; +}; + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +// Linear backoff with jitter: the losers of one round must not retry in lockstep. +const retryDelayMs = (attempt: number): number => + STORAGE_RESERVATION_RETRY_DELAY_MS * attempt + Math.random() * STORAGE_RESERVATION_RETRY_DELAY_MS; + @Injectable() export class PhotoStorageService { constructor( private readonly prisma: PrismaService, private readonly configService: ConfigService, - ) {} + private readonly logger: PinoLogger, + ) { + this.logger.setContext(this.constructor.name); + } private getLimitBytes(): bigint { return this.configService.getOrThrow("photos.storageLimitBytes"); } - async getUsedBytes(userId: string): Promise { - const result = await this.prisma.photo.aggregate({ + async getUsedBytes(userId: string, db: Prisma.TransactionClient = this.prisma): Promise { + const result = await db.photo.aggregate({ where: { addedById: userId, status: { in: [PhotoStatus.PENDING, PhotoStatus.READY] }, @@ -45,9 +86,65 @@ export class PhotoStorageService { }; } + /** + * Read-only quota check. It is not race-safe on its own: two concurrent + * callers can both pass it before either inserts a row, so never rely on it + * to gate an insert — use reserveUploadBytes() for that. + */ async assertCanUpload(userId: string, requestedBytes: number): Promise { - const requested = BigInt(requestedBytes); - const usedBytes = await this.getUsedBytes(userId); + await this.assertWithinQuota(this.prisma, userId, BigInt(requestedBytes)); + } + + /** + * Atomically checks the uploader's quota and inserts the given PENDING rows. + * + * The check and the insert run in one Serializable transaction, so + * overlapping reservations for the same uploader cannot all slip under the + * cap: Postgres commits one and aborts the others with a serialization + * failure. Losers retry (re-reading usage each time) and finally surface as + * 409. Presigned URLs should be minted only after this resolves, so no + * transaction is held open across S3 calls. + */ + async reserveUploadBytes(userId: string, rows: UploadReservationRow[]): Promise { + const requestedBytes = rows.reduce((sum, row) => sum + BigInt(row.sizeBytes), 0n); + + for (let attempt = 1; attempt <= STORAGE_RESERVATION_MAX_ATTEMPTS; attempt++) { + try { + await this.prisma.$transaction( + async (tx) => { + await this.assertWithinQuota(tx, userId, requestedBytes); + await tx.photo.createMany({ data: rows }); + }, + { isolationLevel: Prisma.TransactionIsolationLevel.Serializable }, + ); + return; + } catch (error) { + if (!isSerializationFailure(error)) throw error; + + const willRetry = attempt < STORAGE_RESERVATION_MAX_ATTEMPTS; + this.logger.warn( + { + event: "photo.storage.reservation_conflict", + userId, + attempt, + maxAttempts: STORAGE_RESERVATION_MAX_ATTEMPTS, + willRetry, + }, + "Storage reservation lost a serialization conflict", + ); + if (!willRetry) { + throw new ConflictException({ + code: STORAGE_RESERVATION_CONFLICT_CODE, + message: PHOTO_SERVICE_ERRORS.STORAGE_RESERVATION_CONFLICT, + }); + } + await sleep(retryDelayMs(attempt)); + } + } + } + + private async assertWithinQuota(db: Prisma.TransactionClient, userId: string, requested: bigint): Promise { + const usedBytes = await this.getUsedBytes(userId, db); const limitBytes = this.getLimitBytes(); if (usedBytes + requested <= limitBytes) return; diff --git a/api/src/photos/photos.constants.ts b/api/src/photos/photos.constants.ts index 75045aa..3d3adbe 100644 --- a/api/src/photos/photos.constants.ts +++ b/api/src/photos/photos.constants.ts @@ -30,6 +30,20 @@ export const FREE_TIER_STORAGE_LIMIT_BYTES = 5n * 1024n * 1024n * 1024n; // 5 Gi export const STORAGE_QUOTA_EXCEEDED_CODE = "STORAGE_QUOTA_EXCEEDED"; +export const STORAGE_RESERVATION_CONFLICT_CODE = "STORAGE_RESERVATION_CONFLICT"; + +// Quota reservation runs as a Serializable transaction. When reservations for +// the same uploader overlap, Postgres aborts all but one per round with a +// serialization failure (SQLSTATE 40001); the losers retry with a short +// jittered linear backoff before giving up with 409. Five attempts cleared +// bursts of eight parallel in-quota batches without a 409 when measured +// against Postgres 16; three attempts started giving up at four. +export const STORAGE_RESERVATION_MAX_ATTEMPTS = 5; + +// Attempt n waits DELAY * n plus up to DELAY of jitter so retries do not +// re-collide in lockstep. +export const STORAGE_RESERVATION_RETRY_DELAY_MS = 25; + // Per-photo outcome of a confirm call. Only READY mutates the row; the rest // report why verification failed so the client can retry or re-upload. export const CONFIRM_PHOTO_STATUSES = { @@ -49,4 +63,5 @@ export const PHOTO_SERVICE_ERRORS = { READ_FORBIDDEN: (photoId: string) => `Not authorized to read photo with ID "${photoId}"`, DELETE_FORBIDDEN: (photoId: string) => `Not authorized to delete photo with ID "${photoId}"`, STORAGE_QUOTA_EXCEEDED: "Storage quota exceeded", + STORAGE_RESERVATION_CONFLICT: "Storage reservation conflicted with a concurrent upload, please retry", }; diff --git a/api/src/photos/photos.service.spec.ts b/api/src/photos/photos.service.spec.ts index 055832d..e4bcd9c 100644 --- a/api/src/photos/photos.service.spec.ts +++ b/api/src/photos/photos.service.spec.ts @@ -1,4 +1,4 @@ -import { ForbiddenException, NotFoundException, PayloadTooLargeException } from "@nestjs/common"; +import { ConflictException, ForbiddenException, NotFoundException, PayloadTooLargeException } from "@nestjs/common"; import { Test, TestingModule } from "@nestjs/testing"; import { Event, EventAccess, Photo, PrismaClient } from "generated/prisma/client"; import { DeepMockProxy, mockDeep } from "jest-mock-extended"; @@ -21,7 +21,7 @@ describe("PhotosService", () => { headObject: jest.Mock; deleteObject: jest.Mock; }; - let photoStorageService: { assertCanUpload: jest.Mock }; + let photoStorageService: { reserveUploadBytes: jest.Mock }; const callerId = "11111111-1111-1111-1111-111111111111"; const eventId = "66666666-6666-6666-6666-666666666666"; @@ -98,7 +98,7 @@ describe("PhotosService", () => { headObject: jest.fn(), deleteObject: jest.fn().mockResolvedValue(undefined), }; - photoStorageService = { assertCanUpload: jest.fn().mockResolvedValue(undefined) }; + photoStorageService = { reserveUploadBytes: jest.fn().mockResolvedValue(undefined) }; const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -123,7 +123,7 @@ describe("PhotosService", () => { prisma.event.findUnique.mockResolvedValue(null); await expect(service.createUploadSlots(eventId, callerId, files)).rejects.toBeInstanceOf(NotFoundException); - expect(prisma.photo.createMany).not.toHaveBeenCalled(); + expect(photoStorageService.reserveUploadBytes).not.toHaveBeenCalled(); }); it("throws ForbiddenException when the caller is not a member of the event", async () => { @@ -131,7 +131,7 @@ describe("PhotosService", () => { prisma.event.findUnique.mockResolvedValue(eventWithAccess([]) as never); await expect(service.createUploadSlots(eventId, callerId, files)).rejects.toBeInstanceOf(ForbiddenException); - expect(prisma.photo.createMany).not.toHaveBeenCalled(); + expect(photoStorageService.reserveUploadBytes).not.toHaveBeenCalled(); }); it("throws ForbiddenException when the caller is a viewer", async () => { @@ -139,7 +139,7 @@ describe("PhotosService", () => { prisma.event.findUnique.mockResolvedValue(eventWithAccess([callerAccess("VIEWER")]) as never); await expect(service.createUploadSlots(eventId, callerId, files)).rejects.toBeInstanceOf(ForbiddenException); - expect(prisma.photo.createMany).not.toHaveBeenCalled(); + expect(photoStorageService.reserveUploadBytes).not.toHaveBeenCalled(); }); it("denies access when the caller has not completed onboarding", async () => { @@ -147,38 +147,61 @@ describe("PhotosService", () => { prisma.event.findUnique.mockResolvedValue(eventWithAccess([callerAccess("ORGANIZER")]) as never); await expect(service.createUploadSlots(eventId, callerId, files)).rejects.toBeInstanceOf(ForbiddenException); - expect(photoStorageService.assertCanUpload).not.toHaveBeenCalled(); + expect(photoStorageService.reserveUploadBytes).not.toHaveBeenCalled(); expect(prisma.photo.createMany).not.toHaveBeenCalled(); }); - it("throws PayloadTooLargeException when the upload would exceed storage quota", async () => { + it("throws PayloadTooLargeException and mints no URLs when the reservation exceeds storage quota", async () => { prisma.user.findUnique.mockResolvedValue(callerWithDetails); prisma.event.findUnique.mockResolvedValue(eventWithAccess([callerAccess("ORGANIZER")]) as never); - photoStorageService.assertCanUpload.mockRejectedValue( + photoStorageService.reserveUploadBytes.mockRejectedValue( new PayloadTooLargeException(PHOTO_SERVICE_ERRORS.STORAGE_QUOTA_EXCEEDED), ); await expect(service.createUploadSlots(eventId, callerId, files)).rejects.toBeInstanceOf( PayloadTooLargeException, ); - expect(photoStorageService.assertCanUpload).toHaveBeenCalledWith(callerId, 3072); + expect(photoStorageService.reserveUploadBytes).toHaveBeenCalledTimes(1); + expect(s3Service.getPresignedUploadUrl).not.toHaveBeenCalled(); + }); + + it("propagates ConflictException and mints no URLs when the reservation keeps conflicting", async () => { + prisma.user.findUnique.mockResolvedValue(callerWithDetails); + prisma.event.findUnique.mockResolvedValue(eventWithAccess([callerAccess("ORGANIZER")]) as never); + photoStorageService.reserveUploadBytes.mockRejectedValue( + new ConflictException(PHOTO_SERVICE_ERRORS.STORAGE_RESERVATION_CONFLICT), + ); + + await expect(service.createUploadSlots(eventId, callerId, files)).rejects.toBeInstanceOf(ConflictException); + expect(s3Service.getPresignedUploadUrl).not.toHaveBeenCalled(); + }); + + it("never inserts rows outside the quota reservation", async () => { + prisma.user.findUnique.mockResolvedValue(callerWithDetails); + prisma.event.findUnique.mockResolvedValue(eventWithAccess([callerAccess("ORGANIZER")]) as never); + + await service.createUploadSlots(eventId, callerId, files); + expect(prisma.photo.createMany).not.toHaveBeenCalled(); + expect(prisma.photo.create).not.toHaveBeenCalled(); }); it.each(["ORGANIZER", "PARTICIPANT"] as const)( - "creates PENDING rows and returns presigned slots for a %s", + "reserves quota for the PENDING rows, then returns presigned slots for a %s", async (accessLevel) => { prisma.user.findUnique.mockResolvedValue(callerWithDetails); prisma.event.findUnique.mockResolvedValue(eventWithAccess([callerAccess(accessLevel)]) as never); - prisma.photo.createMany.mockResolvedValue({ count: files.length }); const slots = await service.createUploadSlots(eventId, callerId, files); - expect(photoStorageService.assertCanUpload).toHaveBeenCalledWith(callerId, 3072); - expect(prisma.photo.createMany).toHaveBeenCalledTimes(1); - const { data } = prisma.photo.createMany.mock.calls[0][0] as { data: Record[] }; - expect(data).toHaveLength(files.length); - for (const [index, row] of data.entries()) { + expect(photoStorageService.reserveUploadBytes).toHaveBeenCalledTimes(1); + const [reservedFor, rows] = photoStorageService.reserveUploadBytes.mock.calls[0] as [ + string, + Record[], + ]; + expect(reservedFor).toBe(callerId); + expect(rows).toHaveLength(files.length); + for (const [index, row] of rows.entries()) { expect(row).toMatchObject({ eventId, addedById: callerId, @@ -191,13 +214,17 @@ describe("PhotosService", () => { expect(slots).toHaveLength(files.length); for (const [index, slot] of slots.entries()) { - expect(slot).toEqual({ photoId: data[index].id, uploadUrl: "https://signed-put" }); + expect(slot).toEqual({ photoId: rows[index].id, uploadUrl: "https://signed-put" }); } expect(s3Service.getPresignedUploadUrl).toHaveBeenCalledWith({ - key: data[0].s3Key, + key: rows[0].s3Key, contentType: files[0].contentType, expiresInSeconds: UPLOAD_URL_TTL_SECONDS, }); + // URLs are minted only after the reservation has committed. + expect(photoStorageService.reserveUploadBytes.mock.invocationCallOrder[0]).toBeLessThan( + s3Service.getPresignedUploadUrl.mock.invocationCallOrder[0], + ); }, ); }); diff --git a/api/src/photos/photos.service.ts b/api/src/photos/photos.service.ts index d75afcd..08a2366 100644 --- a/api/src/photos/photos.service.ts +++ b/api/src/photos/photos.service.ts @@ -71,11 +71,7 @@ export class PhotosService { throw new ForbiddenException(PHOTO_SERVICE_ERRORS.CREATE_FORBIDDEN(eventId)); } - const requestedBytes = files.reduce((sum, file) => sum + file.sizeBytes, 0); - await this.photoStorageService.assertCanUpload(callerId, requestedBytes); - - // Create a new photo row for each file. - // Each photo has a unique S3 Key and status of PENDING. + // Build a PENDING row per file up front: the S3 key embeds the photo id. const rows = files.map((file) => { const photoId = randomUUID(); return { @@ -88,8 +84,12 @@ export class PhotosService { status: PhotoStatus.PENDING, }; }); - await this.prisma.photo.createMany({ data: rows }); + // Quota check and insert run in one serializable transaction, so concurrent + // batches for the same uploader cannot both slip under the cap. + await this.photoStorageService.reserveUploadBytes(callerId, rows); + // Presign only once the reservation has committed: no transaction is held + // open across S3 calls, and a rejected batch mints no URLs. return Promise.all( rows.map(async (row) => ({ photoId: row.id, diff --git a/api/test/photos.e2e-spec.ts b/api/test/photos.e2e-spec.ts index bdfef85..f542a0b 100644 --- a/api/test/photos.e2e-spec.ts +++ b/api/test/photos.e2e-spec.ts @@ -1,9 +1,13 @@ import { INestApplication } from "@nestjs/common"; -import { PrismaClient } from "generated/prisma/client"; +import { Prisma, PrismaClient } from "generated/prisma/client"; import { Server } from "http"; import { DeepMockProxy, mockReset } from "jest-mock-extended"; import { EVENT_SERVICE_ERRORS } from "src/events/events.constants"; -import { PHOTO_SERVICE_ERRORS, FREE_TIER_STORAGE_LIMIT_BYTES } from "src/photos/photos.constants"; +import { + PHOTO_SERVICE_ERRORS, + FREE_TIER_STORAGE_LIMIT_BYTES, + STORAGE_RESERVATION_MAX_ATTEMPTS, +} from "src/photos/photos.constants"; import { S3Service } from "src/sdk/aws/s3/s3.service"; import { API_GLOBAL_PREFIX } from "src/swagger/swagger.config"; import request from "supertest"; @@ -95,6 +99,8 @@ describe("PhotosController (e2e)", () => { mockReset(prisma); prisma.user.findUnique.mockResolvedValue(buildUserWithDetails()); prisma.photo.aggregate.mockResolvedValue({ _sum: { sizeBytes: 0 } } as never); + // Interactive transactions run their callback against the same mock client. + prisma.$transaction.mockImplementation(async (fn) => fn(prisma)); for (const mock of Object.values(s3Service)) mock.mockReset(); s3Service.getPresignedUploadUrl.mockResolvedValue(TEST_SIGNED_PUT_URL); @@ -116,6 +122,10 @@ describe("PhotosController (e2e)", () => { expect(body.data).toHaveLength(1); expect(body.data[0].uploadUrl).toBe(TEST_SIGNED_PUT_URL); expect(body.data[0].photoId).toMatch(/^[0-9a-f-]{36}$/); + // Quota check + insert are reserved atomically under Serializable isolation. + expect(prisma.$transaction).toHaveBeenCalledWith(expect.any(Function), { + isolationLevel: Prisma.TransactionIsolationLevel.Serializable, + }); expect(prisma.photo.createMany).toHaveBeenCalledTimes(1); }); @@ -176,6 +186,24 @@ describe("PhotosController (e2e)", () => { const body = response.body as ErrorResponse; expect(body.message).toBe(PHOTO_SERVICE_ERRORS.STORAGE_QUOTA_EXCEEDED); expect(prisma.photo.createMany).not.toHaveBeenCalled(); + expect(s3Service.getPresignedUploadUrl).not.toHaveBeenCalled(); + }); + + it("returns 409 when the quota reservation keeps losing serialization conflicts", async () => { + prisma.event.findUnique.mockResolvedValue(eventWithAccess([buildOrganizerAccess()]) as never); + prisma.$transaction.mockRejectedValue( + new Prisma.PrismaClientKnownRequestError("Transaction failed due to a write conflict or a deadlock.", { + code: "P2034", + clientVersion: "7.8.0", + }), + ); + + const response = await request(httpServer).post(uploadUrlsPath()).set(authHeader()).send(payload).expect(409); + + const body = response.body as ErrorResponse; + expect(body.message).toBe(PHOTO_SERVICE_ERRORS.STORAGE_RESERVATION_CONFLICT); + expect(prisma.$transaction).toHaveBeenCalledTimes(STORAGE_RESERVATION_MAX_ATTEMPTS); + expect(s3Service.getPresignedUploadUrl).not.toHaveBeenCalled(); }); }); From 627d912bd795a83c823d04c363d9b921439cb4e0 Mon Sep 17 00:00:00 2001 From: MehrshadFb Date: Wed, 2 Sep 2026 20:55:35 -0400 Subject: [PATCH 2/3] refactor(api): move prisma error detection into the prisma layer --- api/src/photos/photo-storage.service.ts | 23 +------ api/src/prisma/prisma.errors.spec.ts | 80 +++++++++++++++++++++++++ api/src/prisma/prisma.errors.ts | 48 +++++++++++++++ 3 files changed, 129 insertions(+), 22 deletions(-) create mode 100644 api/src/prisma/prisma.errors.spec.ts create mode 100644 api/src/prisma/prisma.errors.ts diff --git a/api/src/photos/photo-storage.service.ts b/api/src/photos/photo-storage.service.ts index e5c3769..e927476 100644 --- a/api/src/photos/photo-storage.service.ts +++ b/api/src/photos/photo-storage.service.ts @@ -2,6 +2,7 @@ import { ConflictException, Injectable, PayloadTooLargeException } from "@nestjs import { ConfigService } from "@nestjs/config"; import { PhotoStatus, Prisma } from "generated/prisma/client"; import { PinoLogger } from "nestjs-pino"; +import { isSerializationFailure } from "src/prisma/prisma.errors"; import { PrismaService } from "src/prisma/prisma.service"; import { PHOTO_SERVICE_ERRORS, @@ -20,28 +21,6 @@ export interface UserStorageSnapshot { /** A PENDING photo row to insert once its bytes are reserved against the uploader's quota. */ export type UploadReservationRow = Prisma.PhotoCreateManyInput; -// Postgres reports a serialization failure as SQLSTATE 40001. Prisma maps it to -// P2034 when a statement inside the callback fails, but when the failure surfaces -// at COMMIT the driver adapter's own error ({ cause: { kind, originalCode } }) is -// rethrown unmapped, so both shapes have to be recognised. -const PRISMA_TRANSACTION_WRITE_CONFLICT = "P2034"; -const DRIVER_TRANSACTION_WRITE_CONFLICT = "TransactionWriteConflict"; -const POSTGRES_SERIALIZATION_FAILURE = "40001"; -const MAX_CAUSE_DEPTH = 5; - -type ErrorLike = { code?: unknown; kind?: unknown; originalCode?: unknown; cause?: unknown }; - -const isSerializationFailure = (error: unknown): boolean => { - let current: unknown = error; - for (let depth = 0; depth < MAX_CAUSE_DEPTH && typeof current === "object" && current !== null; depth++) { - const { code, kind, originalCode, cause } = current as ErrorLike; - if (code === PRISMA_TRANSACTION_WRITE_CONFLICT || code === POSTGRES_SERIALIZATION_FAILURE) return true; - if (kind === DRIVER_TRANSACTION_WRITE_CONFLICT || originalCode === POSTGRES_SERIALIZATION_FAILURE) return true; - current = cause; - } - return false; -}; - const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); // Linear backoff with jitter: the losers of one round must not retry in lockstep. diff --git a/api/src/prisma/prisma.errors.spec.ts b/api/src/prisma/prisma.errors.spec.ts new file mode 100644 index 0000000..71ad743 --- /dev/null +++ b/api/src/prisma/prisma.errors.spec.ts @@ -0,0 +1,80 @@ +import { Prisma } from "generated/prisma/client"; +import { isSerializationFailure } from "./prisma.errors"; + +describe("isSerializationFailure", () => { + const knownRequestError = (code: string) => + new Prisma.PrismaClientKnownRequestError("Transaction failed", { code, clientVersion: "7.8.0" }); + + // Wraps `leaf` in `depth` layers of cause, so the marker sits `depth` links down. + const nest = (depth: number, leaf: unknown): unknown => { + let current = leaf; + for (let i = 0; i < depth; i++) current = new Error(`wrapper ${i}`, { cause: current }); + return current; + }; + + describe("recognises every shape the same failure arrives in", () => { + it("matches the code Prisma maps a failed statement to", () => { + expect(isSerializationFailure(knownRequestError("P2034"))).toBe(true); + }); + + it("matches a raw Postgres SQLSTATE on the error itself", () => { + expect(isSerializationFailure(Object.assign(new Error("could not serialize access"), { code: "40001" }))).toBe( + true, + ); + }); + + it("matches the driver adapter error rethrown unmapped when COMMIT fails", () => { + const failure = Object.assign(new Error("TransactionWriteConflict"), { + name: "DriverAdapterError", + cause: { + kind: "TransactionWriteConflict", + originalCode: "40001", + originalMessage: "could not serialize access due to read/write dependencies among transactions", + }, + }); + + expect(isSerializationFailure(failure)).toBe(true); + }); + + it("matches a SQLSTATE carried as originalCode without the adapter's kind", () => { + expect(isSerializationFailure({ cause: { originalCode: "40001" } })).toBe(true); + }); + }); + + describe("leaves unrelated failures alone", () => { + it.each([ + ["a unique constraint violation", knownRequestError("P2002")], + ["a missing record", knownRequestError("P2025")], + ["a plain error", new Error("connection reset")], + ])("returns false for %s", (_label, error) => { + expect(isSerializationFailure(error)).toBe(false); + }); + + it.each([ + ["null", null], + ["undefined", undefined], + ["a string", "40001"], + ["a number", 40001], + ])("returns false for %s rather than throwing", (_label, value) => { + expect(isSerializationFailure(value)).toBe(false); + }); + }); + + describe("bounds how far it walks", () => { + it("finds a failure at the deepest link it inspects", () => { + expect(isSerializationFailure(nest(4, knownRequestError("P2034")))).toBe(true); + }); + + it("gives up on a failure buried past that depth", () => { + expect(isSerializationFailure(nest(5, knownRequestError("P2034")))).toBe(false); + }); + + it("terminates on a cause chain that loops back on itself", () => { + const outer: { cause?: unknown } = {}; + const inner = { cause: outer }; + outer.cause = inner; + + expect(isSerializationFailure(outer)).toBe(false); + }); + }); +}); diff --git a/api/src/prisma/prisma.errors.ts b/api/src/prisma/prisma.errors.ts new file mode 100644 index 0000000..1ccc6a4 --- /dev/null +++ b/api/src/prisma/prisma.errors.ts @@ -0,0 +1,48 @@ +/** + * Recognising a database failure means recognising Prisma's error *shapes*, not + * just its codes. The same Postgres failure reaches us two different ways: raised + * inside a transaction callback it arrives mapped, as a `PrismaClientKnownRequestError` + * with a `P` code, but raised at `COMMIT` it is rethrown by the driver adapter + * unmapped, carrying the raw SQLSTATE inside a nested `cause`. + * + * That dialect lives here, beside `PrismaService`, so no feature module has to + * learn it. Add new predicates to this file rather than to a service. + */ + +// Prisma's mapped code for a transaction that lost a write conflict or deadlock. +const PRISMA_TRANSACTION_WRITE_CONFLICT = "P2034"; +// The driver adapter's own name for that failure when it surfaces at COMMIT. +const DRIVER_TRANSACTION_WRITE_CONFLICT = "TransactionWriteConflict"; +// Postgres SQLSTATE 40001, the serialization failure behind both of the above. +const POSTGRES_SERIALIZATION_FAILURE = "40001"; + +// Wrapped errors nest a few levels deep. The bound also stops a cyclic cause +// chain from spinning forever. +const MAX_CAUSE_DEPTH = 5; + +type ErrorLike = { code?: unknown; kind?: unknown; originalCode?: unknown; cause?: unknown }; + +/** Applies a predicate to the thrown error and each error in its cause chain. */ +const someCause = (error: unknown, predicate: (candidate: ErrorLike) => boolean): boolean => { + let current: unknown = error; + for (let depth = 0; depth < MAX_CAUSE_DEPTH && typeof current === "object" && current !== null; depth++) { + if (predicate(current as ErrorLike)) return true; + current = (current as ErrorLike).cause; + } + return false; +}; + +/** + * True when a transaction was aborted because it could not be serialized against + * a concurrent one. Retrying the whole transaction is the correct response; the + * caller decides how many times and how long to wait. + */ +export const isSerializationFailure = (error: unknown): boolean => + someCause( + error, + ({ code, kind, originalCode }) => + code === PRISMA_TRANSACTION_WRITE_CONFLICT || + code === POSTGRES_SERIALIZATION_FAILURE || + kind === DRIVER_TRANSACTION_WRITE_CONFLICT || + originalCode === POSTGRES_SERIALIZATION_FAILURE, + ); From 6b041c739d94ac5b3120dd779006e4adfa25514c Mon Sep 17 00:00:00 2001 From: MehrshadFb Date: Wed, 2 Sep 2026 21:03:57 -0400 Subject: [PATCH 3/3] refactor(api): share sleep and jittered backoff helpers --- api/src/common/utils/async.utils.spec.ts | 71 ++++++++++++++++++++++++ api/src/common/utils/async.utils.ts | 15 +++++ api/src/photos/photo-storage.service.ts | 9 +-- 3 files changed, 88 insertions(+), 7 deletions(-) create mode 100644 api/src/common/utils/async.utils.spec.ts create mode 100644 api/src/common/utils/async.utils.ts diff --git a/api/src/common/utils/async.utils.spec.ts b/api/src/common/utils/async.utils.spec.ts new file mode 100644 index 0000000..5ed24d8 --- /dev/null +++ b/api/src/common/utils/async.utils.spec.ts @@ -0,0 +1,71 @@ +import { jitteredLinearBackoffMs, sleep } from "./async.utils"; + +describe("sleep", () => { + afterEach(() => { + jest.useRealTimers(); + }); + + it("resolves only once the delay has elapsed", async () => { + jest.useFakeTimers(); + let resolved = false; + const pending = sleep(1000).then(() => { + resolved = true; + }); + + jest.advanceTimersByTime(999); + await Promise.resolve(); + expect(resolved).toBe(false); + + jest.advanceTimersByTime(1); + await pending; + expect(resolved).toBe(true); + }); + + it("resolves on the next tick for a zero delay", async () => { + await expect(sleep(0)).resolves.toBeUndefined(); + }); +}); + +describe("jitteredLinearBackoffMs", () => { + const BASE = 25; + + const withRandom = (value: number, run: () => T): T => { + const spy = jest.spyOn(Math, "random").mockReturnValue(value); + try { + return run(); + } finally { + spy.mockRestore(); + } + }; + + it("grows linearly with the attempt number", () => { + // No jitter, so the growth is visible on its own. + const delays = withRandom(0, () => [1, 2, 3].map((attempt) => jitteredLinearBackoffMs(attempt, BASE))); + + expect(delays).toEqual([BASE, BASE * 2, BASE * 3]); + }); + + it("adds at most one further base delay of jitter", () => { + // Math.random() is exclusive of 1, so this is the supremum, never reached. + expect(withRandom(0.999999, () => jitteredLinearBackoffMs(1, BASE))).toBeLessThan(BASE * 2); + expect(withRandom(0.5, () => jitteredLinearBackoffMs(1, BASE))).toBe(BASE * 1.5); + }); + + it("keeps every real sample inside its attempt's window", () => { + for (let attempt = 1; attempt <= 5; attempt++) { + for (let sample = 0; sample < 200; sample++) { + const delay = jitteredLinearBackoffMs(attempt, BASE); + + expect(delay).toBeGreaterThanOrEqual(BASE * attempt); + expect(delay).toBeLessThan(BASE * (attempt + 1)); + } + } + }); + + it("spreads concurrent losers of the same round apart", () => { + // The whole point of the jitter: identical callers must not wake together. + const delays = new Set(Array.from({ length: 50 }, () => jitteredLinearBackoffMs(1, BASE))); + + expect(delays.size).toBeGreaterThan(1); + }); +}); diff --git a/api/src/common/utils/async.utils.ts b/api/src/common/utils/async.utils.ts new file mode 100644 index 0000000..edc7641 --- /dev/null +++ b/api/src/common/utils/async.utils.ts @@ -0,0 +1,15 @@ +/** Resolves after `ms` milliseconds. */ +export const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Delay before retry number `attempt` (1-based): `baseMs` per attempt so far, + * plus up to one more `baseMs` of jitter. + * + * The jitter is the load-bearing half. Callers that lose the same round of + * contention would otherwise all wake at the same instant and collide again, + * turning a backoff into a synchronised retry storm. Linear growth suits + * contention that clears a few participants per round; a caller waiting on an + * unrelated outage usually wants exponential growth instead. + */ +export const jitteredLinearBackoffMs = (attempt: number, baseMs: number): number => + baseMs * attempt + Math.random() * baseMs; diff --git a/api/src/photos/photo-storage.service.ts b/api/src/photos/photo-storage.service.ts index e927476..b0706d2 100644 --- a/api/src/photos/photo-storage.service.ts +++ b/api/src/photos/photo-storage.service.ts @@ -2,6 +2,7 @@ import { ConflictException, Injectable, PayloadTooLargeException } from "@nestjs import { ConfigService } from "@nestjs/config"; import { PhotoStatus, Prisma } from "generated/prisma/client"; import { PinoLogger } from "nestjs-pino"; +import { jitteredLinearBackoffMs, sleep } from "src/common/utils/async.utils"; import { isSerializationFailure } from "src/prisma/prisma.errors"; import { PrismaService } from "src/prisma/prisma.service"; import { @@ -21,12 +22,6 @@ export interface UserStorageSnapshot { /** A PENDING photo row to insert once its bytes are reserved against the uploader's quota. */ export type UploadReservationRow = Prisma.PhotoCreateManyInput; -const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); - -// Linear backoff with jitter: the losers of one round must not retry in lockstep. -const retryDelayMs = (attempt: number): number => - STORAGE_RESERVATION_RETRY_DELAY_MS * attempt + Math.random() * STORAGE_RESERVATION_RETRY_DELAY_MS; - @Injectable() export class PhotoStorageService { constructor( @@ -117,7 +112,7 @@ export class PhotoStorageService { message: PHOTO_SERVICE_ERRORS.STORAGE_RESERVATION_CONFLICT, }); } - await sleep(retryDelayMs(attempt)); + await sleep(jitteredLinearBackoffMs(attempt, STORAGE_RESERVATION_RETRY_DELAY_MS)); } } }