fix(epay): 4 regressions from adversarial review of the hardening diff

Adversarial review (9 agents) of f7f7c59..28c870f found 4 confirmed bugs in the hardening itself; all fixed: 1. Parallel-download index race: two items with the SAME nrCadastral in one batch both scanned MinIO, both computed index 1, the second putObject silently overwrote the first paid extract. Pre-allocate per-cadastral indices sequentially before the parallel block; storeCfExtract takes an explicit index (epay-queue.ts, epay-storage.ts). 2. Metadata-fail orphan charge: on saveMetadata failure the row was popped from cleanup tracking even when deleteCartItem was NOT confirmed, leaving an undeletable metadata-less row in the global cart that submitOrder would check out and charge. Now: pop only on confirmed delete; if unconfirmed, mark cartDirty and ABORT before submit (epay-queue.ts). 3. Recover vs live queue race: the widened recover WHERE (orderId:null + cart/ordering/... states) could scoop a concurrently-processing batch's rows and re-stamp them with the wrong orderId. Block recover while getQueueStatus().processing (recover/route.ts). 4. 'review' status leaked as 'done' in the geoportal CF-order modal (minioPath short-circuit) — handed an unverified PDF as a finished extract. Check review/failed BEFORE the minioPath fallback (cf-order-modal.tsx). Plus 2 nits: download-zip excludes 'review' rows server-side; retry button surfaces recover errors/results instead of swallowing them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-05 00:17:12 +03:00
parent 28c870fb12
commit b62132ab9e
6 changed files with 137 additions and 23 deletions
@@ -59,6 +59,7 @@ export async function GET(req: Request) {
        minioPath: true,
        documentDate: true,
        completedAt: true,
+        status: true,
      },
    });

@@ -71,7 +72,9 @@ export async function GET(req: Request) {
    for (let i = 0; i < ids.length; i++) {
      const id = ids[i]!;
      const extract = extractMap.get(id);
-      if (!extract?.minioPath) continue;
+      // Skip rows without a file, and "review" rows (PDF present but the
+      // CF↔doc match is unverified — must not land in a "valid extracts" zip).
+      if (!extract?.minioPath || extract.status === "review") continue;

      const dateForName = extract.documentDate ?? extract.completedAt ?? new Date();
      const d = new Date(dateForName);
@@ -15,7 +15,7 @@

 import { NextResponse } from "next/server";
 import { prisma } from "@/core/storage/prisma";
-import { recoverBatch } from "@/modules/parcel-sync/services/epay-queue";
+import { recoverBatch, getQueueStatus } from "@/modules/parcel-sync/services/epay-queue";
 import { requireCfAccess } from "@/core/auth/cf-access";

 export const runtime = "nodejs";
@@ -85,6 +85,22 @@ export async function GET(req: Request) {
    );
  }

+  // Cross-guard against the live batch queue: while a batch is processing,
+  // its rows sit at (orderId:null, status in cart/ordering/...) — exactly the
+  // orphan window the WHERE below matches. Recovering then would re-stamp a
+  // live batch's rows with THIS order's id (wrong PDF, status corruption).
+  // A genuinely-crashed batch leaves __epayQueueProcessing=false (reset on
+  // restart), so blocking here only defers against an actively-running queue.
+  if (getQueueStatus().processing) {
+    return NextResponse.json(
+      {
+        error:
+          "O comandă ePay este în curs de procesare. Așteaptă finalizarea înainte de recuperare.",
+      },
+      { status: 409 },
+    );
+  }
+
  // Candidate rows: anything already tagged with this order that isn't
  // terminal, PLUS recent orphaned rows (orderId:null) in a recoverable
  // state — the operator asserts they belong to this order.
@@ -254,6 +254,23 @@ export function CfOrderModal({
            const row = (data.orders ?? []).find((o) => o.id === id);
            if (row) {
              const s = (row.status || "").toLowerCase();
+              // "review" rows DO have minioPath + documentName but the PDF
+              // may belong to another parcel (ambiguous CF↔doc match) — must
+              // be checked BEFORE the minioPath/documentName completion
+              // fallback, or it would short-circuit to "done" and hand the
+              // operator an unverified extract.
+              if (s === "review") {
+                setError(
+                  "Comanda necesită verificare manuală (potrivire ambiguă document↔parcelă). Verifică în ParcelSync → ePay înainte de a folosi extrasul.",
+                );
+                setPhase("error");
+                return;
+              }
+              if (s === "failed" || s === "error") {
+                setError("Comanda a eșuat la ANCPI.");
+                setPhase("error");
+                return;
+              }
              if (
                s === "completed" ||
                s === "done" ||
@@ -264,11 +281,6 @@ export function CfOrderModal({
                setPhase("done");
                return;
              }
-              if (s === "failed" || s === "error") {
-                setError("Comanda a eșuat la ANCPI.");
-                setPhase("error");
-                return;
-              }
            }
          }
        } catch {
@@ -328,17 +328,35 @@ export function EpayTab() {
   *    order, no new charge. For rows that failed at the download/poll
   *    stage (the order exists at ANCPI but we never stored the PDF). -- */
  const [retryingId, setRetryingId] = useState<string | null>(null);
+  const [retryNotice, setRetryNotice] = useState<string | null>(null);
  const handleRetryDownload = async (order: CfExtractRecord) => {
    setRetryingId(order.id);
+    setRetryNotice(null);
    try {
      const res = await fetch(
        `/api/ancpi/recover?extractId=${encodeURIComponent(order.id)}`,
      );
-      // 409 → the row has no orderId yet (never reached ANCPI); nothing to
-      // recover by row. Other errors surface on the next refresh.
-      await res.json().catch(() => ({}));
+      const data = (await res.json().catch(() => ({}))) as {
+        error?: string;
+        completed?: number;
+        attempted?: number;
+      };
+      if (!res.ok) {
+        // 409 (queue busy / no orderId yet), 404, 500 — tell the user.
+        setRetryNotice(data.error ?? `Reîncercare eșuată (${res.status}).`);
+      } else if ((data.completed ?? 0) > 0) {
+        setRetryNotice(
+          `Recuperat: ${data.completed}/${data.attempted ?? data.completed} extrase.`,
+        );
+      } else {
+        setRetryNotice(
+          "Nimic de recuperat — comanda nu există la ANCPI sau e deja finalizată.",
+        );
+      }
      void fetchOrders(true);
      void fetchEpayStatus();
+    } catch {
+      setRetryNotice("Eroare rețea la reîncercare.");
    } finally {
      setRetryingId(null);
    }
@@ -911,6 +929,20 @@ export function EpayTab() {
        </div>
      )}

+      {/* -- Retry notice ------------------------------------------- */}
+      {retryNotice && (
+        <div className="flex items-center justify-between gap-2 rounded-md border bg-muted/40 px-3 py-2 text-xs">
+          <span>{retryNotice}</span>
+          <button
+            type="button"
+            className="text-muted-foreground hover:text-foreground"
+            onClick={() => setRetryNotice(null)}
+          >
+            ✕
+          </button>
+        </div>
+      )}
+
      {/* -- Active orders indicator -------------------------------- */}
      {hasActive && (
        <div className="flex items-center gap-2 text-xs text-muted-foreground">
@@ -16,7 +16,7 @@
 import { prisma } from "@/core/storage/prisma";
 import { EpayClient } from "./epay-client";
 import { getEpayCredentials, updateEpayCredits } from "./epay-session-store";
-import { storeCfExtract } from "./epay-storage";
+import { storeCfExtract, getNextFileIndex } from "./epay-storage";
 import type { CfExtractCreateInput } from "./epay-types";

 /* ------------------------------------------------------------------ */
@@ -310,8 +310,10 @@ async function processBatch(
    // bail the moment ANCPI reports more rows than we put in.
    // cartCount tracks the rows actually in the cart (incremented on add,
    // decremented only on a CONFIRMED delete) so the invariant stays correct
-    // even if a cleanup delete fails.
+    // even if a cleanup delete fails. cartDirty trips when a metadata-less
+    // row could not be confirmed-deleted → we must not submit.
    let cartCount = 0;
+    let cartDirty = false;
    for (let idx = 0; idx < items.length; idx++) {
      const item = items[idx]!;
      const { extractId, input } = item;
@@ -384,17 +386,42 @@ async function processBatch(
        await updateStatus(extractId, "failed", {
          errorMessage: "Salvarea metadatelor în ePay a eșuat.",
        });
-        // Remove this metadata-less row from the cart so it can't be
-        // checked out and charged. Only decrement cartCount if ANCPI
-        // confirmed the delete — otherwise the row is still there and the
-        // invariant must keep counting it.
+        item.basketRowId = undefined; // exclude from validItems regardless
+        // Remove this metadata-less row from the cart so it can't be checked
+        // out and charged. Only drop it from tracking + decrement cartCount
+        // if ANCPI CONFIRMED the delete; otherwise the row is still in the
+        // cart, must stay in cleanup tracking, and the cart is now "dirty".
        const deleted = await client.deleteCartItem(basketRowId, idx);
-        if (deleted) cartCount--;
-        ourBasketIdsForCleanup.pop();
-        item.basketRowId = undefined;
+        if (deleted) {
+          cartCount--;
+          ourBasketIdsForCleanup.pop();
+        } else {
+          // Undeletable metadata-less row → submitting now would check it out
+          // and charge for it. Refuse to submit this batch.
+          cartDirty = true;
+        }
      }
    }

+    // A metadata-less row we couldn't remove is still in the global cart;
+    // submitOrder would check out the WHOLE cart and charge for it. Abort
+    // (best-effort wipe) instead of submitting a cart we can't guarantee
+    // clean. The catch below would NOT fire on the success path, so this
+    // explicit guard is what stops the unintended charge.
+    if (cartDirty) {
+      console.error(
+        "[epay-queue] Cart has an undeletable metadata-less row — aborting before submit.",
+      );
+      await client.deleteCartItems(ourBasketIdsForCleanup);
+      for (const id of extractIds) {
+        await updateStatus(id, "failed", {
+          errorMessage:
+            "Coș ePay nu a putut fi curățat (ștergere neconfirmată) — comanda a fost oprită pentru a evita o plată fără metadate. Reîncearcă.",
+        });
+      }
+      return null;
+    }
+
    // Filter to only items that had successful metadata saves
    const validItems = items.filter((i) => i.basketRowId !== undefined);
    if (validItems.length === 0) {
@@ -506,8 +533,14 @@ async function finalizeOrder(
      item: QueueItem;
      doc: (typeof downloadableDocs)[number];
      matchedByIndex: boolean;
+      index: number;
    };
    const plans: Plan[] = [];
+    // Per-cadastral file index, pre-allocated SEQUENTIALLY so two parallel
+    // tasks for the same cadastral never collide on the MinIO scan (which is
+    // a read-modify-write). Seed each distinct cadastral from MinIO once,
+    // then hand out 1,2,… within this batch.
+    const nextIndexByCad = new Map<string, number>();
    for (let i = 0; i < validItems.length; i++) {
      const item = validItems[i]!;
      const nrCF = item.input.nrCF ?? item.input.nrCadastral;
@@ -532,12 +565,19 @@ async function finalizeOrder(
        });
        continue;
      }
-      plans.push({ item, doc, matchedByIndex });
+
+      const cad = item.input.nrCadastral;
+      let next = nextIndexByCad.get(cad);
+      if (next === undefined) next = await getNextFileIndex(cad);
+      nextIndexByCad.set(cad, next + 1);
+
+      plans.push({ item, doc, matchedByIndex, index: next });
    }

    // Step 6: download + store in parallel (bounded). Each task is fully
-    // self-contained so a failure on one row doesn't abort the others.
-    await runWithConcurrency(plans, DOWNLOAD_CONCURRENCY, async ({ item, doc, matchedByIndex }) => {
+    // self-contained so a failure on one row doesn't abort the others. The
+    // file index is pre-allocated above, so parallel stores never overwrite.
+    await runWithConcurrency(plans, DOWNLOAD_CONCURRENCY, async ({ item, doc, matchedByIndex, index: fileIndex }) => {
      try {
        await updateStatus(item.extractId, "downloading", {
          idDocument: doc.idDocument,
@@ -559,6 +599,7 @@ async function finalizeOrder(
            stare: finalStatus.status,
            produs: "EXI_ONLINE",
          },
+          fileIndex,
        );

        if (!doc.dataDocument) {
@@ -83,15 +83,25 @@ export function buildFileName(
 /**
 * Store a CF extract PDF in MinIO.
 * Returns the MinIO path and file index.
+ *
+ * Pass `explicitIndex` to skip the MinIO scan and use a caller-allocated
+ * index. Required when storing concurrently (parallel downloads): the scan
+ * is an unsynchronised read-modify-write, so two tasks for the same
+ * cadastral would both compute index 1 and the second putObject would
+ * silently overwrite the first. The caller pre-allocates distinct indices.
 */
 export async function storeCfExtract(
  pdfBuffer: Buffer,
  nrCadastral: string,
  metadata: Record<string, string>,
+  explicitIndex?: number,
 ): Promise<{ path: string; fileName: string; index: number }> {
  await ensureAncpiBucket();

-  const index = await getNextFileIndex(nrCadastral);
+  const index =
+    explicitIndex !== undefined
+      ? explicitIndex
+      : await getNextFileIndex(nrCadastral);
  const fileName = buildFileName(index, nrCadastral, new Date());
  // Store in subfolder per cadastral number
  const path = `parcele/${nrCadastral}/${fileName}`;