diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b9421fef..4870f746 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -182,11 +182,24 @@ DoctorSpecialty (join table) id, doctor_profile_id, specialty_id FacilityProfile (1:1 User) - id, user_id, name, type (enum: lab|pharmacy), capabilities: FacilityCapability[] + id, user_id, name, type (enum: lab|pharmacy), version (int, default 1 — issue #49, §5.4) + capabilities: FacilityCapability[] FacilityCapability id, facility_profile_id, capability_type (enum: lab_test|medication_dispense), detail (e.g. test category) + version (int, default 1 — issue #49, §5.4) ``` +**`version` columns (Phase 8, issue #49):** added by +`backend/prisma/migrations/20261019090000_add_optimistic_concurrency/` as +`INTEGER NOT NULL DEFAULT 1` on exactly the mutable records above plus +`Appointment` (§4.2) and `PaymentIntent`. They are the storage half of the +`ETag`/`If-Match` convention in §5.4 and carry no meaning on their own — +nothing may treat a higher `version` as "more correct", and no code may +compare-then-write instead of guarding the UPDATE itself. Tables that are +append-only (`Prescription`, `LabOrder`) or immutable +(`Specialty`, `WalletLink`, `AuditEvent`, the export tables) deliberately do +**not** have one; the integration test asserts that set so a later change +can't quietly widen it. ### 4.2 Availability & Appointments ``` @@ -194,12 +207,16 @@ AvailabilitySlot id, doctor_profile_id, start_time, end_time, status (enum: open|held|booked) # Uniqueness/exclusion constraint at DB level on (doctor_profile_id, start_time, end_time) # to make double-booking a constraint violation, not an app-logic bug. + # No `version`: a slot is only ever moved by the conditional claim in §5.1, + not edited. Appointment id, patient_profile_id, doctor_profile_id, availability_slot_id (FK, unique, NULLABLE) intake_id (FK -> IntakeForm — still not a column, Phase 2 issue #1/#2) status (enum: scheduled|completed|cancelled|no_show) origin (enum: direct_booking|referral), referral_id (nullable FK -> Referral) + version (int, default 1 — issue #49, §5.4; no If-Match-guarded route exists + for appointments yet, but every future status write must move it) created_at, updated_at # availability_slot_id is nullable (Phase 3): a cancelled appointment # releases its slot back to `open` and drops the unique link, so the @@ -598,6 +615,48 @@ T5, enforced as a curated config policy. 4. Facility updates its own `FulfillmentRecord` status; for labs, attaches `LabResult`. 5. Order's own `status` field is updated to `fulfilled` only via this facility-confirmed path — patient/doctor cannot self-mark it. +### 5.4 Mutable-record optimistic concurrency (Phase 8, issue #49) +Slot booking (§5.1) is protected by a conditional update, but nothing else was: a +doctor editing their profile in one tab and an admin in another, or two tabs +updating the same appointment, silently overwrote each other — last write wins, +and the loser never finds out. This is the convention that closes that whole +class of bug, and **new mutable-resource routes must follow it.** + +1. An individual mutable-resource read returns a strong `ETag` carrying the row's + integer `version`, e.g. `ETag: "4"` (also mirrored as `version` in the JSON + body). Collection reads deliberately do not get one: a client fetches the + individual resource before editing. +2. `PATCH`/`PUT` sends that exact value back in `If-Match`. Missing header → + `428 Precondition Required` (`code: 'IF_MATCH_REQUIRED'`); weak validator, a + validator list, or anything else that isn't one strong `""` → `400` + (`code: 'INVALID_IF_MATCH'`) rather than a guess at which member was meant; + a version that no longer matches → `412 Precondition Failed` + (`code: 'VERSION_CONFLICT'`). +3. The repository performs **one** conditional update — `UPDATE ... WHERE id = ? + AND version = ?` with `version = version + 1` in the same statement — and + returns the row that statement wrote. A zero-row result is a stale write. + Read-compare-write is forbidden: it reopens the exact race it closes, because + two callers can both read version 4 and both pass a check made before the + write. In Prisma that is `updateManyAndReturn` / `updateMany` with + `count === 1`, never `findUnique` → compare → `update`. +4. Everything else keeps working the way it did. Endpoints whose state + transition is *already* conditional (booking's slot claim, payment + fund/release/refund, doctor verification, token redemption) keep that status + guard and are **not** additionally `If-Match`-gated — two guards on one write + produce two different failure modes and the second one teaches clients + nothing. They do still increment `version`, and still publish the new `ETag`, + so a client can precondition whatever it does next. Every other write to a + versioned row increments it too, including system writes (email verification). +5. Applies to `User`, `PatientProfile`, `DoctorProfile`, `FacilityProfile`, + `FacilityCapability`, `Appointment` and `PaymentIntent` (columns documented in + §4.1/§4.2). Append-only `Prescription`/`LabOrder` are excluded by design — + a correction is a new row (`supersedes_*`), so there is nothing to lose. +6. Shared pieces live in `backend/src/middleware/optimistic-concurrency.ts` + (`requireIfMatch`, `expectedVersion`, `setVersionEtag`, `versionConflict`) so + every module answers with the same status, message and code. Proof: + `backend/tests/integration/optimistic-concurrency.test.mts`, including eight + simultaneous writes on one version where exactly one succeeds. + ## 6. Hard questions (architecture-level — see PRD §8 for product-level) ### 6.1 What do we think would break? @@ -606,6 +665,7 @@ T5, enforced as a curated config policy. 3. Polymorphic `FulfillmentRecord.order_id` (§4.7) — polymorphic FKs are a known footgun for referential integrity in Postgres. Needs either two separate tables (`PrescriptionFulfillment`, `LabOrderFulfillment`) or a check-constraint + application-level guard. **Current schema sketch is a simplification that should be revisited before implementation**, not treated as final. **Resolved 2026-10-22 (Phase 5, issue #13 — `docs/adr/0012-fulfillment-record-typed-order-fks.md`):** neither option exactly as posed — `FulfillmentRecord` stays **one table**, but the `(order_type, order_id)` pair is gone. It is replaced by two real foreign keys (`prescription_id`, `lab_order_id`, each `UNIQUE`) with a PostgreSQL `CHECK` requiring exactly one, plus the `facility_id` FK the sketch left as a TODO. Referential integrity is now the database's job, and the CHECK only chooses between two already-validated columns — which is what an `order_type`+`order_id` guard could never do, since Postgres cannot make a bare `TEXT` conditionally reference one of two tables. Keeping one table (rather than splitting) is what keeps §5.3 step 5 a single transition implementation and the facility's assigned-orders queue a single indexed query; the discriminator mechanism is the one `PaymentIntent` already uses (issue #85). Deferred by that decision rather than invented around: re-routing an order to a different facility after a `rejected` fulfillment would need a partial unique index, so in v1 a `rejected` row is terminal for that order — a corrected order is a new `supersedes_*_id` row and gets its own fulfillment. 4. JWT refresh rotation without a revocation list — a stolen refresh token has no clean kill switch until this is added. 5. **Discovered 2026-10-15 while building email verification (Phase 8, issue #34):** the email/SMS provider is still undecided (Phase 6, issue #17), so verification links can't actually be delivered in a hosted environment yet — `backend/src/modules/auth/services/email-dispatcher.ts` is a no-op seam until a provider is integrated. Until then `backend/scripts/mark-email-verified.mjs` is the documented out-of-band path for an operator with DB access. Not a reason to skip the guard: the guard is what makes the flow safe once delivery exists, and shipping it locked-off would just mean shipping it untested. +6. **Resolved 2026-10-19 (issue #49):** silent last-write-wins on every mutable record that wasn't the slot claim — a doctor in one tab and an admin in another, or two tabs editing one appointment, each wrote and the second erased the first with no signal to either. §5.4 now makes the compare and the write one conditional statement behind an `If-Match` precondition. **Still open:** the convention is only *enforced* where a route opts in, so a future mutable route that forgets `requireIfMatch` silently opts out — there is no lint or schema-level way to catch that yet, and ESLint itself is still untracked (issue #28). 6. **Discovered 2026-10-17 (issue #41):** after `DELETE /patients/me`, the caller's access token (stateless JWT, 15-minute TTL) still passes `authenticate` until it expires — there's no revocation list (item 4 above, issue #18). It can't reach anything: every patient route resolves the caller through `getMyPatientProfile`, which refuses an anonymised profile; booking/payments refuse it via `requireVerifiedEmail` (verification is cleared); refresh refuses `deleted_at`. Closing the window itself is #18's job. Also: Postgres reports `ON DELETE RESTRICT` as SQLSTATE 23001, which Prisma surfaces as an *unknown* request error, not `P2003` — `patients/services` `isForeignKeyRefusal` handles both; `middleware/error-handler.ts` currently maps neither to a 4xx. diff --git a/ARCHITECTURE_ESSENTIALS.md b/ARCHITECTURE_ESSENTIALS.md index 17444042..a35b4231 100644 --- a/ARCHITECTURE_ESSENTIALS.md +++ b/ARCHITECTURE_ESSENTIALS.md @@ -19,6 +19,7 @@ Quick-reference only. Full reasoning lives in `ARCHITECTURE.md`. Full product sc 4. **Cross-module access only through a module's service interface.** Never import another module's `repositories/*` directly. 5. **Facilities write to their own `FulfillmentRecord` only** — never directly to `Prescription.status` / `LabOrder.status`. 6. Every module = `routes/ controllers/ services/ repositories/`. Keep that shape. +7. **Mutable-record edits use optimistic concurrency (Phase 8, issue #49).** Individual-resource read returns a strong version `ETag` (`"4"`); PATCH/PUT must send it in `If-Match` — missing is `428`, weak/list/garbage is `400`, stale is `412`. Write it as ONE conditional `UPDATE ... WHERE id = ? AND version = ?` that increments `version` (Prisma: `updateManyAndReturn`); read-compare-write is forbidden. Status transitions that are *already* conditional (booking, payment status, verification) keep their own guard, don't get a second `If-Match` one, but still bump + republish the version. Append-only `Prescription`/`LabOrder` are excluded. See ARCHITECTURE.md §5.4. ## Data model map (see ARCHITECTURE.md §4 for full field lists) `User` (including `locale`, default `en`, Phase 9 #133) → `PatientProfile | DoctorProfile | FacilityProfile`; `User` → `AdminInvite` (Phase 8, issue #39 — hashed/expiring/single-use; `User.deactivatedAt` soft-deactivates an account, refusing login/refresh/access tokens without deleting anything) @@ -37,6 +38,8 @@ Quick-reference only. Full reasoning lives in `ARCHITECTURE.md`. Full product sc Account deletion (Phase 8, issue #41 — docs/adr/0010): anonymise in place (`User.deletedAt`, `PatientProfile.anonymizedAt`), never cascade. `PatientProfile.userId` is ON DELETE RESTRICT; new patient-linked clinical tables must be RESTRICT too **and** register a retention hold in `src/retention-holds.ts`. `FacilityProfile` → `WalletLink` (payout address) ; `PatientProfile` → `PatientWalletLink` (payer address, ADR 0005) ; `Prescription` or `LabOrder` → exactly one typed `PaymentIntent` FK (issue #85), whose `amount` is in the smallest unit of a per-environment allowlisted `assetCode` (issue #79) +Mutable records above (plus `Appointment` and `PaymentIntent`) carry a `version` int for the `ETag`/`If-Match` convention — hard rule 7, ARCHITECTURE.md §5.4. Mutable profile fields a new route exposes need a conditional write, not just a `.update()`. + ## Payments (Phase 5.5 — see docs/adr/0004-custodial-payments-v1.md) - **Custodial v1**: `docforum-core` holds its own Stellar payer + releaser identities (`PLATFORM_PAYER_SECRET`/`PLATFORM_RELEASER_SECRET` env vars). Patients never hold or sign with a wallet — the only reason this is possible without violating `docforum-web`'s "no direct Stellar calls" hard rule. - `WalletLink` is scoped to the **facility** (the on-chain payee), not the patient. diff --git a/backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql b/backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql new file mode 100644 index 00000000..13f0dba6 --- /dev/null +++ b/backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql @@ -0,0 +1,31 @@ +-- Optimistic concurrency for mutable records (Phase 8, issue #49). +-- +-- One integer per row, defaulting to 1 so existing rows are valid at their +-- current state without a backfill. It is the storage half of the ETag/If-Match +-- convention in ARCHITECTURE.md §5.4 and is meaningless on its own: nothing may +-- compare-then-write, the compare has to live in the UPDATE's WHERE clause. +-- +-- Deliberately NOT added to Prescription/LabOrder (append-only — a correction is +-- a new row, so there is no update to lose) or to immutable tables +-- (AvailabilitySlot, Specialty, WalletLink, AuditEvent, the export tables). + +-- AlterTable +ALTER TABLE "User" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "PatientProfile" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "DoctorProfile" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "FacilityProfile" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "FacilityCapability" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "Appointment" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; + +-- AlterTable +ALTER TABLE "PaymentIntent" ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1; \ No newline at end of file diff --git a/backend/prisma/schema.prisma b/backend/prisma/schema.prisma index 89501909..e77ff0db 100644 --- a/backend/prisma/schema.prisma +++ b/backend/prisma/schema.prisma @@ -439,6 +439,7 @@ model Guardianship { model DoctorProfile { id String @id @default(uuid()) + version Int @default(1) userId String @unique fullName String verificationStatus VerificationStatus @default(pending) @@ -489,6 +490,7 @@ model DoctorSpecialty { model FacilityProfile { id String @id @default(uuid()) + version Int @default(1) userId String @unique name String type FacilityType @@ -578,6 +580,7 @@ model AvailabilitySlot { model Appointment { id String @id @default(uuid()) + version Int @default(1) patientProfileId String doctorProfileId String // Nullable because a CANCELLED appointment releases its slot back to diff --git a/backend/src/middleware/optimistic-concurrency.ts b/backend/src/middleware/optimistic-concurrency.ts new file mode 100644 index 00000000..d4929a04 --- /dev/null +++ b/backend/src/middleware/optimistic-concurrency.ts @@ -0,0 +1,84 @@ +// Optimistic concurrency for mutable records (Phase 8, issue #49). +// See ARCHITECTURE.md §5.4 for the convention every new module must follow. +// +// The whole point is that the compare and the write are ONE statement +// (`UPDATE ... WHERE id = ? AND version = ?` + `version = version + 1`). +// A read-compare-write would reopen the exact race this closes: two callers +// can both read version 4, both pass the check, and the second write +// silently destroys the first. +import type { NextFunction, Request, Response } from 'express'; +import { HttpError } from '../http-error'; + +declare global { + // eslint-disable-next-line @typescript-eslint/no-namespace + namespace Express { + interface Request { + expectedVersion?: number; + } + } +} + +/** + * A weak validator (`W/"4"`) or a list (`"4", "7"`) is refused rather than + * parsed: this API only ever issues one strong validator per resource, and + * guessing which member of a list the caller meant is how silent overwrites + * come back. + */ +const VERSION_ETAG = /^"([1-9]\d*)"$/; + +/** Strong `ETag` carrying the row's integer version, e.g. `ETag: "4"`. + * Call it on the individual-resource read; collection reads deliberately + * don't get one (§5.4.5). */ +export function setVersionEtag(res: Response, version: number) { + res.set('ETag', `"${version}"`); +} + +/** Rejects a mutable-resource write that doesn't carry the version it read. + * Must run after `authenticate`/`requireRole` so an unauthorised caller gets + * 401/403 rather than being told the resource exists. */ +export function requireIfMatch(req: Request, _res: Response, next: NextFunction) { + const value = req.get('If-Match'); + if (!value) { + throw new HttpError( + 428, + 'If-Match is required. Read the resource first and retry with its ETag in the If-Match header.', + 'IF_MATCH_REQUIRED', + ); + } + + const match = VERSION_ETAG.exec(value.trim()); + const version = match ? Number(match[1]) : Number.NaN; + if (!Number.isSafeInteger(version)) { + throw new HttpError( + 400, + 'If-Match must be the strong version ETag returned by this API, e.g. If-Match: "4".', + 'INVALID_IF_MATCH', + ); + } + + req.expectedVersion = version; + next(); +} + +/** The version the caller claimed. Read by a controller after + * `requireIfMatch` has run; throws rather than defaulting to 0, because a + * missing precondition silently skipping the guard would reintroduce the + * lost-update bug this exists to prevent. */ +export function expectedVersion(req: Request): number { + const version = req.expectedVersion; + if (typeof version !== 'number') { + throw new Error('requireIfMatch must run before expectedVersion().'); + } + return version; +} + +/** The error a repository's zero-row conditional update means: someone else + * moved the record since the caller read it. Shared so every module returns + * the same status, message and code. */ +export function versionConflict() { + return new HttpError( + 412, + 'This record changed since you read it. Fetch it again and reapply your change.', + 'VERSION_CONFLICT', + ); +} \ No newline at end of file diff --git a/backend/src/modules/auth/repositories/index.ts b/backend/src/modules/auth/repositories/index.ts index 494a0aed..59981aa5 100644 --- a/backend/src/modules/auth/repositories/index.ts +++ b/backend/src/modules/auth/repositories/index.ts @@ -143,7 +143,7 @@ export async function consumeVerificationToken(tokenId: string, consumedAt: Date } export function markUserEmailVerified(userId: string, verifiedAt: Date) { - return prisma.user.update({ where: { id: userId }, data: { emailVerifiedAt: verifiedAt } }); + return prisma.user.update({ where: { id: userId }, data: { emailVerifiedAt: verifiedAt, version: { increment: 1 } } }); } // --- Login throttling (Phase 8, issue #35) ------------------------------- diff --git a/backend/src/modules/doctors/controllers/index.ts b/backend/src/modules/doctors/controllers/index.ts index b56e0c7c..e945e507 100644 --- a/backend/src/modules/doctors/controllers/index.ts +++ b/backend/src/modules/doctors/controllers/index.ts @@ -4,10 +4,20 @@ import { publicDoctorListQuerySchema, setDoctorSpecialtyVerificationSchema, setM import { HttpError } from '../../../http-error'; import { getParam } from '../../../request-params'; import { auditActor } from '../../../middleware/request-id'; +import { expectedVersion, setVersionEtag } from '../../../middleware/optimistic-concurrency'; export async function getMe(req: Request, res: Response) { if (!req.user) throw new HttpError(401, 'Authentication required.'); const profile = await doctorsService.getMyDoctorProfile(req.user.id); + setVersionEtag(res, profile.version); + res.status(200).json(profile); +} + +export async function updateMe(req: Request, res: Response) { + if (!req.user) throw new HttpError(401, 'Authentication required.'); + const { fullName } = updateDoctorProfileSchema.parse(req.body); + const profile = await doctorsService.updateMyDoctorProfile(req.user.id, expectedVersion(req), fullName); + setVersionEtag(res, profile.version); res.status(200).json(profile); } diff --git a/backend/src/modules/doctors/repositories/index.ts b/backend/src/modules/doctors/repositories/index.ts index b2cf14c6..7114ec59 100644 --- a/backend/src/modules/doctors/repositories/index.ts +++ b/backend/src/modules/doctors/repositories/index.ts @@ -128,6 +128,25 @@ export function findDoctorProfileById(id: string, db: DbClient = prisma) { return db.doctorProfile.findUnique({ where: { id } }); } +/** + * The optimistic-locking write (ARCHITECTURE.md §5.4, issue #49). + * + * One statement: the row is only updated if it still carries the version the + * caller read, and that same statement bumps it. `updateManyAndReturn` gives + * back the row *this* statement wrote, so the response the caller sees is the + * state it produced — a re-read would be a second window in which someone + * else's write could land first. Null means the guard didn't match: the + * caller is stale and must re-read. + */ +export function updateDoctorProfile(id: string, version: number, fullName: string) { + return prisma.doctorProfile + .updateManyAndReturn({ + where: { id, version }, + data: { fullName, version: { increment: 1 } }, + }) + .then((rows) => rows[0] ?? null); +} + export function findDoctorSummariesByIds(ids: string[]) { return prisma.doctorProfile.findMany({ where: { id: { in: ids } }, select: { id: true, fullName: true } }); } @@ -146,7 +165,7 @@ export async function lockVerificationStatus( } export function updateVerificationStatus(db: DbClient, id: string, status: VerificationStatus) { - return db.doctorProfile.update({ where: { id }, data: { verificationStatus: status } }); + return db.doctorProfile.update({ where: { id }, data: { verificationStatus: status, version: { increment: 1 } } }); } /** diff --git a/backend/src/modules/doctors/routes/index.ts b/backend/src/modules/doctors/routes/index.ts index b8db553c..d69a30c3 100644 --- a/backend/src/modules/doctors/routes/index.ts +++ b/backend/src/modules/doctors/routes/index.ts @@ -1,5 +1,6 @@ import { Router } from 'express'; import { authenticate, requireRole } from '../../../middleware/auth'; +import { requireIfMatch } from '../../../middleware/optimistic-concurrency'; import * as doctorsController from '../controllers'; export const doctorsRouter = Router(); diff --git a/backend/src/modules/doctors/services/index.ts b/backend/src/modules/doctors/services/index.ts index fe3765bf..d9ade8fa 100644 --- a/backend/src/modules/doctors/services/index.ts +++ b/backend/src/modules/doctors/services/index.ts @@ -1,5 +1,6 @@ import { HttpError } from '../../../http-error'; import { prisma } from '../../../db/prisma-client'; +import { versionConflict } from '../../../middleware/optimistic-concurrency'; import * as auditService from '../../audit/services'; import * as doctorsRepo from '../repositories'; import type { VerificationStatus } from '@prisma/client'; diff --git a/backend/src/modules/payments/controllers/index.ts b/backend/src/modules/payments/controllers/index.ts index 73652a66..a49f0f60 100644 --- a/backend/src/modules/payments/controllers/index.ts +++ b/backend/src/modules/payments/controllers/index.ts @@ -4,6 +4,7 @@ import { walletLinkSchema, paymentHistoryQuerySchema } from '../validation'; import { HttpError } from '../../../http-error'; import { getParam } from '../../../request-params'; import { auditActor } from '../../../middleware/request-id'; +import { setVersionEtag } from '../../../middleware/optimistic-concurrency'; // BigInt doesn't serialize via JSON.stringify by default — every response // here that carries one converts to a string explicitly. The prepared XDR @@ -62,6 +63,7 @@ export async function getOwnWallet(req: Request, res: Response) { export async function getIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.getPaymentIntent(id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } @@ -81,18 +83,21 @@ export async function prepareIntent(req: Request, res: Response) { export async function fundIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.fundPaymentIntent(auditActor(req), id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } export async function releaseIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.releasePaymentIntent(auditActor(req), id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } export async function refundIntent(req: Request, res: Response) { const id = getParam(req, 'id'); const intent = await paymentsService.refundPaymentIntent(auditActor(req), id); + setVersionEtag(res, intent.version); res.status(200).json(serializeIntent(intent)); } diff --git a/backend/src/modules/payments/repositories/index.ts b/backend/src/modules/payments/repositories/index.ts index fb4f1bd3..58516a54 100644 --- a/backend/src/modules/payments/repositories/index.ts +++ b/backend/src/modules/payments/repositories/index.ts @@ -183,7 +183,7 @@ export async function markEscrowed( ) { const result = await tx.paymentIntent.updateMany({ where: { id, status: 'created' }, - data: { status: 'escrowed', ...input }, + data: { status: 'escrowed', ...input, version: { increment: 1 } }, }); return result.count === 1 ? tx.paymentIntent.findUniqueOrThrow({ where: { id } }) : null; } @@ -248,7 +248,10 @@ export async function markStatus( from: PaymentIntentStatus, to: PaymentIntentStatus, ) { - const result = await tx.paymentIntent.updateMany({ where: { id, status: from }, data: { status: to } }); + const result = await tx.paymentIntent.updateMany({ + where: { id, status: from }, + data: { status: to, version: { increment: 1 } }, + }); return result.count === 1 ? tx.paymentIntent.findUniqueOrThrow({ where: { id } }) : null; } diff --git a/backend/tests/integration/optimistic-concurrency.test.mts b/backend/tests/integration/optimistic-concurrency.test.mts new file mode 100644 index 00000000..bf0503b7 --- /dev/null +++ b/backend/tests/integration/optimistic-concurrency.test.mts @@ -0,0 +1,359 @@ +// Integration tests for optimistic concurrency on mutable records +// (Phase 8, issue #49; convention documented in ARCHITECTURE.md §5.4). +// +// This is the lost-update proof: two actors read the same record, both +// decide to write, and the API must let exactly one of them through and +// answer the other with 412 instead of silently discarding their change. +// Self-contained, same embedded-Postgres pattern as booking.test.mts, on its +// own port (5443). The escrow SDK is faked — on-chain behaviour is the +// testnet suite's job. +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import EmbeddedPostgres from 'embedded-postgres'; + +const BACKEND_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const PORT = 5443; +const runId = Date.now(); +const TX_HASH = 'd'.repeat(64); + +const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'docforum-concurrency-pg-')); +const pg = new EmbeddedPostgres({ + databaseDir: dataDir, + user: 'docforum', + password: 'docforum', + port: PORT, + persistent: false, +}); +const databaseUrl = `postgresql://docforum:docforum@localhost:${PORT}/docforum_concurrency_test`; + +type App = ReturnType; +type Prisma = typeof import('../../src/db/prisma-client.js').prisma; + +const emailedTokens = new Map(); + +describe('optimistic concurrency (version + If-Match)', () => { + let app: App; + let prisma: Prisma; + let admin: { id: string; token: string }; + let doctor: { userId: string; profileId: string; token: string }; + /** A second doctor, so the admin verification test can't move the version + * the PATCH /doctors/me assertions are built on. */ + let otherDoctor: { userId: string; profileId: string; token: string }; + let patient: { userId: string; profileId: string; token: string }; + let facilityId: string; + let slotId: string; + + const adminAuth = () => ({ Authorization: `Bearer ${admin.token}` }); + const asUser = (d: { token: string }) => ({ Authorization: `Bearer ${d.token}` }); + + async function signup(role: 'patient' | 'doctor', label: string) { + const email = `${label}-${runId}@example.com`; + const res = await request(app) + .post('/auth/signup') + .send({ role, email, password: 'longenough123', fullName: `${label} Fullname` }); + expect(res.status).toBe(200); + expect((await request(app).post('/auth/verify-email').send({ token: emailedTokens.get(email) })).status).toBe(200); + return { userId: res.body.user.id as string, token: res.body.accessToken as string, email }; + } + + async function createIntent() { + return prisma.paymentIntent.create({ + data: { + orderType: 'lab_order', + orderId: `order-${Math.random()}`, + patientProfileId: patient.profileId, + facilityId, + amountStroops: 5000n, + }, + }); + } + + beforeAll(async () => { + await pg.initialise(); + await pg.start(); + await pg.createDatabase('docforum_concurrency_test'); + execFileSync('npx', ['prisma', 'migrate', 'deploy'], { + cwd: BACKEND_ROOT, + env: { ...process.env, DATABASE_URL: databaseUrl }, + stdio: 'pipe', + }); + + process.env.DATABASE_URL = databaseUrl; + process.env.PLATFORM_PAYER_SECRET = `CCONCPPAYERSENTINEL${runId}`; + process.env.PLATFORM_RELEASER_SECRET = `CCONCPRELEASERSENTINEL${runId}`; + process.env.ESCROW_CONTRACT_ID = 'CTESTCONTRACT'; + process.env.ESCROW_TOKEN_ID = 'CTESTTOKEN'; + + const appModule = await import('../../src/app.js'); + const dbModule = await import('../../src/db/prisma-client.js'); + const authServices = await import('../../src/modules/auth/services/index.js'); + const paymentsServices = await import('../../src/modules/payments/services/index.js'); + app = appModule.createApp(); + prisma = dbModule.prisma; + + authServices.setEmailSender(async (message) => { + const match = /[?&]token=([^&\s]+)/.exec(message.text); + if (match) emailedTokens.set(message.to, decodeURIComponent(match[1])); + }); + + class FakeEscrowClient { + async createEscrow() { + return { escrowId: 501n, txHash: TX_HASH }; + } + async release() {} + async refund() {} + } + paymentsServices.setEscrowSdkForTests({ + Keypair: { fromSecret: () => ({ publicKey: () => 'GPLATFORMRELEASERPUBLICKEY' }) }, + EscrowClient: FakeEscrowClient, + } as unknown as import('../../src/modules/payments/services/index.js').EscrowSdkModule); + + const adminUser = await prisma.user.create({ + data: { email: `admin-${runId}@example.com`, passwordHash: 'x', role: 'admin', emailVerifiedAt: new Date() }, + }); + admin = { + id: adminUser.id, + token: jwt.sign({ sub: adminUser.id, role: 'admin' }, 'test-access-secret', { expiresIn: '15m' }), + }; + + const doctorUser = await signup('doctor', 'cc-doctor'); + doctor = { ...doctorUser, profileId: (await prisma.doctorProfile.findUniqueOrThrow({ where: { userId: doctorUser.userId } })).id }; + const otherDoctorUser = await signup('doctor', 'cc-doctor-other'); + otherDoctor = { + ...otherDoctorUser, + profileId: (await prisma.doctorProfile.findUniqueOrThrow({ where: { userId: otherDoctorUser.userId } })).id, + }; + const patientUser = await signup('patient', 'cc-patient'); + patient = { + ...patientUser, + profileId: (await prisma.patientProfile.findUniqueOrThrow({ where: { userId: patientUser.userId } })).id, + }; + + const tomorrow = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString().slice(0, 10); + await request(app) + .post(`/availability/doctors/${doctor.profileId}/slots/generate`) + .set(asUser(doctor)) + .send({ date: tomorrow, startHour: 9, endHour: 12, slotLengthMinutes: 30 }); + slotId = ((await request(app).get(`/availability/doctors/${doctor.profileId}/slots`)).body as { id: string }[])[0]!.id; + + const facilityUser = await prisma.user.create({ data: { email: `lab-${runId}@example.com`, passwordHash: 'x', role: 'facility' } }); + facilityId = (await prisma.facilityProfile.create({ data: { userId: facilityUser.id, name: 'Concurrency Lab', type: 'lab' } })).id; + await prisma.walletLink.create({ data: { facilityId, stellarPublicKey: `G${'B'.repeat(55)}` } }); + }, 180_000); + + afterAll(async () => { + const authServices = await import('../../src/modules/auth/services/index.js'); + const paymentsServices = await import('../../src/modules/payments/services/index.js'); + authServices.resetEmailSender(); + paymentsServices.setEscrowSdkForTests(undefined); + await prisma?.$disconnect(); + await pg.stop(); + fs.rmSync(dataDir, { recursive: true, force: true }); + }); + + describe('doctor profile — PATCH /doctors/me', () => { + it('publishes a strong version ETag on the individual resource read', async () => { + const res = await request(app).get('/doctors/me').set(asUser(doctor)); + expect(res.status).toBe(200); + expect(res.body.version).toBe(1); + expect(res.headers.etag).toBe('"1"'); + }); + + it('refuses a write with no If-Match and says how to recover (428)', async () => { + const res = await request(app).patch('/doctors/me').set(asUser(doctor)).send({ fullName: 'No Precondition' }); + expect(res.status).toBe(428); + expect(res.body.code).toBe('IF_MATCH_REQUIRED'); + expect(res.body.message).toMatch(/If-Match/); + // Nothing changed: a refused precondition is not a partial write. + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).fullName).not.toBe( + 'No Precondition', + ); + }); + + it('rejects a weak validator, a list and garbage with 400 rather than guessing', async () => { + for (const value of ['W/"2"', '"2", "3"', '2', 'abc', '"-1"', '"0"']) { + const res = await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', value).send({ fullName: 'Bad Tag' }); + expect(res.status, `If-Match: ${value}`).toBe(400); + expect(res.body.code).toBe('INVALID_IF_MATCH'); + } + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version).toBe(1); + }); + + it('applies the write and increments the version in the same response', async () => { + const current = await request(app).get('/doctors/me').set(asUser(doctor)); + const res = await request(app) + .patch('/doctors/me') + .set(asUser(doctor)) + .set('If-Match', current.headers.etag) + .send({ fullName: 'Renamed Doctor' }); + expect(res.status).toBe(200); + expect(res.body.fullName).toBe('Renamed Doctor'); + expect(res.body.version).toBe(current.body.version + 1); + expect(res.headers.etag).toBe(`"${res.body.version}"`); + // The database agrees — the increment happened in the guarded UPDATE. + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version).toBe( + res.body.version, + ); + }); + + it('rejects a stale write with 412 and leaves the winning value in place', async () => { + const stale = (await request(app).get('/doctors/me').set(asUser(doctor))).headers.etag; + const first = await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', stale).send({ fullName: 'First Writer' }); + expect(first.status).toBe(200); + + const loser = await request(app) + .patch('/doctors/me') + .set(asUser(doctor)) + .set('If-Match', stale) + .send({ fullName: 'Second Writer' }); + expect(loser.status).toBe(412); + expect(loser.body.code).toBe('VERSION_CONFLICT'); + + // The classic lost-update check: the first writer's value survived, and + // the version moved exactly once — not twice. + const stored = await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } }); + expect(stored.fullName).toBe('First Writer'); + expect(stored.version).toBe(first.body.version); + + // And re-reading then retrying the loser's intent succeeds. + const retry = await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', `"${stored.version}"`).send({ + fullName: 'Second Writer', + }); + expect(retry.status).toBe(200); + expect(retry.body.fullName).toBe('Second Writer'); + }); + + it('lets exactly one of 8 simultaneous writes on the same version through', async () => { + const etag = (await request(app).get('/doctors/me').set(asUser(doctor))).headers.etag; + const versionBefore = (await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version; + + const responses = await Promise.all( + Array.from({ length: 8 }, (_, i) => + request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', etag).send({ fullName: `Racer ${i}` }), + ), + ); + + expect(responses.filter((r) => r.status === 200)).toHaveLength(1); + expect(responses.filter((r) => r.status === 412)).toHaveLength(7); + for (const loser of responses.filter((r) => r.status === 412)) expect(loser.body.code).toBe('VERSION_CONFLICT'); + + const stored = await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } }); + expect(stored.version).toBe(versionBefore + 1); + expect(stored.fullName).toBe(responses.find((r) => r.status === 200)!.body.fullName); + }); + + it('authorises before checking the precondition, and validates the body', async () => { + // No token -> 401, not a hint that this route has a precondition. + expect((await request(app).patch('/doctors/me').send({ fullName: 'Anon' })).status).toBe(401); + // Wrong role -> 403, likewise. + expect((await request(app).patch('/doctors/me').set(asUser(patient)).send({ fullName: 'Patient' })).status).toBe( + 403, + ); + + const etag = (await request(app).get('/doctors/me').set(asUser(doctor))).headers.etag; + const before = (await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version; + expect( + (await request(app).patch('/doctors/me').set(asUser(doctor)).set('If-Match', etag).send({ fullName: '' })).status, + ).toBe(400); + expect( + ( + await request(app) + .patch('/doctors/me') + .set(asUser(doctor)) + .set('If-Match', etag) + .send({ fullName: 'Sneaky', verificationStatus: 'verified' }) + ).status, + ).toBe(400); + expect((await prisma.doctorProfile.findUniqueOrThrow({ where: { id: doctor.profileId } })).version).toBe(before); + }); + }); + + describe('already-conditional transitions are not given a second guard', () => { + it('books without If-Match — the slot claim is already the guard', async () => { + const res = await request(app) + .post('/appointments') + .set({ Authorization: `Bearer ${patient.token}` }) + .send({ availabilitySlotId: slotId }); + expect(res.status).toBe(201); + expect((await prisma.appointment.findUniqueOrThrow({ where: { id: res.body.id } })).version).toBe(1); + }); + + it('flips doctor verification without If-Match and still bumps the version', async () => { + const before = await prisma.doctorProfile.findUniqueOrThrow({ where: { id: otherDoctor.profileId } }); + const res = await request(app) + .patch(`/doctors/${otherDoctor.profileId}/verification`) + .set(adminAuth()) + .send({ status: 'verified' }); + expect(res.status).toBe(200); + expect(res.body.verificationStatus).toBe('verified'); + expect(res.body.version).toBe(before.version + 1); + }); + + it('funds and releases without If-Match, advancing the version each time', async () => { + const intent = await createIntent(); + expect(intent.version).toBe(1); + + const read = await request(app).get(`/payments/intents/${intent.id}`).set(adminAuth()); + expect(read.status).toBe(200); + expect(read.headers.etag).toBe('"1"'); + expect(read.body.version).toBe(1); + + const funded = await request(app).post(`/payments/intents/${intent.id}/fund`).set(adminAuth()); + expect(funded.status).toBe(200); + expect(funded.body.version).toBe(2); + expect(funded.headers.etag).toBe('"2"'); + + const released = await request(app).post(`/payments/intents/${intent.id}/release`).set(adminAuth()); + expect(released.status).toBe(200); + expect(released.body.version).toBe(3); + + expect((await prisma.paymentIntent.findUniqueOrThrow({ where: { id: intent.id } })).version).toBe(3); + + // The status transition's own conditional write is untouched: a second + // release still loses on status (409), not on the version. + expect((await request(app).post(`/payments/intents/${intent.id}/release`).set(adminAuth())).status).toBe(409); + expect((await prisma.paymentIntent.findUniqueOrThrow({ where: { id: intent.id } })).version).toBe(3); + }); + }); + + describe('schema', () => { + it('puts a version column on the mutable models and nothing else', async () => { + const rows = await prisma.$queryRaw<{ table_name: string }[]>` + SELECT table_name FROM information_schema.columns + WHERE table_schema = 'public' AND column_name = 'version' + ORDER BY table_name + `; + // Append-only Prescription/LabOrder and every immutable record + // (Specialty, WalletLink, AuditEvent, the export tables) are absent by + // design — see ARCHITECTURE.md §5.4.4. + expect(rows.map((r) => r.table_name)).toEqual([ + 'Appointment', + 'DoctorProfile', + 'FacilityCapability', + 'FacilityProfile', + 'PatientProfile', + 'PaymentIntent', + 'User', + ]); + }); + + it('bumps the User version when the address is verified', async () => { + const email = `cc-verify-${runId}@example.com`; + const signupRes = await request(app) + .post('/auth/signup') + .send({ role: 'patient', email, password: 'longenough123', fullName: 'Verify Me' }); + expect(signupRes.status).toBe(200); + const userId = signupRes.body.user.id as string; + expect((await prisma.user.findUniqueOrThrow({ where: { id: userId } })).version).toBe(1); + + expect((await request(app).post('/auth/verify-email').send({ token: emailedTokens.get(email) })).status).toBe(200); + expect((await prisma.user.findUniqueOrThrow({ where: { id: userId } })).version).toBe(2); + }); + }); +}); \ No newline at end of file