Skip to content

fix(homegrown): close post-merge soft-hold races (TIN-2764) #119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jess Sullivan (Jesssullivan) merged 1 commit into from
Jul 11, 2026
Merged

fix(homegrown): close post-merge soft-hold races (TIN-2764) #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions docs/parity-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)* |

Expand All @@ -93,16 +93,17 @@ 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…')`
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
Expand Down
180 changes: 163 additions & 17 deletions src/adapters/__tests__/homegrown-adapter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
}));

// ---------------------------------------------------------------------------
Expand All @@ -107,7 +112,8 @@ type MockRow = Record<string, unknown>;
* `.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 =
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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" });
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand Down
Loading
Loading