fix(booking): the cancel and reschedule routes move status through the CAS helpers, and cancelled slots are tombstoned - #1383
Merged
Conversation
The notification block landed unformatted, so the file fails `prettier --check` on dev and every later diff carries reflow noise. Format-only; no behaviour change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgadJydWEKkdhhzY58yiL7
…CAS helpers A successful cancel wrote no BookingStatusHistory row and left every cancelled slot with `deletedAt: null`, which is why that table is empty on production and why cancelled sessions still read as occupied to every caller that filters on the tombstone. Between them the two routes and `reschedule-withdraw` carried 17 raw status writes (8/8/1): each re-checked the status in its own WHERE, so the compare-and-set was sound, but none of them appended the audit row or released the calendar. Every one now goes through `lib/booking/transitions.ts` on the same `tx`. The WHERE guards survive as `fromIn` plus their non-status predicates, the sweeps that legitimately match nothing pass `allowZero`, and each route maps `IllegalTransitionError` back to the code it has always answered — 409 NOT_CANCELLABLE and 409 NOT_RESCHEDULABLE — so no client contract moves. Only the cancel path tombstones: a rescheduled slot is kept and restored in place by a withdrawal, so it must not be retired. `transitionRescheduleRequest` CASes one row by id, so the cancel reads its open proposals inside the tx and moves each; a proposal answered concurrently by the expiry cron, which holds no appointment lock, leaves the booking in the state this step wanted anyway. The slot sweeps pass no appointmentId, so #1333's per-row resolution files each history row against the appointment the slot actually sits on rather than the one the request came in on. The pin extends the suite that already drives this route. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgadJydWEKkdhhzY58yiL7
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MgadJydWEKkdhhzY58yiL7
✅ Deploy Preview for familiarise ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 90 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (11)
Comment |
This was referenced Sep 5, 2026
…as-helpers # Conflicts: # docs/booking/05-troubleshooting-and-changelog.md
…weep scope serves both sweeps (#1383) Behaviour-identical shape-only change to clear the two Sonar issues this PR's new code raised on `app/api/appointments/[appointmentId]/cancel/route.ts`. typescript:S3776 — the `prisma.$transaction` callback scored a cognitive complexity of 20 against a limit of 15. Two blocks move out to module scope, where their own branching no longer counts against the callback: the reschedule-proposal decline loop becomes `declineOpenReschedules`, and the "which rows does this cancel sweep" decision becomes `cancelSweepScope`. The callback now scores 8. typescript:S3358 — the nested ternary choosing the slot sweep's WHERE is gone. It was written out twice, once for `transitionSlotCompletion` and once for `setParticipantStatus`, and the two had to stay in step; `cancelSweepScope` is now the single answer and both sweeps read it. The scope is resolved before the transaction opens, off the same pre-transaction `appointment` row the ternaries read, so the value is identical. No status write, WHERE clause, CAS from-set, status code, `code` string or response body changes. The gate itself is already OK on this head — the duplicated blocks Sonar lists for the reschedule route, the cancel route and `scripts/appointments/send-appointment-reminders.ts` are all present verbatim on `origin/dev`, so after the dev merge they are no longer new code and `new_duplicated_lines_density` reads 0.0%. Verification: cold `tsc --noEmit` clean, `eslint` 0 problems, `prettier --check` clean, and 78 jest suites / 1251 tests pass across the cancel-refund, refund-preview-parity, booking-algorithm and maintenance suites. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EyngsXG829TRTBof4CSGT1
Contributor
Author
Sonar pass on this PR's new codeThe quality gate was already
What this commit does fix is the two open issues that genuinely are this PR's new code, both in
Shape only: no status write, WHERE clause, CAS from-set, HTTP status, Verification — cold 🤖 Generated with Claude Code |
|
teetangh
added a commit
that referenced
this pull request
Sep 5, 2026
…l it is confirming, and accept serialises on the appointment lock (#1340) (#1515) ## Summary Issue #1340 is titled as a double-booking race in the reschedule auto-confirm path. That race cannot open: `manualAllocate` writes every slot inside one `prisma.$transaction` under `lockAutoAllocate` plus the consultee lock, with the `slot_no_confirmed_overlap` GiST constraint as the backstop, and `openForAppointmentId` cannot dangle through this path because `DECLINED` is terminal and `transitionRescheduleRequest` clears the reservation on every terminal target. What is live at those exact lines is a different and worse defect, and this PR fixes that. **The real defect.** `SlotAllocationService.resolveConsumedPreferenceRequests` ends every allocating transaction with a supersede sweep that `DECLINE`s each open `RescheduleRequest` whose `releasedSlotIds` intersect the slots being placed. That sweep is right for a consultant who answers a proposal by placing *different* times by hand. It was wrong for the two confirmation callers, which place the proposal's *own* times: - `lib/booking/reschedule-auto-confirm.ts:68` and `lib/booking/reschedule-respond.ts:77` both call `SlotAllocationService.allocate({ mode: "manual", slots: <the proposed times>, wideLock: true })`, and only afterwards CAS the proposal `PENDING_REVIEW → AUTO_ACCEPTED` / `→ ACCEPTED`. - By the time that CAS runs, the sweep has already moved the row to `DECLINED` inside the allocator's transaction, so the `updateMany` matches zero rows and `transitionRescheduleRequest` throws `IllegalTransitionError`. - The booking has moved either way. `reschedule/route.ts:~727` swallows the error and answers `autoConfirmed: false` to a consultee whose session was in fact rescheduled; the explicit accept rethrows, so the consultee gets a **409 on a booking that moved**, and the MOVED notification in `reschedule-respond.ts` is never reached. The audit trail records a refusal for a request that was granted. **The fix.** `AllocationRequest` gains an opt-in `excludeRescheduleRequestId`, threaded through `dispatch` into `manualAllocate` and `autoAllocate` and down to the sweep, whose supersede `WHERE` gains `...(excludeId ? { id: { not: excludeId } } : {})`. Both confirmation callers pass their own proposal id. Nothing else in the allocator moves: locks, TTLs and transaction boundaries are untouched, and an allocation that is not confirming a specific proposal still supersedes every open proposal on those slots exactly as before. **Secondary (the same issue's real concurrency gap).** The accept path took no appointment lock at all. Accept is a lifecycle mutation that moves this appointment's slots, but the allocator's locks are keyed by consultant and by consultee, never by appointment, so an accept and a concurrent cancel of the same booking never contended for anything. `respond/route.ts` now wraps `acceptProposal` in `withAppointmentLock`, the same `appointment-lock:` atom the cancel and reschedule routes take, and maps `AppointmentBusyError` to 423 and `BookingLockUnavailableError` to 503 exactly as `reschedule/route.ts` does. Lock order is unchanged — the appointment atom is the coarsest key and is taken before the allocator acquires its own. Decline is deliberately left outside the lock: it moves nothing. ## Files touched | File | Change | |---|---| | `utils/slotAllocation/types.ts` | `AllocationRequest.excludeRescheduleRequestId?: string` with the why-comment. | | `utils/slotAllocation/SlotAllocationService.ts` | Threads the id through `dispatch` → `autoAllocate` / `manualAllocate` → `resolveConsumedPreferenceRequests`; the supersede `WHERE` gains `id: { not: … }`. | | `lib/booking/reschedule-auto-confirm.ts` | Passes `excludeRescheduleRequestId: request.id`. | | `lib/booking/reschedule-respond.ts` | Passes `excludeRescheduleRequestId: request.id`. | | `app/api/appointments/[appointmentId]/reschedule/respond/route.ts` | Accept runs inside `withAppointmentLock`; 423 / 503 lock answers. | | `__tests__/booking-algorithm/reschedule-respond.test.ts` | The pin's caller half (+ the lock case). | | `__tests__/booking-algorithm/preference-scored-allocation.test.ts` | The pin's allocator half. | | `docs/booking/07-rescheduling-flow.md`, `docs/booking/18-state-machines.md` | The confirmation sequence and the exclusion rule. | ## Verification | Check | Result | |---|---| | `npx prettier --check` on every changed file | The five code files and `reschedule-respond.test.ts` are clean. `lib/booking/reschedule-respond.ts`, `preference-scored-allocation.test.ts` and both docs still warn, and every remaining hunk is byte-identical to `origin/dev` (pre-existing wrapping in the big `select` payloads, the two markdown tables and unrelated object literals); no added line is unformatted. | | `npx eslint` on the changed files | 0 errors, 3 warnings — all three are the pre-existing `no-explicit-any` at `SlotAllocationService.ts` dev-lines 3446/3499 and `types.ts` dev-line 147, shifted by this diff and otherwise identical. | | Cold `tsc --noEmit` (`tsconfig.tsbuildinfo` deleted, 8 GB heap) | exit 0, after the rebase onto `73c0d9f2c`. | | `npx jest __tests__/booking-algorithm __tests__/booking __tests__/payments/cancel-route-refund.test.ts` | exit 0 — **72 suites, 1132 tests passed**, re-run after the rebase. | **The pin fails without the fix.** Reverting the five production files to `origin/dev` and re-running the two suites fails 5 assertions: ``` ● #1340 — resolveConsumedPreferenceRequests and the confirming proposal › declines a different stale proposal on the same slots but never the excluded one ● accept re-validates through the allocator before anything is written › sends the proposed times through manual allocation under the wide lock ● #1340 — a confirmation keeps the proposal it is confirming › accept names its own proposal to the allocator and then closes it ACCEPTED ● #1340 — a confirmation keeps the proposal it is confirming › auto-confirm names its own proposal to the allocator and then closes it AUTO_ACCEPTED ● the respond route drives the loop for the counterparty › serializes the accept on the appointment lock and answers 423 while it is held Tests: 5 failed, 55 passed, 60 total ``` Reverting only the supersede `WHERE` (the one-line heart of the fix) fails the allocator case on its own. That case drives the real `resolveConsumedPreferenceRequests` against a transaction stub that answers `findMany` the way Postgres would, so the assertion rides on the `WHERE` the sweep actually issues rather than on a stubbed outcome: with an exclusion in flight only the *other* stale proposal is declined, and with no exclusion both still are. ## Not done - `app/api/appointments/[appointmentId]/reschedule/route.ts` is untouched: PR #1383 owns that file. Its auto-confirm call at `:~717` therefore still runs **outside** `withAppointmentLock`, so the lock-scope half of the auto-confirm path stays as it is. Worth a follow-up once #1383 lands; the correctness fix above does not depend on it. - The #1497 limiter rider was not taken. - No `db push`, no schema change, no notification-workflow change. Closes #1340 Part of #1433 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01EyngsXG829TRTBof4CSGT1
teetangh
added a commit
that referenced
this pull request
Sep 5, 2026
…rg tiers, and a credit-funded partial cancel restores the credit in full (#1499, #1500, #1372) (#1513) ## Summary **The model (#1499).** Refund terms move out of the `Appointment.cancellationPolicySnapshot` Json column and into typed, versioned rows. `CancellationPolicy` is one published version of a ladder — its scope, version, status, and the percentage a consultant-initiated cancellation settles at — and `CancellationPolicyTier` holds its rungs. `Appointment.cancellationPolicyId` points at the exact row that governed the sale. A published version is **immutable**: editing a ladder archives the current `ACTIVE` row and inserts a new one at the next version, so the freeze the old snapshot provided by convention is now structural, and no code path can rewrite terms a buyer already agreed to. **Resolution.** Checkout resolves the governing version exactly once, inside the booking transaction, through `resolveCheckoutCancellationPolicyId()`. An organisation's ladder governs the bookings the organisation **funds** — on a refund it is the organisation's money that moves — so the organisation id is passed only on the sponsored path; a personal booking merely tagged to an organisation keeps the platform ladder, as does an organisation that has never published. Sessions allocated later against a subscription **inherit** the version from the row checkout created rather than resolving afresh, which is the whole point of versioning. The platform default lives at a fixed id, is seeded, and is also provisioned idempotently by `ensurePlatformCancellationPolicy()` so a database nobody seeded cannot fail a checkout. **The credit rule (#1500).** The credits rail cannot pay a fraction — `refundBookingPayment` refuses an `amountPaise` on a `free_` intent — so a partial tier previously had nothing it could pay and escalated to a human. That escalation is replaced by a rule with two halves: - **Any tier above 0% restores the credit IN FULL**, via `refundBookingPayment` with no `amountPaise`. The buyer gave the notice the ladder rewards, and the rail's inability to divide should not cost them the refund. - **A 0% tier restores NOTHING**, falling through to the existing `POLICY_ZERO` arm. A late cancel bites a credit buyer exactly as it bites a card buyer; paying a full credit back for a cancellation that earns a card buyer nothing would make free credit strictly better than money and delete the late-cancel deterrent. The whole rule is one predicate in `quoteBookingRefund` — `isFreeCreditFunded && refundPct > 0` — surfaced as `creditRestoresInFull`. `MANUAL_REVIEW` is gone from the route, the response union, the client and the docs. ## Schema Additive only. - **`CancellationPolicy`** — `organizationId` (null = the platform default), `version`, `status` (`CancellationPolicyStatus`: `ACTIVE` | `ARCHIVED`), `consultantInitiatedBps`, `policyText`, `publishedByUserId`, `createdAt`, `archivedAt`. `@@unique([organizationId, version])`, `@@index([organizationId, status])`. - **`CancellationPolicyTier`** — `policyId`, `hoursBefore`, `refundBps`. `@@unique([policyId, hoursBefore])`, `@@index([policyId])`. - **`Appointment.cancellationPolicyId String?`** — FK at `onDelete: SetNull`, plus `@@index([cancellationPolicyId])`. `SetNull` because a booking must survive its organisation being torn down; losing the pointer degrades to the platform ladder rather than deleting money history. Null and "sold before this change" are deliberately indistinguishable. - **`Appointment.cancellationPolicySnapshot Json?`** stays, **frozen**: never written, never read, annotated as such, dropped at the pre-MVP reset. A column a running deploy still reads must not be dropped under it, and this repo writes no backfill migrations. - Back-relations on `Organization` and `User`; enums declared below the models that use them. - **Staged, commented** in `prisma/sql/check-constraints.sql`: the `NULLS NOT DISTINCT` partial unique for "one ACTIVE version per scope" and for `(organizationId, version)`. Postgres treats null keys as distinct, so the platform row escapes the Prisma `@@unique` entirely. These stay commented because `check-db-sidecars` strips comments and would demand an unapplied index. Until they land, one active version per scope is enforced by the Serializable rotation in `publishOrgCancellationPolicy()`, and readers order `version desc` and take one so a slip degrades to "newest wins". **This PR requires `npm run db:push` by the orchestrator after merge**, followed by `npm run db:assert-sidecars`. CI's `check-db-drift` skips new enums and tables, so CI is green before the push; the seed and manual QA need the push first. ## Files touched **Schema and seed** — `prisma/schema.prisma`, `prisma/sql/check-constraints.sql`, `prisma/seed.ts`, `prisma/seedFiles/16b-create-cancellation-policy.ts` (new). **Money core** — `lib/payments/operations/cancellation-policy.ts` (stays Prisma-free: `RefundTier`, `CancellationPolicyTerms`, `PLATFORM_DEFAULT_TERMS`, `tiersFromBps`, `validateTierLadder`, `computeRefundPct`, `quoteBookingRefund`), `lib/payments/operations/cancellation-policy-store.ts` (new: the one select shape, the platform provisioner, the resolver, the publish routine), `lib/payments/operations/checkout.ts`, `utils/slotAllocation/SlotAllocationService.ts`. **The six readers** — `lib/booking/cancellation-scope.ts`, `lib/booking/rejection-refund.ts`, `lib/trials/cancellation.ts`, `lib/payments/operations/event-refunds.ts`, `lib/support/context.ts`, and the two cancel routes (`app/api/appointments/[appointmentId]/cancel/route.ts`, `.../cancel/preview/route.ts`). `parsePolicySnapshot` and `resolveCancellationPolicySnapshot` are deleted; no reader consults the Json column. **Org editor** — `app/api/organizations/[orgId]/cancellation-policy/route.ts` (new; GET gated on `settings.manage`, PUT on `minimumRole: "OWNER"`, no PATCH or DELETE because a version is immutable), `app/dashboard/organization/[orgId]/settings/CancellationPolicyCard.tsx` (new) wired from `GeneralPanel.tsx` behind `isAtLeast("OWNER")`, `lib/enterprise/audit-actions.ts`. **Client** — `components/appointments/consultee/useEventActions.ts`. **Tests** — `__tests__/enterprise/multi-engagement-cap.test.ts` (#1372's three cases, in the existing file), `__tests__/booking/cancellation-policy.test.ts`, `__tests__/payments/{cancel-route-refund,refund-preview-parity,attendee-removal-refund,trial-cancellation-refund,rejection-refund,consultation-atom-parity,checkout-open-order-reuse,allocation-utilization-integrity}.test.ts`, `__tests__/booking-algorithm/{cancellation-scope,allocation-top-up,row-walk-truncation,collaborator-availability-modes,slotAllocationService}.test.ts`. **Docs** — `docs/booking/08-cancellation-flow.md`, `docs/booking/17-org-funded-checkout.md`, `docs/enterprise/70-design-decisions/28-typed-versioned-cancellation-policy.md` (new) + its index row in `00-README.md`, `.claude/skills/booking/references/money-boundary.md`. ## Verification | Check | Result | | --- | --- | | `npx prisma generate` then cold `npx tsc --noEmit` (`tsconfig.tsbuildinfo` deleted, 8 GB heap) | **exit 0**, no errors | | `npx eslint` on all 33 changed code files | **0 errors, 0 warnings introduced**. 1 error + 44 warnings remain, all byte-identical at `origin/dev`: `jest/no-mocks-import` and 32 `no-explicit-any` in `slotAllocationService.test.ts`, 9 `no-explicit-any` in `checkout-open-order-reuse.test.ts`, 1 `eqeqeq` in `checkout.ts`, 2 `no-explicit-any` in `SlotAllocationService.ts`. The other 29 files are clean. | | `npx prettier --check` on every changed file | clean. The 10 files my edits drifted were verified clean at baseline first, then formatted. | | `npx prisma format` | no change; schema already formatted | | `npx jest __tests__/booking __tests__/payments __tests__/enterprise __tests__/booking-algorithm/cancellation-scope.test.ts` | **exit 0** — 227 suites passed, 2440 tests passed | The five allocation suites initially failed (60 tests) because `createAppointments` now reads `tx.appointment.findFirst` to inherit the policy and their transaction mocks did not define it. Fixed by adding `findFirst: jest.fn().mockResolvedValue(null)` to those five mocks — mock plumbing, not a semantic change. ## Limitations / not done - **Org-funded event seats fall back to the platform ladder.** One shared `Appointment` row serves every registrant of a webinar or class, across every funding organisation, so it cannot carry one buyer's terms; its FK stays null. Whole-event refunds already assumed the platform ladder, so this is consistent, but organisation tiers genuinely do not reach event seats. Reaching them means moving the terms onto the participant row — worth a follow-up issue if it is wanted. - **A `free_` intent with a non-zero amount is out of scope.** That is a mixed payment; it takes the money arm and is refused `INVALID_AMOUNT`, exactly as today. Both halves of the `isFreeCreditFunded` predicate are load-bearing for this reason. - **Merge order: land #1383 first.** It rewrites the status writes in the cancel transaction body (~`:39-447`). This PR touches only the import block and the post-transaction refund block, so expect at most one import-block conflict. - **`docs/booking/05-troubleshooting-and-changelog.md` is deliberately untouched.** Its line about "the only surviving MANUAL_REVIEW path" is now stale and is amended by PR-D's consolidated changelog pass. - No end-to-end run: that is the orchestrator-announced round, and it needs the schema pushed first. Closes #1499 Closes #1500 Closes #1372 Part of #1503 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01EyngsXG829TRTBof4CSGT1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Wave-6 PR E (umbrella #1319). Found by the 2026-09-03 production cleanup: a user cancel succeeded and wrote no
BookingStatusHistoryrow, and the cancelled slots keptdeletedAt = null. The two most-used lifecycle routes were moving request, slot and reschedule status with rawupdateManycalls (status re-checked in the WHERE, so a CAS, but outside the helpers), which is whyBookingStatusHistorywas empty on production.transitionConsultationRequest/transitionSubscriptionRequest/transitionWebinarEvent/transitionClassEventwith the route's own from-sets asfromInand the session user as actor;IllegalTransitionErrormaps to the unchanged 409NOT_CANCELLABLE. The three slot writes collapse to onetransitionSlotCompletionto CANCELLED withdeletedAt, scoped throughfromIn(nevercompletionStatusin thewhere). The open reschedule proposal is read in the transaction and each is CAS-closed to DECLINED.transitionSlotCompletionto RESCHEDULED withisTentative: trueand deliberately nodeletedAt(rule 2: a reschedule keeps its rows so a withdrawal can restore them); the four request/event writes go through the helpers; the shared catch answers the unchanged 409NOT_RESCHEDULABLE.proposedSlotsandrescheduleRequest.createare untouched.reschedule-withdraw.ts. Its one raw write (RESCHEDULED → SCHEDULED) is now the helper withallowZero, which is load-bearing there; its existing helper calls gain the actor.appointmentIdon purpose: a whole-subscription cancel would otherwise file sibling appointments' slot history under the appointment the request arrived on; the request/event transitions pass it explicitly (the override fix(booking): every status history row names its appointment, and creation is a row #1378 documents).Raw status writes: cancel route 8 → 0, reschedule route 8 → 0, withdraw 1 → 0.
lib/booking/transitions.tsis untouched.Verification
Cold
tscclean;__tests__/booking-algorithm+__tests__/booking+__tests__/payments+__tests__/maintenance: 129 suites / 1,724 tests green; prettier clean; eslint per-file counts identical todev. Pin extended in the existing__tests__/payments/cancel-route-refund.test.ts: a successful consultation cancel writes one CONSULTATION history row (actor, reason, appointment id) plus one per moved slot withdeletedAt; a lost CAS still answers 409 with zero history rows. Both fail on pre-change code.Rebased onto
devat b713e48 (#1378 included); five route assertions loosened toobjectContainingbecause #1378 changed the helper'sselect. Commit 1 is format-only (reschedule-withdraw.tswas prettier-dirty ondev).Out of scope, found on the way
transitionSlotCompletionappends history rows in a sequential loop, one INSERT per slot inside the transaction; a whole-class cancel with many attendees would benefit from acreateMany. Left for a follow-up on the helper file.Part of #1319.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MgadJydWEKkdhhzY58yiL7