diff --git a/apps/edr-passenger-api/src/modules/payments/payment-client.service.ts b/apps/edr-passenger-api/src/modules/payments/payment-client.service.ts index e723dc8ce..bf6c86fea 100644 --- a/apps/edr-passenger-api/src/modules/payments/payment-client.service.ts +++ b/apps/edr-passenger-api/src/modules/payments/payment-client.service.ts @@ -22,6 +22,13 @@ export interface PaymentDiagnostic { provider: ProviderStatus | null; } +/** + * Why a settlement check came back `unverifiable` (mirrors the payment service's + * ReconcileUnverifiableReason). `IN_FLIGHT` means money is actually moving and must be waited out; + * `PROVIDER_ERROR` can be a permanently unreachable gateway, which a sweep may eventually give up on. + */ +export type SettlementUnverifiableReason = "IN_FLIGHT" | "PROVIDER_ERROR"; + /** Settlement check from POST /payments/reconcile (verify-before-cancel). */ export interface SettlementResult { /** At least one intent for the order is paid (incl. a late capture just registered). */ @@ -31,6 +38,8 @@ export interface SettlementResult { /** Settlement could not be confirmed — a provider query errored, a payment is in flight, OR the * payment service was unreachable. The caller MUST NOT cancel the order. */ unverifiable: boolean; + /** Set whenever `unverifiable` — which of the two causes applies. */ + reason?: SettlementUnverifiableReason; } /** @@ -117,7 +126,9 @@ export class PaymentClientService { this.logger.warn( `reconcile ${referenceType}/${referenceId} failed: ${message}; treating as unverifiable (will not cancel)`, ); - return { paid: false, unverifiable: true }; + // The payment service itself is unreachable — indistinguishable from a dead gateway, and + // like one it may never recover, so it is a PROVIDER_ERROR (give-up-able), not IN_FLIGHT. + return { paid: false, unverifiable: true, reason: "PROVIDER_ERROR" }; } } @@ -173,9 +184,6 @@ export class PaymentClientService { body?: unknown, ): Promise { const url = `${this.baseUrl}${path}`; - this.logger.log("====================================================================="); - this.logger.log(`URL ${url}`); - this.logger.log("====================================================================="); try { const response = await firstValueFrom( this.http.request({ diff --git a/apps/edr-passenger-api/src/modules/payments/payments.service.ts b/apps/edr-passenger-api/src/modules/payments/payments.service.ts index 137df45ee..5cd483830 100644 --- a/apps/edr-passenger-api/src/modules/payments/payments.service.ts +++ b/apps/edr-passenger-api/src/modules/payments/payments.service.ts @@ -39,6 +39,7 @@ import { import { PaymentClientService, PaymentDiagnostic, + SettlementUnverifiableReason, } from "./payment-client.service"; import { CurrencyService } from "../currency/currency.service"; import { AuditService } from "../../common/audit.service"; @@ -1116,10 +1117,17 @@ export class PaymentsService { * - not paid → verified unpaid; a cancellation caller may proceed. * - unverifiable (provider query errored, in-flight, or payment service unreachable) → a * cancellation caller must NOT cancel this cycle; defer and retry later. + * + * When unverifiable, `reason` says WHY, and the two are not interchangeable: `IN_FLIGHT` is a + * payment actually moving (defer forever — this is the case the guard exists for), while + * `PROVIDER_ERROR` may be a gateway that never comes back, which a sweep is allowed to give up + * on after a grace window rather than retry once a minute in perpetuity. */ - async reconcileAndConfirmIfPaid( - bookingId: string, - ): Promise<{ paid: boolean; verified: boolean }> { + async reconcileAndConfirmIfPaid(bookingId: string): Promise<{ + paid: boolean; + verified: boolean; + reason?: SettlementUnverifiableReason; + }> { const current = await this.prisma.booking.findUnique({ where: { id: bookingId }, select: { status: true }, @@ -1134,10 +1142,11 @@ export class PaymentsService { ); if (settlement.unverifiable) { + const reason = settlement.reason ?? "PROVIDER_ERROR"; this.logger.warn( - `reconcile-before-cancel: settlement UNVERIFIABLE for booking ${bookingId} — not cancelling`, + `reconcile-before-cancel: settlement UNVERIFIABLE (${reason}) for booking ${bookingId} — not cancelling`, ); - return { paid: false, verified: false }; + return { paid: false, verified: false, reason }; } if (settlement.paid) { diff --git a/apps/edr-passenger-api/src/modules/tasks/tasks.service.ts b/apps/edr-passenger-api/src/modules/tasks/tasks.service.ts index 284eb61c0..fe4207a50 100644 --- a/apps/edr-passenger-api/src/modules/tasks/tasks.service.ts +++ b/apps/edr-passenger-api/src/modules/tasks/tasks.service.ts @@ -14,6 +14,15 @@ const AUDIT_LOG_RETENTION_DAYS = 365; const WEBHOOK_EVENT_RETENTION_DAYS = 90; const GATE_LOG_RETENTION_DAYS = 180; +// How long past its payment deadline a booking may sit undecided because the GATEWAY cannot be +// reached (PROVIDER_ERROR) before the sweep stops deferring and cancels anyway. Without a bound, +// a permanently unreachable provider pins a booking as PENDING_PAYMENT forever — its seats stay +// held and the sweep re-queries it once a minute, indefinitely. NEVER applied to an IN_FLIGHT +// settlement: money that is actually moving is waited out no matter how long it takes. +// Raise this in production — a 10-minute gateway outage should not mass-cancel bookings that may +// well be paid (a late payment then lands on a CANCELLED booking and needs a manual refund). +const RECONCILE_GRACE_MINUTES = Number(process.env.RECONCILE_GRACE_MINUTES) || 5; + function fmtTime(d: Date): string { return d.toLocaleTimeString('en-GB', { hour: '2-digit', @@ -325,15 +334,38 @@ export class TasksService { // Deadline passed — but NEVER cancel a booking that is actually paid. The payment.succeeded // event may have been lost (RabbitMQ down) or arrived late, leaving a paid booking stuck // PENDING_PAYMENT. Ask the payment service over HTTP; it confirms the booking synchronously - // if paid. Only proceed to cancel when settlement is VERIFIED unpaid. + // if paid. Cancel only on a VERIFIED-unpaid settlement — or, past the grace window below, + // on a settlement the gateway simply refuses to answer for. const settlement = await paymentsService.reconcileAndConfirmIfPaid(booking.id); - if (settlement.paid || !settlement.verified) { - this.logger.log( - `Skip auto-cancel ${booking.bookingRef}: ${settlement.paid ? 'PAID → confirmed' : 'unverifiable → deferred'}`, - ); + if (settlement.paid) { + this.logger.log(`Skip auto-cancel ${booking.bookingRef}: PAID → confirmed`); continue; } + // Unverifiable: defer — but not forever. IN_FLIGHT is real money moving, so it is waited + // out indefinitely. A PROVIDER_ERROR (dead gateway, payment service down) is bounded by + // RECONCILE_GRACE_MINUTES past the deadline; beyond that the booking is cancelled on an + // UNVERIFIED settlement, which is recorded explicitly below so finance can chase it. + let unverifiedGiveUp = false; + if (!settlement.verified) { + const graceExpiresAt = new Date( + paymentDeadline.getTime() + RECONCILE_GRACE_MINUTES * 60 * 1000, + ); + if (settlement.reason === 'IN_FLIGHT' || now < graceExpiresAt) { + this.logger.log( + `Skip auto-cancel ${booking.bookingRef}: unverifiable (${settlement.reason ?? 'PROVIDER_ERROR'}) → deferred`, + ); + continue; + } + unverifiedGiveUp = true; + this.logger.error( + `Auto-cancelling ${booking.bookingRef} on an UNVERIFIED settlement — the gateway has ` + + `been unreachable for ${RECONCILE_GRACE_MINUTES}+ min past the deadline. If this ` + + `booking was in fact paid, the payment will land on a CANCELLED booking and needs a ` + + `manual refund.`, + ); + } + // 1a. Release held seats (Journey rows are the occupancy source of truth once paid) await this.prisma.journey.deleteMany({ where: { bookingId: booking.id } as any }); @@ -349,15 +381,21 @@ export class TasksService { }); } - // 2. Audit record (no refund — payment was never completed) + // 2. Audit record (no refund — payment was verified never completed, or, on an unverified + // give-up, flagged for review because we could not establish that) await this.prisma.bookingCancellation.create({ data: { bookingId: booking.id, cancelledBy: 'SYSTEM', - reason: 'Payment not completed before deadline', + reason: unverifiedGiveUp + ? `Payment not completed before deadline; settlement UNVERIFIED — gateway unreachable ` + + `for ${RECONCILE_GRACE_MINUTES}+ min past the deadline. Confirm no payment was taken.` + : 'Payment not completed before deadline', refundAmount: 0, refundMethod: booking.paymentIntent?.method ?? 'NONE', - refundStatus: 'NOT_APPLICABLE', + // An unverified give-up may yet turn out to have been paid, so it is neither + // NOT_APPLICABLE nor a refund actually owed — flag it for a human instead. + refundStatus: unverifiedGiveUp ? 'REVIEW_REQUIRED' : 'NOT_APPLICABLE', }, }).catch(() => null); @@ -380,7 +418,10 @@ export class TasksService { await this.sms.sendSms({ to: booking.contactPhone, message }).catch(() => null); } - this.logger.log(`Auto-cancelled: ${booking.bookingRef} (deadline was ${fmtTime(paymentDeadline)})`); + this.logger.log( + `Auto-cancelled: ${booking.bookingRef} (deadline was ${fmtTime(paymentDeadline)})` + + (unverifiedGiveUp ? ' — UNVERIFIED settlement, review required' : ''), + ); cancelledCount++; } catch (err) { this.logger.error( diff --git a/apps/edr-payment-api/src/modules/intents/intents.service.cbe-bill.spec.ts b/apps/edr-payment-api/src/modules/intents/intents.service.cbe-bill.spec.ts index 953533d02..e80f3f640 100644 --- a/apps/edr-payment-api/src/modules/intents/intents.service.cbe-bill.spec.ts +++ b/apps/edr-payment-api/src/modules/intents/intents.service.cbe-bill.spec.ts @@ -155,4 +155,130 @@ describe("IntentsService CBE_BILL", () => { expect(snapshot.status).toBe(ProviderPaymentStatus.REQUIRES_ACTION); expect(applySpy).not.toHaveBeenCalled(); }); + + /** + * Regression: reconcileReference used to route every non-FAILED intent through + * queryProviderStatus, which THROWS "Unknown provider" for CBE_BILL (no map entry — D5). The + * throw was counted as a provider error, so the check returned `unverifiable` forever and the + * owning app could never auto-cancel the booking: seats stayed held and the sweep re-queried + * the same booking once a minute for days. With no outbound query to make, the stored status + * IS the answer. + */ + describe("reconcileReference (reconcile-before-cancel)", () => { + const cbeIntent = (status: ProviderPaymentStatus) => + ({ + id: "intent-1", + service: PaymentService.PASSENGER, + referenceType: PaymentReferenceType.BOOKING, + referenceId: "booking-1", + merchantOrderId: "PSG-x", + provider: ProviderMethod.CBE_BILL, + status, + amountMinor: 1500, + currency: "ETB", + billReference: "000100000015", + }) as unknown as PaymentIntent; + + it("reports an unpaid CBE_BILL intent as VERIFIED not paid, not unverifiable", async () => { + repository.findAllByReference.mockResolvedValue([ + cbeIntent(ProviderPaymentStatus.REQUIRES_ACTION), + ] as never); + + const result = await service.reconcileReference( + PaymentService.PASSENGER, + PaymentReferenceType.BOOKING, + "booking-1", + ); + + expect(result).toEqual({ paid: false, unverifiable: false }); + expect(result.reason).toBeUndefined(); + }); + + it("still reports a retired-but-settled CBE_BILL intent as paid", async () => { + // The inbound /cbe/payment already flipped it; step 2 of the resolution catches it. + repository.findAllByReference.mockResolvedValue([ + cbeIntent(ProviderPaymentStatus.SUCCEEDED), + ] as never); + + const result = await service.reconcileReference( + PaymentService.PASSENGER, + PaymentReferenceType.BOOKING, + "booking-1", + ); + + expect(result.paid).toBe(true); + expect(result.unverifiable).toBe(false); + }); + + it("does not let an unqueryable sibling mask a real provider error", async () => { + const telebirr = { + ...cbeIntent(ProviderPaymentStatus.REQUIRES_ACTION), + id: "intent-2", + provider: ProviderMethod.TELEBIRR, + } as unknown as PaymentIntent; + providers.set(ProviderMethod.TELEBIRR, { + method: ProviderMethod.TELEBIRR, + queryStatus: jest.fn().mockRejectedValue(new Error("ETIMEDOUT")), + }); + repository.findAllByReference.mockResolvedValue([ + cbeIntent(ProviderPaymentStatus.REQUIRES_ACTION), + telebirr, + ] as never); + + const result = await service.reconcileReference( + PaymentService.PASSENGER, + PaymentReferenceType.BOOKING, + "booking-1", + ); + + expect(result).toEqual({ + paid: false, + unverifiable: true, + reason: "PROVIDER_ERROR", + }); + providers.delete(ProviderMethod.TELEBIRR); + }); + + it("reports IN_FLIGHT ahead of PROVIDER_ERROR so a caller never gives up on moving money", async () => { + const processing = { + ...cbeIntent(ProviderPaymentStatus.REQUIRES_ACTION), + id: "intent-2", + provider: ProviderMethod.TELEBIRR, + } as unknown as PaymentIntent; + const failing = { + ...cbeIntent(ProviderPaymentStatus.REQUIRES_ACTION), + id: "intent-3", + provider: ProviderMethod.WAAFI, + } as unknown as PaymentIntent; + providers.set(ProviderMethod.TELEBIRR, { + method: ProviderMethod.TELEBIRR, + queryStatus: jest + .fn() + .mockResolvedValue({ status: ProviderPaymentStatus.PROCESSING }), + }); + providers.set(ProviderMethod.WAAFI, { + method: ProviderMethod.WAAFI, + queryStatus: jest.fn().mockRejectedValue(new Error("ETIMEDOUT")), + }); + repository.findAllByReference.mockResolvedValue([ + processing, + failing, + ] as never); + repository.findById.mockResolvedValue(processing); + jest + .spyOn(service, "applyProviderResult") + .mockResolvedValue({ alreadyTerminal: false }); + + const result = await service.reconcileReference( + PaymentService.PASSENGER, + PaymentReferenceType.BOOKING, + "booking-1", + ); + + expect(result.unverifiable).toBe(true); + expect(result.reason).toBe("IN_FLIGHT"); + providers.delete(ProviderMethod.TELEBIRR); + providers.delete(ProviderMethod.WAAFI); + }); + }); }); diff --git a/apps/edr-payment-api/src/modules/intents/intents.service.ts b/apps/edr-payment-api/src/modules/intents/intents.service.ts index 21da0707f..53d4e70bd 100644 --- a/apps/edr-payment-api/src/modules/intents/intents.service.ts +++ b/apps/edr-payment-api/src/modules/intents/intents.service.ts @@ -49,6 +49,14 @@ export interface ProviderResultInput { rawResponse?: Record; } +/** + * Why a settlement check came back `unverifiable`. The two causes are NOT interchangeable: + * `IN_FLIGHT` is money actually moving and must be waited out indefinitely, while + * `PROVIDER_ERROR` can be a permanently unreachable gateway — a caller may eventually give up on + * that one rather than defer forever (see TasksService's reconcile grace window). + */ +export type ReconcileUnverifiableReason = "IN_FLIGHT" | "PROVIDER_ERROR"; + /** Result of {@link IntentsService.reconcileReference} — a settlement check for a domain order. */ export interface ReconcileReferenceResult { /** True when at least one intent for the order is settled (SUCCEEDED, incl. a just-registered late capture). */ @@ -56,10 +64,12 @@ export interface ReconcileReferenceResult { /** Snapshot of the paying intent when `paid`. */ intent?: PaymentIntentSnapshot; /** - * True when we could NOT confirm "not paid": at least one candidate intent's provider status - * query errored, so its settlement is unknown. Callers must treat this as "do not cancel". + * True when we could NOT confirm "not paid": a candidate intent's provider status query errored, + * or a payment is still in flight. Callers must treat this as "do not cancel". */ unverifiable: boolean; + /** Set whenever `unverifiable` — which of the two causes applies. */ + reason?: ReconcileUnverifiableReason; } @Injectable() @@ -486,19 +496,44 @@ export class IntentsService { const candidates = intents.filter( (i) => i.status !== ProviderPaymentStatus.FAILED, ); + + // Inbound-only methods (CBE_BILL) have deliberately no PAYMENT_PROVIDER_MAP entry — plan D5, + // docs/cbe/CBE_IMPLEMENTATION_PLAN.md. There is NO outbound query to make, so their stored + // status is the best truth available and step 2 above already checked it. Counting them as + // provider errors made every CBE_BILL order permanently `unverifiable` and therefore + // impossible to auto-cancel — the caller deferred forever, once a minute, indefinitely. + const queryable = candidates.filter((i) => this.providers.has(i.provider)); + const unqueryable = candidates.length - queryable.length; + if (unqueryable > 0) { + this.logger.log( + `reconcile: ${unqueryable}/${candidates.length} intent(s) for ${referenceType}/${referenceId} ` + + `have no outbound status query (inbound-only provider) — trusting the stored status`, + ); + } + + // Queried in parallel: a booking that accumulated several dead sessions used to serialise one + // 10s provider timeout per intent, so a single stuck order could hold the caller's sweep for + // 30s+. Results are still APPLIED in order, and we still stop at the first settled intent. + const probes = await Promise.all( + queryable.map(async (intent) => { + try { + return { intent, status: await this.queryProviderStatus(intent) }; + } catch (err) { + this.logger.warn( + `reconcile: queryStatus failed for intent ${intent.id} (${intent.merchantOrderId}): ${ + err instanceof Error ? err.message : String(err) + }`, + ); + return { intent, status: null }; + } + }), + ); + let providerErrors = 0; let inFlight = false; - for (const intent of candidates) { - let status: ProviderStatus; - try { - status = await this.queryProviderStatus(intent); - } catch (err) { + for (const { intent, status } of probes) { + if (!status) { providerErrors++; - this.logger.warn( - `reconcile: queryStatus failed for intent ${intent.id} (${intent.merchantOrderId}): ${ - err instanceof Error ? err.message : String(err) - }`, - ); continue; } @@ -528,7 +563,15 @@ export class IntentsService { } } - return { paid: false, unverifiable: providerErrors > 0 || inFlight }; + // IN_FLIGHT outranks PROVIDER_ERROR: a caller that gives up after N minutes of gateway errors + // must NEVER apply that give-up to an order whose payment is actually moving. + if (inFlight) { + return { paid: false, unverifiable: true, reason: "IN_FLIGHT" }; + } + if (providerErrors > 0) { + return { paid: false, unverifiable: true, reason: "PROVIDER_ERROR" }; + } + return { paid: false, unverifiable: false }; } /** Best-effort live provider status for a merchant order id; never throws (returns null). */