fix: ( payments ) stop reconcile-before-cancel deferring bookings forever

2026-08-26 12:41:04 +00:00 · 2026-08-11 11:19:39 +03:00
parent 8ce6483e26
commit dc2078dd28
5 changed files with 258 additions and 31 deletions
--- 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<T> {
    const url = `${this.baseUrl}${path}`;
-    this.logger.log("=====================================================================");
-    this.logger.log(`URL ${url}`);
-    this.logger.log("=====================================================================");
    try {
      const response = await firstValueFrom(
        this.http.request<T>({
--- 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) {
--- 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(
--- 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);
+    });
+  });
 });
--- 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<string, unknown>;
 }

+/**
+ * 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). */