diff --git a/docs/parity-matrix.md b/docs/parity-matrix.md index 9245d46..ab6f88f 100644 --- a/docs/parity-matrix.md +++ b/docs/parity-matrix.md @@ -77,13 +77,13 @@ the methods below. | 6 | `getAvailableDates` | **REAL** — engine: AE :: `getDatesWithAvailability` › `returns only days with business hours` (adapter wiring pinned only by the HG shape test) | **REAL** — AT :: `transforms availability dates`; AI :: `returns dates within the requested range` | **REAL** — `POST /availability/dates`: AD :: `queues the next month after a successful date request`; AD :: `serves a cached month from Redis and queues the following month` | | 7 | `getAvailableSlots` | **REAL** — engine: AE :: `generates slots for a 5-hour window with 60min duration at 30min intervals`; AE :: `removes slots that overlap with bookings` | **REAL** — AT :: `transforms availability times to TimeSlot`; AI :: `returns time slots for a specific date` | **REAL** — `POST /availability/slots`: AS :: `serves repeated slot requests from Redis without rereading Acuity` | | 8 | `checkSlotAvailability` | **REAL** — engine: AE :: `isSlotAvailable` › `returns true for an open slot within hours` | **REAL** — AT :: `checkSlotAvailability returns boolean`; AI :: `checkSlotAvailability returns boolean` | **REAL** — `POST /availability/check` does a genuine slot read + membership match (`wizard.ts` slot-membership path), but no route-level test exists (gap G4). | -| 9 | `softHoldSlot` | **REAL** — HG :: `inserts an advisory soft hold and returns SlotSoftHold`; held slots actually block availability (`homegrown.ts:371–400`) | **REAL** — Acuity blocks API: AT :: `transforms block to SlotSoftHold`; AT :: `requires provider ID for soft hold`; AI :: `creates soft hold (block) for slot protection` | **ADVISORY-ONLY, ALWAYS-FAILS** — `Effect.fail(ReservationError BLOCK_FAILED)` at `remote-adapter.ts:263` and `wizard.ts:304`. Kit pipeline tolerates it: PL :: `continues without softHold if softHold fails`. No bridge test asserts the failure itself (gap G5). See note below. | +| 9 | `softHoldSlot` | **REAL** — HG :: `inserts an advisory soft hold and returns SlotSoftHold`; with `withTransaction`, practitioner-day locking makes hold acquisition an atomic pre-payment admission gate and the Postgres concurrency test proves only one same-slot hold wins | **REAL** — Acuity blocks API: AT :: `transforms block to SlotSoftHold`; AT :: `requires provider ID for soft hold`; AI :: `creates soft hold (block) for slot protection` | **ADVISORY-ONLY, ALWAYS-FAILS** — `Effect.fail(ReservationError BLOCK_FAILED)` at `remote-adapter.ts:263` and `wizard.ts:304`. Kit pipeline tolerates unsupported holds but propagates `SLOT_TAKEN` before payment. No bridge test asserts the failure itself (gap G5). See note below. | | 10 | `releaseSoftHold` | **REAL** — HG :: `sets releasedAt on the soft hold` | **REAL** — `DELETE /blocks/:id`: AI :: `creates soft hold (block) for slot protection` (asserts block count 1 → 0 after release) | **ADVISORY** — no-op `Effect.succeed(undefined)`, `remote-adapter.ts:271`. *(untested — gap G5)*. Pipeline-level call evidence (mock): PL :: `releases soft hold on payment intent failure`. | -| 11 | `createBooking` | **REAL** — HG :: `resolves service, finds client, gets practitioner, inserts booking`; idempotency: HG :: `replays the existing booking for a duplicate idempotency key without a second insert` | **REAL** — AI :: `creates and cancels a booking` | **TOMBSTONED** — `POST /booking/create` answers `410 ASYNC_REQUIRED` (`handler.ts:1373`, delegating to the paid-route tombstone at `handler.ts:1376`). Real bookings run only via async jobs: AN :: `enqueues paid booking commands without running browser automation in the request`; FR :: `produces the booking and the full journal evidence trail on success`. The unpaid tombstone itself has no dedicated test (gap G6). | +| 11 | `createBooking` | **REAL** — HG :: `resolves service, finds client, gets practitioner, inserts booking`; idempotency: HG :: `replays the existing booking for a duplicate idempotency key without a second insert`; write safety: practitioner-day transaction lock + occupied re-read reject same-slot and overlapping concurrent writers with `SLOT_TAKEN` | **REAL** — AI :: `creates and cancels a booking` | **TOMBSTONED** — `POST /booking/create` answers `410 ASYNC_REQUIRED` (`handler.ts:1373`, delegating to the paid-route tombstone at `handler.ts:1376`). Real bookings run only via async jobs: AN :: `enqueues paid booking commands without running browser automation in the request`; FR :: `produces the booking and the full journal evidence trail on success`. The unpaid tombstone itself has no dedicated test (gap G6). | | 12 | `createBookingWithPaymentRef` | **REAL** — creates then stamps `paymentRef`/`paymentMethod`/`paymentStatus` (`homegrown.ts`), but only the HG shape test pins it *(behavior untested — gap G7)* | **REAL** — AT :: `includes payment ref from notes` | **TOMBSTONED** — `410 ASYNC_REQUIRED`: BP :: `rejects the synchronous paid booking endpoint so consumers migrate to async jobs`; BP :: `does not run the old sync endpoint even when a request-scoped coupon is present`. Real path is the async job protocol (same AN/FR citations as #11), with segment-boundary resume (note below). | | 13 | `getBooking` | **REAL** — HG :: `joins booking, service, client, and practitioner data` | **REAL** — AT :: `transforms appointment to Booking` | **ADVISORY** — `NOT_IMPLEMENTED` fail, `remote-adapter.ts:291` / `wizard.ts:339`. *(untested — gap G8)* | | 14 | `cancelBooking` | **REAL** — HG :: `sets status to cancelled` | **REAL** — AI :: `creates and cancels a booking` | **ADVISORY** — `NOT_IMPLEMENTED` fail, `remote-adapter.ts:294` / `wizard.ts:342`. *(untested — gap G8)* | -| 15 | `rescheduleBooking` | **REAL** — HG :: `updates datetime and returns refreshed booking` | **REAL** — AI :: `handles reschedule operation` | **ADVISORY** — `NOT_IMPLEMENTED` fail, `remote-adapter.ts:297` / `wizard.ts:345`. *(untested — gap G8)* | +| 15 | `rescheduleBooking` | **REAL** — HG :: `updates datetime and returns refreshed booking`; the target slot is transaction-locked and revalidated, while cancelled/completed bookings are rejected | **REAL** — AI :: `handles reschedule operation` | **ADVISORY** — `NOT_IMPLEMENTED` fail, `remote-adapter.ts:297` / `wizard.ts:345`. *(untested — gap G8)* | | 16 | `findOrCreateClient` | **REAL** — HG :: `returns existing client with isNew=false and updates info`; HG :: `creates new client when email not found` | **REAL** — AT :: `finds existing client by email`; AT :: `indicates new client when email not found` | **ADVISORY** — synthetic `local-${email}` stub, `remote-adapter.ts:304`. *(untested — gap G2)* | | 17 | `getClientByEmail` | **REAL** — HG :: `returns ClientInfo when client exists`; HG :: `returns null when client not found` | **REAL** — AT :: `returns null for unknown client email` | **ADVISORY** — always `null`, `remote-adapter.ts:307`. *(untested — gap G2)* | @@ -93,7 +93,7 @@ scope for this matrix. ## Verified prompt-03 flags -### `softHoldSlot` is advisory-only and currently failing — confirmed, with nuance +### Bridge `softHoldSlot` is advisory-only and currently failing — confirmed, with nuance Both bridge implementations return a **failed Effect on every call** — `Errors.reservation('BLOCK_FAILED', 'Advisory soft holds are not supported…')` @@ -101,8 +101,9 @@ at `remote-adapter.ts:263–270` and `wizard.ts:304–311` (bridge `fc1c328`). The nuance: this is an **intentional always-fail by design**, not a regressing or flaky test — the interface itself documents soft holds as advisory (`src/adapters/types.ts:98–105`), and the kit booking pipeline degrades -gracefully: PL :: `continues without softHold if softHold fails` proves a -booking still completes with `softHold: undefined`. What is missing is any +gracefully for `BLOCK_FAILED`: PL :: `continues without softHold if softHold +fails` proves a booking still completes with `softHold: undefined`. +`SLOT_TAKEN` is not swallowed; it stops before payment. What is missing is any bridge-side test asserting the `BLOCK_FAILED` failure contract itself (gap G5). ### Bridge resume = segment-boundary replay — confirmed diff --git a/src/adapters/__tests__/homegrown-adapter.test.ts b/src/adapters/__tests__/homegrown-adapter.test.ts index c504838..39f90b7 100644 --- a/src/adapters/__tests__/homegrown-adapter.test.ts +++ b/src/adapters/__tests__/homegrown-adapter.test.ts @@ -91,6 +91,11 @@ vi.mock("drizzle-orm", () => ({ lt: (col: string, val: unknown) => ({ op: "lt", col, val }), gt: (col: string, val: unknown) => ({ op: "gt", col, val }), isNull: (col: string) => ({ op: "isNull", col }), + not: (arg: unknown) => ({ op: "not", arg }), + sql: (strings: TemplateStringsArray, ...values: unknown[]) => ({ + strings, + values, + }), })); // --------------------------------------------------------------------------- @@ -107,7 +112,8 @@ type MockRow = Record; * `.orderBy(...)`. An `Error` entry rejects on either path. */ const selectTerminal = (entry: MockRow[] | Error) => { - const p = entry instanceof Error ? Promise.reject(entry) : Promise.resolve(entry); + const p = + entry instanceof Error ? Promise.reject(entry) : Promise.resolve(entry); // Suppress unhandled-rejection noise on the branch that is never awaited. p.catch(() => {}); const settle = @@ -199,11 +205,9 @@ const createSequencedMockDb = ( }; return { - select: vi - .fn() - .mockImplementation(() => ({ - from: vi.fn().mockImplementation(makeSelectChain), - })), + select: vi.fn().mockImplementation(() => ({ + from: vi.fn().mockImplementation(makeSelectChain), + })), insert: vi.fn().mockImplementation(makeInsertChain), update: vi.fn().mockReturnValue({ set: vi.fn().mockReturnValue({ @@ -561,7 +565,10 @@ describe("HomegrownAdapter", () => { describe("softHoldSlot", () => { it("inserts an advisory soft hold and returns SlotSoftHold", async () => { - const mockDb = createMockDb({ insert: [RESERVATION_ROW] }); + const mockDb = createSequencedMockDb( + [[], [], []], // occupied bookings, time blocks, active holds + [[RESERVATION_ROW]], + ); const adapter = createAdapter({ getDb: async () => mockDb }); const result = await Effect.runPromise( @@ -584,7 +591,10 @@ describe("HomegrownAdapter", () => { }); it("defaults expiration to 10 minutes when not specified", async () => { - const mockDb = createMockDb({ insert: [RESERVATION_ROW] }); + const mockDb = createSequencedMockDb( + [[], [], []], // occupied bookings, time blocks, active holds + [[RESERVATION_ROW]], + ); const adapter = createAdapter({ getDb: async () => mockDb }); // The adapter calculates expiresAt internally — we just verify the @@ -599,6 +609,49 @@ describe("HomegrownAdapter", () => { expect(result.id).toBe("res-uuid-1"); }); + + it("starts expiration after waiting for the practitioner-day lock", async () => { + const initialNow = new Date("2026-04-20T13:00:00.000Z").getTime(); + let now = initialNow; + const dateNow = vi.spyOn(Date, "now").mockImplementation(() => now); + const mockDb = Object.assign( + createSequencedMockDb( + [[PRACTITIONER_ROW], [], [], []], + [[RESERVATION_ROW]], + ), + { + execute: vi.fn().mockImplementation(async () => { + // Simulate lock contention longer than the requested hold TTL. + now = initialNow + 11 * 60_000; + }), + }, + ); + + try { + const adapter = createAdapter({ + getDb: async () => mockDb, + withTransaction: (fn) => fn(mockDb), + }); + + await Effect.runPromise( + adapter.softHoldSlot({ + serviceId: "svc-uuid-1", + datetime: "2026-04-20T14:00:00.000Z", + duration: 60, + expirationMinutes: 10, + }), + ); + + const insertChain = mockDb.insert.mock.results[0]?.value; + expect(insertChain.values).toHaveBeenCalledWith( + expect.objectContaining({ + expiresAt: "2026-04-20T13:21:00.000Z", + }), + ); + } finally { + dateNow.mockRestore(); + } + }); }); describe("releaseSoftHold", () => { @@ -947,13 +1000,11 @@ describe("HomegrownAdapter", () => { })), insert: vi.fn().mockReturnValue({ values: vi.fn().mockReturnValue({ - returning: vi - .fn() - .mockRejectedValue( - Object.assign(new Error("duplicate key value"), { - code: "23505", - }), - ), + returning: vi.fn().mockRejectedValue( + Object.assign(new Error("duplicate key value"), { + code: "23505", + }), + ), }), }), update: vi.fn().mockReturnValue({ @@ -1308,7 +1359,9 @@ describe("HomegrownAdapter", () => { getDb: async () => mockDb, }); - const error = await Effect.runPromise(Effect.flip(adapter.getProviders())); + const error = await Effect.runPromise( + Effect.flip(adapter.getProviders()), + ); expect(error._tag).toBe("ValidationError"); expect(error).toMatchObject({ field: "defaultPractitionerHandle" }); @@ -1575,7 +1628,9 @@ describe("HomegrownAdapter", () => { const { db } = createGateMockDb([adjacent]); const adapter = createAdapter({ getDb: async () => db }); - const result = await Effect.runPromise(adapter.createBooking(bookingRequest)); + const result = await Effect.runPromise( + adapter.createBooking(bookingRequest), + ); expect(result.id).toBe("booking-uuid-1"); expect(db.insert).toHaveBeenCalledOnce(); @@ -1614,6 +1669,97 @@ describe("HomegrownAdapter", () => { ); }); + it("queries the previous local day when the configured buffer crosses midnight", async () => { + const { db, whereConds } = createGateMockDb([]); + const adapter = createAdapter({ + getDb: async () => db, + bufferMinutes: 30, + }); + + await Effect.runPromise( + adapter.createBooking({ + ...bookingRequest, + // 00:10 EDT on April 21; the 30-minute buffer starts at 23:40 EDT + // on April 20, so occupied rows from the prior local day matter. + datetime: "2026-04-21T04:10:00.000Z", + }), + ); + + const cond = whereConds.find( + (c: any) => + c?.op === "and" && + Array.isArray(c.args) && + c.args.some((a: any) => a?.col === "datetime" && a?.op === "lt"), + ) as { args: Array<{ col: string; op: string; val: string }> }; + const lowerBound = cond.args.find( + (a) => a.col === "endTime" && a.op === "gt", + )?.val; + + expect(lowerBound).toBe("2026-04-20T04:00:00.000Z"); + }); + + it("queries the next local day when the configured buffer crosses midnight", async () => { + const { db, whereConds } = createGateMockDb([]); + const adapter = createAdapter({ + getDb: async () => db, + bufferMinutes: 30, + }); + + await Effect.runPromise( + adapter.createBooking({ + ...bookingRequest, + // 22:50-23:50 EDT on April 21; the 30-minute buffer ends at 00:20 + // EDT on April 22, so occupied rows from the next local day matter. + datetime: "2026-04-22T02:50:00.000Z", + }), + ); + + const cond = whereConds.find( + (c: any) => + c?.op === "and" && + Array.isArray(c.args) && + c.args.some((a: any) => a?.col === "datetime" && a?.op === "lt"), + ) as { args: Array<{ col: string; op: string; val: string }> }; + const upperBound = cond.args.find( + (a) => a.col === "datetime" && a.op === "lt", + )?.val; + + expect(upperBound).toBe("2026-04-23T04:00:00.000Z"); + }); + + it("excludes a soft hold only when its id and slot match the booking", async () => { + const { db, whereConds } = createGateMockDb([]); + const adapter = createAdapter({ getDb: async () => db }); + + await Effect.runPromise( + adapter.createBooking({ + ...bookingRequest, + softHoldId: "owned-hold-id", + }), + ); + + const holdQuery = whereConds.find( + (c: any) => + c?.op === "and" && + Array.isArray(c.args) && + c.args.some((a: any) => a?.op === "isNull"), + ) as { args: Array<{ op: string; arg?: { args?: unknown[] } }> }; + const exclusion = holdQuery.args.find((arg) => arg.op === "not"); + + expect(exclusion?.arg).toEqual({ + op: "and", + args: [ + { op: "eq", col: "id", val: "owned-hold-id" }, + { + op: "eq", + col: "datetime", + val: "2026-04-20T14:00:00.000Z", + }, + { op: "eq", col: "duration", val: 60 }, + ], + }); + }); + it("rejects rescheduleBooking with SLOT_TAKEN when the new slot overlaps another booking", async () => { // A different booking occupies the target slot. The reschedule gate // excludes the booking being moved, so this conflict is a real one. diff --git a/src/adapters/homegrown.ts b/src/adapters/homegrown.ts index ffc16ab..412d333 100644 --- a/src/adapters/homegrown.ts +++ b/src/adapters/homegrown.ts @@ -82,19 +82,22 @@ export interface HomegrownAdapterConfig { /** * Optional transaction runner for the slot-validating write paths. * - * When provided, createBooking and rescheduleBooking run their + * When provided, softHoldSlot, createBooking, and rescheduleBooking run their * read-validate-write critical section inside this callback so the * availability re-check and the insert/update are one atomic unit, and a * practitioner-day Postgres advisory lock (pg_advisory_xact_lock) can * serialize concurrent writers for the same practitioner + local calendar * day, including writers targeting overlapping but different start times. - * Supply `(fn) => db.transaction(fn)`. + * This also makes soft-hold acquisition a pre-payment admission gate: two + * checkouts cannot both acquire a hold for one slot. Supply + * `(fn) => db.transaction(fn)`. * * When omitted, those paths fall back to withDb and the atomicity of the * critical section then depends entirely on the semantics of the consumer's - * withDb: a bare passthrough gives none, so two concurrent writers can still - * double-book the same slot. Provide withTransaction in production Postgres - * deployments; the two-writer integration test only closes the race with it. + * withDb: a bare passthrough gives none, so two concurrent checkouts can both + * acquire holds and two concurrent writers can still double-book the same + * slot. Provide withTransaction in production Postgres deployments; the + * concurrency integration tests only close these races with it. * * Scope of the additive change: supplying withTransaction is additive for the * advisory lock only. Omitting it disables the lock (concurrent writers are no @@ -229,6 +232,13 @@ export const createHomegrownAdapter = ( // from any other pg_advisory_xact_lock users sharing the connection. const SLOT_LOCK_NAMESPACE = 0x5343; + /** Advance a YYYY-MM-DD key by one calendar day without timezone drift. */ + const nextDayKey = (dayKey: string): string => { + const date = new Date(`${dayKey}T00:00:00.000Z`); + date.setUTCDate(date.getUTCDate() + 1); + return date.toISOString().slice(0, 10); + }; + /** * Derive a signed 32-bit key (pg int4) for a resource + local day * (YYYY-MM-DD) via FNV-1a. Concurrent writers targeting the same @@ -269,21 +279,16 @@ export const createHomegrownAdapter = ( endIso: string, ): Promise => { const { sql } = await import("drizzle-orm"); - const bufferedStart = new Date(new Date(startIso).getTime() - buffer * 60_000); + const bufferedStart = new Date( + new Date(startIso).getTime() - buffer * 60_000, + ); const bufferedEnd = new Date(new Date(endIso).getTime() + buffer * 60_000); - const dayKeys = new Set(); - // Walk the padded interval in 24h steps (plus the endpoint) so every - // local date it touches is covered regardless of span length. - for ( - let t = bufferedStart.getTime(); - t < bufferedEnd.getTime(); - t += 24 * 60 * 60_000 - ) { - dayKeys.add(toDateString(new Date(t).toISOString(), tz)); - } - dayKeys.add(toDateString(bufferedEnd.toISOString(), tz)); - // ISO YYYY-MM-DD sorts lexicographically == chronologically. - for (const day of [...dayKeys].sort()) { + const firstDay = toDateString(bufferedStart.toISOString(), tz); + const lastDay = toDateString(bufferedEnd.toISOString(), tz); + // Walk calendar-day keys, not fixed 24-hour UTC jumps. A spring-forward + // transition makes a local day 23 hours and can otherwise skip a date for + // long services or buffers. + for (let day = firstDay; day <= lastDay; day = nextDayKey(day)) { await d.execute( sql`select pg_advisory_xact_lock(${SLOT_LOCK_NAMESPACE}::int4, ${advisoryLockKey( resource, @@ -459,22 +464,23 @@ export const createHomegrownAdapter = ( * * @param excludeBookingId - booking id to omit; a reschedule must not treat * the booking being moved as a conflict with itself. - * @param excludeHoldId - soft-hold id to omit; a booking completing a held - * checkout must not treat the caller's own hold as a conflict. + * @param excludeHold - exact soft-hold capability to omit; a booking + * completing a held checkout must not treat its own matching hold as a + * conflict, while a stale or unrelated id remains occupied. */ const loadOccupiedWith = async ( d: any, startDate: string, endDate: string, excludeBookingId?: string, - excludeHoldId?: string, + excludeHold?: { id: string; datetime: string; duration: number }, ): Promise => { const { bookings: bookingsTable, timeBlocks, slotReservations, } = (await loadSchemas()).booking; - const { gte, lt, and, ne, isNull, gt } = await import("drizzle-orm"); + const { lt, and, eq, ne, not, isNull, gt } = await import("drizzle-orm"); const { startIso, endExclusiveIso } = occupiedDayBoundsUtc( startDate, @@ -513,17 +519,28 @@ export const createHomegrownAdapter = ( ), ); - // Active soft holds (not expired, not released). Reservations carry no end - // column, so they are matched by start instant within the tz-correct - // window rather than by interval overlap. + // Active soft holds (not expired, not released). Reservations carry their + // duration separately, so query every active hold that starts before the + // window ends and perform the end-time overlap check in memory. A lower + // start bound would miss a hold that begins before the window and runs + // into it. const holdConds = [ - gte(slotReservations.datetime, startIso), lt(slotReservations.datetime, endExclusiveIso), gt(slotReservations.expiresAt, new Date().toISOString()), isNull(slotReservations.releasedAt), ]; - if (excludeHoldId) { - holdConds.push(ne(slotReservations.id, excludeHoldId)); + if (excludeHold) { + // Treat the opaque id as a capability only for the exact candidate it + // represents. A stale or unrelated id must not suppress another hold. + holdConds.push( + not( + and( + eq(slotReservations.id, excludeHold.id), + eq(slotReservations.datetime, excludeHold.datetime), + eq(slotReservations.duration, excludeHold.duration), + )!, + ), + ); } const softHoldRows = await d .select({ @@ -580,17 +597,30 @@ export const createHomegrownAdapter = ( excludeBookingId?: string, excludeHoldId?: string, ): Promise => { - const startDate = toDateString(startIso, tz); - const endDate = toDateString(endIso, tz); + const bufferedStart = new Date( + new Date(startIso).getTime() - buffer * 60_000, + ); + const bufferedEnd = new Date(new Date(endIso).getTime() + buffer * 60_000); + // Query every local day touched by the same padded interval used for the + // overlap check. Using the unbuffered dates misses adjacent rows across + // local midnight that conflict only because of the configured buffer. + const startDate = toDateString(bufferedStart.toISOString(), tz); + const endDate = toDateString(bufferedEnd.toISOString(), tz); const occupied = await loadOccupiedWith( d, startDate, endDate, excludeBookingId, - excludeHoldId, + excludeHoldId + ? { + id: excludeHoldId, + datetime: new Date(startIso).toISOString(), + duration: + (new Date(endIso).getTime() - new Date(startIso).getTime()) / + 60_000, + } + : undefined, ); - const bufferedStart = new Date(new Date(startIso).getTime() - buffer * 60_000); - const bufferedEnd = new Date(new Date(endIso).getTime() + buffer * 60_000); if (hasOverlap(bufferedStart, bufferedEnd, occupied)) { domainError( Errors.reservation( @@ -934,20 +964,44 @@ export const createHomegrownAdapter = ( fromAsync(async () => { const { slotReservations } = (await loadSchemas()).booking; const expirationMinutes = params.expirationMinutes ?? 10; - const expiresAt = new Date( - Date.now() + expirationMinutes * 60_000, - ).toISOString(); - - const [row] = await withDb((d) => - d + const start = new Date(params.datetime); + const end = new Date(start.getTime() + params.duration * 60_000); + // Homegrown booking writes currently resolve the configured default + // practitioner, so use that same row id for the lock even when a + // caller supplied provider metadata on the advisory hold. + const lockResource = transactional + ? ((await getDefaultPractitioner())?.id ?? "default") + : "default"; + + const row = await runInTransaction(async (d) => { + // A hold is the pre-payment admission gate. Serialize it with other + // holds and booking writes for the same practitioner-day so two + // checkouts cannot both acquire a hold and charge for one slot. + if (transactional) { + await acquireSlotLocks( + d, + lockResource, + start.toISOString(), + end.toISOString(), + ); + } + await assertSlotOpen(d, start.toISOString(), end.toISOString()); + // Start the hold lifetime only after any lock contention and the + // final availability check. Otherwise a blocked checkout could + // insert a hold that is already expired when it is returned. + const expiresAt = new Date( + Date.now() + expirationMinutes * 60_000, + ).toISOString(); + const [inserted] = await d .insert(slotReservations) .values({ - datetime: params.datetime, + datetime: start.toISOString(), duration: params.duration, expiresAt, }) - .returning(), - ); + .returning(); + return inserted; + }); return { id: row.id, diff --git a/src/core/pipelines.ts b/src/core/pipelines.ts index 80c850a..76e4795 100644 --- a/src/core/pipelines.ts +++ b/src/core/pipelines.ts @@ -42,7 +42,6 @@ const BookingRequestSchema = z.object({ client: ClientInfoSchema, paymentMethod: z.string().optional(), idempotencyKey: z.string().min(1), - softHoldId: z.string().optional(), }); // ============================================================================= @@ -129,7 +128,11 @@ export const completeBookingWithAltPayment = ( notes: `Payment pending: ${request.idempotencyKey}`, }), Effect.map((r) => r as SlotSoftHold | undefined), - Effect.catchAll(() => Effect.succeed(undefined as SlotSoftHold | undefined)), + Effect.catchAll((error) => + error._tag === 'ReservationError' && error.code === 'SLOT_TAKEN' + ? Effect.fail(error) + : Effect.succeed(undefined as SlotSoftHold | undefined), + ), ); // Phase C: Process payment (release soft hold on failure) @@ -158,9 +161,14 @@ export const completeBookingWithAltPayment = ( // Phase D: Create booking (refund + release soft hold on failure). // Thread the caller's own hold id so the write-time slot gate does not // count the Phase B hold as a conflict and fail every held booking. + // softHoldId is internal pipeline metadata. Discard any same-named value + // supplied by a caller and inject only the hold acquired in Phase B. + const { softHoldId: _callerSoftHoldId, ...bookingRequest } = request; + const requestWithHold: BookingRequest = + softHold ? { ...bookingRequest, softHoldId: softHold.id } : bookingRequest; const booking = yield* pipe( scheduler.createBookingWithPaymentRef( - softHold ? { ...request, softHoldId: softHold.id } : request, + requestWithHold, payment.transactionId, paymentAdapter.name, ), diff --git a/src/core/types.ts b/src/core/types.ts index fab5027..3aec6fc 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -215,9 +215,11 @@ export interface BookingRequest { readonly paymentMethod?: string; readonly idempotencyKey: string; /** - * Id of the caller's own advisory soft hold for this slot, when one was - * placed earlier in the flow. The write-time availability gate excludes it - * from conflicts so the caller's own hold cannot fail the booking. + * Opaque server-side capability returned by softHoldSlot for this booking. + * Pass it only when directly composing an adapter's hold and booking methods; + * never populate it from an untrusted client payload. The standard checkout + * pipeline ignores caller-supplied values and injects only the hold it + * acquired itself. */ readonly softHoldId?: string; } diff --git a/src/tests/unit/core/pipelines.test.ts b/src/tests/unit/core/pipelines.test.ts index 1f2c154..cfe718e 100644 --- a/src/tests/unit/core/pipelines.test.ts +++ b/src/tests/unit/core/pipelines.test.ts @@ -245,6 +245,51 @@ describe('completeBookingWithAltPayment', () => { } }); + it('stops before payment when atomic soft-hold acquisition reports SLOT_TAKEN', async () => { + vi.mocked(scheduler.softHoldSlot).mockReturnValue( + Effect.fail( + Errors.reservation( + 'SLOT_TAKEN', + 'Another checkout already holds this slot', + input.request.datetime, + ), + ), + ); + + const error = await expectFailureTag( + completeBookingWithAltPayment(ctx, input), + 'ReservationError', + ); + + expect(error).toMatchObject({ code: 'SLOT_TAKEN' }); + expect(paymentAdapter.createIntent).not.toHaveBeenCalled(); + expect(paymentAdapter.capturePayment).not.toHaveBeenCalled(); + expect(scheduler.createBookingWithPaymentRef).not.toHaveBeenCalled(); + }); + + it('injects only the hold acquired by the pipeline into the booking write', async () => { + const requestWithForeignHold = { + ...input.request, + softHoldId: 'caller-supplied-hold', + }; + + await expectSuccess( + completeBookingWithAltPayment(ctx, { + ...input, + request: requestWithForeignHold, + }), + ); + + expect(scheduler.createBookingWithPaymentRef).toHaveBeenCalledWith( + expect.objectContaining({ softHoldId: '99999' }), + expect.any(String), + expect.any(String), + ); + const bookingRequest = vi.mocked(scheduler.createBookingWithPaymentRef) + .mock.calls[0][0] as typeof requestWithForeignHold; + expect(bookingRequest.softHoldId).not.toBe('caller-supplied-hold'); + }); + it('releases soft hold on payment intent failure', async () => { vi.mocked(paymentAdapter.createIntent).mockReturnValue( Effect.fail(Errors.payment('INTENT_FAILED', 'Failed to create intent', 'cash')) diff --git a/tests/integration/homegrown-concurrency.test.ts b/tests/integration/homegrown-concurrency.test.ts index b92dbfd..af5e2cd 100644 --- a/tests/integration/homegrown-concurrency.test.ts +++ b/tests/integration/homegrown-concurrency.test.ts @@ -27,7 +27,7 @@ */ import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest'; -import { Effect } from 'effect'; +import { Cause, Effect, Option } from 'effect'; import { drizzle } from 'drizzle-orm/node-postgres'; import { sql } from 'drizzle-orm'; import { @@ -256,10 +256,10 @@ suite('HomegrownAdapter concurrency against real Postgres', () => { const SLOT = '2026-04-20T14:00:00.000Z'; // Far-future fixed Monday 10:00 America/New_York (14:00Z in EDT), so the // pipeline's business-hours + min-advance checks pass deterministically. - const PIPELINE_SLOT = '2027-04-19T14:00:00.000Z'; + const PIPELINE_SLOT = '2099-04-20T14:00:00.000Z'; // Starts 60 min into PIPELINE_SLOT's 120-min interval: overlapping but a // DIFFERENT start time, so exact-start lock keys would not contend. - const OVERLAP_SLOT = '2027-04-19T15:00:00.000Z'; + const OVERLAP_SLOT = '2099-04-20T15:00:00.000Z'; const HANDLE = 'alex'; const buildAdapter = () => @@ -398,6 +398,37 @@ suite('HomegrownAdapter concurrency against real Postgres', () => { expect(rows).toHaveLength(1); }, 30000); + it('lets exactly one of two concurrent soft holds for the same slot win before payment', async () => { + const adapter = buildAdapter(); + const hold = () => + adapter.softHoldSlot({ + serviceId, + datetime: PIPELINE_SLOT, + duration: 60, + }); + + const exits = await Promise.all([ + Effect.runPromiseExit(hold()), + Effect.runPromiseExit(hold()), + ]); + + expect(exits.filter((exit) => exit._tag === 'Success')).toHaveLength(1); + const failures = exits.filter((exit) => exit._tag === 'Failure'); + expect(failures).toHaveLength(1); + const failure = Cause.failureOption(failures[0].cause); + expect(Option.isSome(failure)).toBe(true); + expect(Option.getOrThrow(failure)).toMatchObject({ + _tag: 'ReservationError', + code: 'SLOT_TAKEN', + }); + + const rows = await db + .select({ id: slotReservations.id }) + .from(slotReservations) + .where(sql`${slotReservations.datetime} = ${PIPELINE_SLOT}`); + expect(rows).toHaveLength(1); + }, 30000); + // TIN-2764 regression (revenue path): the checkout pipeline places its own // advisory soft hold in Phase B, charges in Phase C, and creates the booking // in Phase D. Pre-fix, Phase D's write-time gate counted the caller's OWN @@ -529,7 +560,7 @@ suite('HomegrownAdapter concurrency against real Postgres', () => { .select({ id: bookings.id }) .from(bookings) .where( - sql`${bookings.datetime} < '2027-04-19T16:00:00.000Z' AND ${bookings.endTime} > '2027-04-19T14:00:00.000Z'`, + sql`${bookings.datetime} < '2099-04-20T16:00:00.000Z' AND ${bookings.endTime} > '2099-04-20T14:00:00.000Z'`, ); expect(rows).toHaveLength(1); }, 30000);