Repository navigation
Per-tenant payment redirects, and an alarm for payments that never reached an order - #6
Merged
helgarpalmieri55 merged 9 commits intoAug 16, 2026
Conversation
…one global URL
`packages/payments/src/storefront-base.ts` resolved the storefront base URL
for Wompi's `redirect-url` and ePayco's bridge page from ONE global env var,
`PAYMENTS_STOREFRONT_BASE_URL`, because `OrderForPayment` carried no tenant
domain. With two or more tenants on Wompi or ePayco, every tenant's shopper
was redirected to whichever single storefront that variable named — the file's
own doc comment had flagged this as "a real, load-bearing, multi-tenant-wrong
limitation" since it was written.
Since the Wompi return page started PATCHing the API, it also became a
cross-tenant WRITE with no attacker involved: tenant B's shopper landing on
tenant A's storefront had A's proxy stamp `x-tenant-domain: A`, writing B's
transaction id onto A's same-numbered order, which A's reconciliation worker
then queried with A's own credentials.
## Threading the real per-tenant URL
- `OrderForPayment.storefrontBaseUrl` — a new REQUIRED field, not a sibling
input and not optional. Not a third `createCheckoutSession` parameter
because `epayco.ts`/`mercadopago.ts` already use an optional third
positional param as their injected-`fetch` test seam. Required rather than
optional-with-a-fallback because an optional field backed by a global
default reproduces the exact failure mode being fixed: a caller that forgets
it produces a silently wrong redirect with no signal. Required means
TypeScript refuses to compile a call site that hasn't decided.
- `DomainResolver` now puts the `TenantDomain.domain` row it matched on
`ResolvedTenant.domain` (the DB's value, never the raw header — a host
matching no row resolves to null and 404s at `PublicTenantGuard`, so a value
here is always a domain registered FOR THIS TENANT). A cached entry from
before this field existed is treated as a miss and re-read, so a deploy
can't serve `http://undefined/...` for the 60s TTL.
- `PublicTenantGuard` sets `req.storefrontTenantDomain` beside
`storefrontTenantId`; `@StorefrontTenantDomain()` reads it; the checkout
controller passes it to `CheckoutService.checkout`, which turns it into a
base URL via the new `tenants/tenant-public-url.ts` — reusing the existing
per-request resolution rather than adding a second mechanism (a tenant can
own a fully custom domain no `PLATFORM_ROOT_DOMAIN` rule derives, and can
own several, of which the right one is the one the shopper arrived on).
## http vs https, explicit
`STOREFRONT_PUBLIC_SCHEME` (`http`|`https`) overrides for every tenant and
throws on a typo. Unset, the scheme is inferred: `localhost`, `*.localhost`,
`*.local`, `*.test` and loopback get `http` (this repo's dev stack — Caddy
serves `http://*.ventia.localhost`); every other, i.e. publicly registrable,
domain gets `https`. `https` is the safe direction for the unknown case, and
the `http` rule only ever matches names nobody can register, so it can never
downgrade a real storefront.
## PAYMENTS_STOREFRONT_BASE_URL is gone, with no dev fallback
A "dev only" fallback would keep the property that made the bug invisible —
a call site that omits the value silently gets a plausible global one — and
would make dev exercise a different path from production. It is also
unnecessary: dev resolves per-tenant domains through the same machinery
production does. `storefront-base.ts` now only VALIDATES the supplied value
(absolute http(s), no query/fragment/userinfo) and throws rather than
substituting anything. Its multi-tenant-wrong warning is deleted, not
softened, because it would now be a stale description of a fixed problem.
## ePayco's missing return path
`standard`-mode shoppers had no automatic way back: `open()` is a full-page
navigation, so the bridge page unloads and no `setHooks` closure can fire,
leaving the manual "Ya pagué" link on a page the shopper had already left.
With a per-tenant public URL available, `epayco.ts` now sets the
session-create `response` field to that tenant's own
`/pago/epayco-retorno/{orderNumber}` route, which is added here (mirroring the
Wompi return page, order number in the PATH so ePayco's appended
`?ref_payco=` can't be swallowed).
Verified first-party only: docs.epayco.com/docs/checkout-implementacion's
verbatim session-create example carries `"response": "https://mysite.com"`;
/docs/paginas-de-respuestas says the shopper returns with `ref_payco` in the
URL parameters and warns the page is "NOT reliable to validate the final state
of the transaction"; github.com/epayco/resources' `onePage/response/response.html`
does `getQueryParam('ref_payco')` and `epayco-ng6`'s `response.component.ts`
does `params['ref_payco'] || params['x_ref_payco']` — hence both spellings are
accepted.
`confirmation` and `method` are still deliberately NOT sent: both touch the
signature-verified settle path (`method`'s legacy analogue selects the
CONFIRMATION webhook's HTTP method), and no sandbox account exists here to
prove a switch-over doesn't drop deliveries.
**This does NOT restore ePayco reconciliation coverage.** A response-page ref
is `hint`-sourced, and `reconciliation.worker.ts`'s provenance gate refuses
hint-sourced refs for ePayco because its status-lookup endpoint is
unauthenticated and merchant-agnostic. An ePayco order whose webhook never
arrives still cannot be settled and still falls through to the 15-minute
stock-reservation expiry worker. Stated in the adapter, both storefront
modules, and the README so nobody reads the capture as closing that gap.
## Security controls: none weakened
`'epayco'` was NOT added to `ACCOUNT_SCOPED_LOOKUP_PROVIDERS`, and
`reconciliation.worker.ts`, `webhooks.controller.ts` and `payments.service.ts`
are untouched. The provenance gate, the amount check, the currency check,
tenant-scoped webhook idempotency and the binding check are all unchanged.
Wompi's `redirect-url` is still added AFTER the integrity signature is
computed, and both return routes still carry the order number in the path.
## Tests
+58 tests (907 -> 965), zero regressions across the full
`turbo run lint typecheck build test` gate. The headline one asserts two
different tenants get two different redirect base URLs, at three layers:
`tenant-public-url.test.ts` (unit), the adapter suites (wompi + epayco, both
the bridge and `response` URLs), and `checkout.test.ts` end-to-end through the
real HTTP stack with one `*.ventia.localhost` tenant and one custom-domain
tenant. Also verified manually against the real dev stack (docker Postgres +
Redis, real API through Caddy, seeded tenants):
demo-moda.ventia.localhost -> http://demo-moda.ventia.localhost/pago/wompi-retorno/1
tienda-demo-tech.example.com -> https://tienda-demo-tech.example.com/pago/wompi-retorno/1
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
The variable was read per-request off process.env but never declared in loadEnv's boot schema, and the API has no global exception filter — so STOREFRONT_PUBLIC_SCHEME=htps passed boot, passed the entire test suite, and then returned a bare 500 on every non-cod checkout for every tenant. The first signal would have been a shopper failing to pay. That contradicted this schema's own stated convention four lines away, where PAYMENTS_ENCRYPTION_KEY notes that a malformed value "must fail loudly at boot, not silently produce garbage ciphertext" — which is exactly the posture a total-checkout-outage misconfiguration deserves. Also drops '::1' from LOCAL_HOST_EXACT: the port strip rewrites it to ':' before the Set is consulted, so the entry could never match, and the adjacent comment claimed IPv6 literals "survive intact" — asserting a property the code did not have. Brackets are mandatory for an IPv6 literal in an authority (RFC 3986), so a bare ::1 never arrives; the '[::1]' entry already handles the real form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
…of burying them in a log A signature-verified PAID webhook that lands on an order the stock-reservation expiry worker already cancelled is durably recorded as `WebhookEvent.result = 'paid_order_not_settleable'`. That row is the only record anywhere that a shopper was charged and has no order — and the only way to find it was a server log or a raw SQL query. No merchant will ever do either, so in practice the one case in the payment system where money has moved and a human must act was invisible. This adds the operator-facing surface, and nothing else: the settle path (`webhooks.controller.ts`, `payments.service.ts`, `reconciliation.worker.ts`, every `packages/payments` adapter) is untouched. DB — `WebhookEvent` becomes tenant-READABLE A new migration grants `ventia_app` exactly SELECT on this table (it had ALL PRIVILEGES revoked) and enables an RLS `FOR SELECT` policy scoped to `app.tenant_id`, plus a `(tenantId, result, processedAt DESC)` index. The model joins `TENANT_MODELS`, so `tenantDb` also AND-scopes it at the application layer. Writes stay owner-only on `platformDb`: the webhook handler is unaffected, and no tenant-scoped code can amend this audit trail. API — `GET /v1/admin/payment-alerts` Owner-or-staff (`AdminSessionGuard`, no `@Roles()` — same gate as the orders controller, because chasing a charged customer is fulfilment work). Paginated with the same clamping as `OrdersService.list`. Projects order number, amount, gateway, timestamp, gateway event id and the referenced order's current state. The raw `payload` is NEVER exposed — it is read only to recover the order link and never reaches a DTO. `WebhookEvent` carries no `orderId`, so the link is recovered from the payload per provider (wompi `data.transaction.reference`, epayco `x_extra1`, mercadopago `data.id` -> `Order.providerRef`). Display-only and strict: a malformed payload, an unknown provider or an ambiguous `providerRef` resolves to nothing rather than to a guess, and the alert still lists with the gateway's own event id. Adding an `orderId` column written by the handler would be strictly better and is raised as a follow-up rather than made here. Admin — a shell-wide banner plus `/pagos-por-revisar` The banner renders in the `(app)` layout, so it is present on every authenticated page: a merchant who logs in to add a product still meets it. It renders nothing at all when there is nothing wrong, so the region never becomes noise. The dedicated page holds the detail, the es-CO explanation of what actually happened, and the three steps to resolve it out of band. No dismiss/acknowledge affordance, deliberately: a one-click hide on a real financial discrepancy, with no actor and no reason, is a worse failure than the nag. If added later it should be an append-only, tenant-owned review row recording who reviewed it and what they did. Tests: cross-tenant isolation (including two tenants holding the same `Order.number`, and RLS proven independently of the service's own filter), the tenant-less-event case, write-protection, payload non-exposure, the clean empty state, and per-provider payload projection. 999 tests pass across the workspace, up from 967, with no regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
…trustworthy amount Closes the review of 4754112. The settle path is untouched: payments.service.ts, reconciliation.worker.ts and every packages/payments adapter have zero diff, and webhooks.controller.ts is comment-only (verified: the whole diff for that file is comment lines). FIX 5 — the alarm had no off switch, and these events are not rare The original build shipped no dismiss, on two arguments. One was wrong: ePayco supports PSE, whose Pendiente -> Aceptada progression is asynchronous by design, while the stock-reservation TTL is 15 minutes — a bank transfer outrunning a 15-minute hold is structurally expected, not a tail event. A merchant with steady PSE volume would accumulate a permanently growing list and a permanent banner, training exactly the skip-that-region habit that defeats the feature. The other argument stands, and shapes the fix: nobody may make a financial discrepancy vanish without a trace. So this is a REVIEW, not a dismiss. A new tenant-owned `WebhookEventReview` records WHO acted and WHAT they did (a small enum + optional note); reviewed alerts leave the banner count and move to a "Revisados" section showing the reviewer and the timestamp. `WebhookEvent` itself is not touched and stays SELECT-only for tenants. Append-only is enforced by Postgres, not by application code: ventia_app gets SELECT + INSERT, UPDATE/DELETE are REVOKEd (undoing ALTER DEFAULT PRIVILEGES from the rls migration), and the RLS policies are FOR SELECT / FOR INSERT only, so a restored grant still matches no rows. Undo is reversible but never destructive: a mistaken review is corrected by APPENDING a `reopened` row, and current state is derived from the latest row per event rather than stored — the mistake and its correction both stay on the record. FIX 4 — the money-shaped output trusted a column the codebase refuses to trust payment-alerts.service.ts resolved orders via `providerRef` with no reference to `providerRefSource`, while schema.prisma calls 'hint' "attacker-controlled by assumption" and NULL "treated exactly like 'hint' by every consumer: fail closed, never open" — a gate reconciliation.worker.ts honours. Chose the stricter option: require `providerRefSource: 'verified'`, rather than resolving-but-suppressing the amount. Suppressing the amount is not enough once guidance is per-row, because the guidance is DERIVED from the resolved order's state — an attacker choosing the order also chooses the instruction the merchant reads, and a planted hint on a DELIVERED/PAID order would render "refund the duplicate" against a legitimate single payment. Only Mercado Pago resolves this way; those alerts now list with a null order, the gateway event id intact. The order-NUMBER path (Wompi/ePayco, the dominant cases) never consults providerRef and is unchanged. FIX 2 / FIX 3 — guidance is now per-row and never describes an impossible action `paid_order_not_settleable` is written whenever markPaid returns false, i.e. for ANY order not PENDING + (PENDING|FAILED) — so one result code covers the expired-cancelled case AND a second PAID event on an already-settled order: a shopper who retried checkout and was genuinely double-charged, who DOES have an order. The blanket "ya estaba cancelado / no tiene un pedido" prose is gone; each row derives its own story from order.status/paymentStatus. And nothing tells a merchant to "toma el pedido de nuevo": orders.controller.ts has no @post and there is no order-creation UI. The page says plainly that orders cannot be created or revived in the panel — a customer who takes it again places a NEW order in the storefront. Refunds stay framed as out-of-band. FIX 6 — a persistent failure no longer looks like a healthy system The banner swallowed every error and rendered nothing, so revoking the SELECT grant made the alarm silently disappear while the shell looked fine. Transient and persistent failures are now distinguished: a network-layer failure stays quiet on the first try, but a non-network status (a 500 is a bug, not a blip) surfaces a quiet one-line notice immediately, and repeated failures surface after two. The API logs the failure server-side with the tenant id. FIX 1 — the hot-path comment no longer describes a false security posture webhooks.controller.ts said WebhookEvent "has no RLS policy at all" and that ventia_app has "ALL PRIVILEGES explicitly REVOKED" — both false since 4754112's migration. Now states what is true: SELECT only, writes owner-only, so this create must stay on platformDb. Same staleness corrected in README.md (which still told readers raw SQL was the only way to find these) and the P0 foundation plan (which still listed WebhookEvent as RLS-excluded). Minor - A row could show "VNT-88888" and "no pudimos identificar el pedido" at once. `orderNumber` is now the resolved order only; the payload's claim is a separate `referencedOrderNumber`, labelled as the gateway's reference. - Pagination tiebreak was non-deterministic (processedAt, createdAt — neither unique), so identical-timestamp rows could duplicate or skip across pages. Added `{ id: 'asc' }` and corrected the comment claiming createdAt broke ties. - `toProviderRef` had no length bound; a 200 KB id went into an IN list. - Register was mixed (banner usted, page tuteo). Tuteo throughout, matching lib/errors.ts. Also: scripts/e2e.sh never exported PAYMENTS_ENCRYPTION_KEY, required since P3a, so it could not boot the API at all and failed at the 90s health-check wait. Tests: 1031 pass across the workspace, up from 999, zero regressions. New coverage includes DB-level append-only enforcement (tenant UPDATE/DELETE rejected via both raw SQL under ventia_app and the tenant Prisma client), cross-tenant isolation on the new table including an RLS WITH CHECK violation, a reviewed alert leaving the banner count, the reopen-as-append undo, per-row guidance differing between a cancelled and an already-settled order, and the unverified-providerRefSource path. A new Playwright e2e proves the two causes render opposite guidance on one page, that a planted hint ref surfaces no "Monto cobrado", and the review -> "Revisados" -> reopen flow in a real browser. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
…ack later
The admin payment-alerts page has to tell a merchant WHICH order a
paid-but-unsettleable charge belonged to. Until now it worked that out
after the fact, by re-reading the stored gateway payload per provider
(payment-alerts/webhook-links.ts). The webhook handler already knew the
answer with certainty at the moment it resolved the order — it just threw
that away and left the alerts page to reconstruct it.
Reconstruction had three problems, and all three go away together:
- It could not resolve Mercado Pago. MP's delivered notification body is
`{type, data:{id}}` — no order reference, no amount — so those alerts
could only be matched through `Order.providerRef`.
- That ref path then needed a trust gate. `providerRef` is written by the
deliberately-unauthenticated hint endpoint, which schema.prisma calls
"attacker-controlled by assumption", so it had to require
`providerRefSource = 'verified'`. Since the per-row guidance is derived
from the resolved order's own status, whoever controlled the link
controlled the instruction the merchant read. Correct, but it meant most
MP alerts listed with no order at all.
- It had to refuse to answer when two orders shared a providerRef, since
that column has no uniqueness constraint.
`WebhookEvent.orderId` is written once by the handler, in the same UPDATE
that stamps the outcome, so an event's disposition and the order it applied
to can never disagree. Resolution becomes one batched lookup by primary key:
no ambiguity to detect, no provenance to gate, no per-provider payload
knowledge, and Mercado Pago resolves like everything else.
`payload` is no longer read on this path at all — the alerts list stopped
selecting it, so the raw gateway body (shopper PII, signature material)
never enters the process on the way to the admin app.
The column is nullable and NOT backfilled. `invalid_reference` and
`order_not_found` rows have no order by construction, and backfilling
historical rows would mean running the payload re-derivation this column
exists to retire, then writing its guesses into the field that is supposed
to be authoritative. A legacy alert lists as unidentified, carrying the
gateway's event id — what the merchant had anyway. No foreign key, matching
InventoryMovement.orderId: this table is the system's record of what a
gateway said, and must not become un-writable, or cascade-deletable, because
of the state of an order it names.
Tests: the handler now asserts it writes the link on the alert path and
records a true null on both no-order branches; the write was mutation-tested
by removing it. The provenance tests are rewritten rather than dropped —
they now assert the stronger property that replaced the gate: a planted hint
ref is ignored entirely, and when payload and stored link name different
orders, the link wins. Added a cross-tenant case proving an orderId pointing
across the boundary resolves to nothing (no FK enforces that; tenantDb and
RLS do). Deleted webhook-links.ts and its 11 projection tests with it.
Full gate 27/27, 1020 tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
…g a payment
`fetch` has no default timeout. Undici waits on a connected but silent socket
indefinitely, which is the characteristic failure of an overloaded API — and
it does NOT surface as a connection error. All eight outbound calls in this
package were unbounded, and each one sits somewhere that cannot absorb an
unbounded wait:
- `createCheckoutSession` runs inside a shopper's checkout request, which
holds an open HTTP connection and, on the Mercado Pago/ePayco paths, a
transaction that has already decremented stock.
- `getTransactionStatus` runs inside the reconciliation worker's repeatable
job. A hang there stops the sweep — silently, for as long as the socket
stays open — so no further order gets reconciled.
- ePayco's `verifyAndParseWebhook` calls the gateway back mid-verification
(its confirmation signature covers neither status nor reference, so both
are re-read from ePayco's own record). A hang there holds the webhook
request that settles the order.
A bounded failure beats an unbounded wait in all three: the gateway retries
the webhook, the worker sweeps again in two minutes, and the shopper gets an
error instead of a spinner.
`fetchGateway` attaches a 10s `AbortSignal.timeout` and rewrites the rejection.
That rewrite is the second half of the fix: `AbortSignal.timeout` rejects with
a bare DOMException reading "The operation was aborted" — no provider, no
endpoint, no duration — which is precisely the message an operator would find
in the logs of a payment that did not go through. It now names all three, in
the same shape as this package's other errors. Non-timeout failures (DNS, TLS,
connection refused) are re-thrown untouched rather than buried.
The timeout is identified by its own signal rather than by the error's name,
because a caller-supplied abort raises the same exception type; blaming the
timeout for someone else's cancellation would send an operator hunting a slow
gateway that was never slow. A caller signal is composed with the bound via
`AbortSignal.any` rather than replacing it.
8 tests, mutation-tested: dropping the signal fails 5 of them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
Turbo runs tasks in a filtered environment: a variable not named in `globalPassThroughEnv` (or a task's own `env`) does not exist in the child process. That default is right — it stops a build depending on something invisible — but it strips the outbound network configuration a proxied environment needs, and the failure lands nowhere near the cause. `@ventia/storefront:build` failed with "self-signed certificate in certificate chain", once per font request, because `next/font` fetches Google Fonts at build time through a TLS-terminating proxy whose CA is named by NODE_EXTRA_CA_CERTS — filtered out by turbo. Running `pnpm next build` directly in the same directory succeeded, since it inherits the real environment. That discrepancy is what made this look like a flaky font CDN for a while. Verified both directions: with this list the storefront builds under turbo's default strict env mode, and removing the list reproduces the certificate error exactly. The `--env-mode=loose` workaround the gate had been running with is no longer needed. Pass-through rather than `globalEnv` deliberately: this is how a build reaches the network, not an input that changes its output. In `globalEnv`, every developer's and CI runner's differing proxy value would bust the whole cache for no behavioural reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
docs/SPEC.md §9 requires "rate limiting (Redis) on auth, checkout, agent, webhooks". Nothing implemented it. The agent surface does not exist yet (P4); the other three are wired here. Express middleware rather than Nest guards, because `/v1/auth/*` is handled by better-auth's `toNodeHandler` mounted directly on the Express adapter, before Nest routing ever sees the request — a guard would never run for exactly the endpoints that most need limiting (credential stuffing, signup spam, verification-email floods). Fixed window (INCR + EXPIRE, one key per caller per window) rather than a sliding log. The log is more precise at the boundary, but stores one member per request — unbounded memory driven by precisely the traffic a limiter exists to survive. The limits are set with the 2x boundary case in mind rather than in spite of it. Fails OPEN. If Redis is unreachable the request proceeds, which is essential for one route in particular: a rejected payment webhook is a settle that did not happen, so a Redis outage must never become lost payments. This is abuse control, not authorization — nothing here decides whether a caller MAY act, only whether they are acting too fast. Webhooks are keyed per tenant+provider, not per IP: gateway callbacks arrive from a handful of the gateway's own addresses, so an IP key would put every merchant in one bucket and let one busy store stall every other store's settlements. A webhook path carrying no tenant is not limited at all rather than being lumped into a shared bucket. `trust proxy` is set to 1 rather than `true`. `true` takes the left-most X-Forwarded-For entry, which the client supplies and can forge, letting an attacker mint a fresh identity per request and bypass the limit entirely. Limits are env-overridable, and the defaults are deliberately generous. Wiring them up surfaced the reason: a large share of Colombian retail traffic is mobile, where carrier-grade NAT puts many unrelated shoppers behind one public address, so my first pass (20 checkouts/min per IP) would have rejected real customers during exactly the traffic spike a merchant most wants. Defaults are now 60/60/600 per minute, and a malformed value falls back rather than becoming NaN — a NaN limit would silently disable limiting altogether. Tests: 11 covering the limiter (per-key and per-limiter isolation, TTL, real window rollover, Retry-After, fail-open, null-key skip, webhook tenant keying) and 4 proving the middleware is actually mounted on the real routes — the latter set the limits to 1/min before building the app, since a direct-drive test would keep passing if the wiring were deleted. Mutation-tested: failing closed breaks the fail-open test, disabling the check breaks 7, and unmounting the auth limiter breaks the wiring test. The suite raises the limits in test/setup.ts because all test traffic shares one address. Full gate 27/27, 550 API tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
…both sweeps
Three related fixes to the order/payment background machinery.
## A cancelled order claimed it was still awaiting payment
`transition()` never wrote `paymentStatus`, so a merchant cancelling an unpaid
online order left it CANCELLED/PENDING — while the automatic 15-minute expiry
of the very same order writes CANCELLED/EXPIRED. Two representations of one
real-world end state, and the merchant's own order list read the second of them
as "waiting on payment" about an order they had just cancelled.
Cancel now maps PENDING/FAILED to EXPIRED, matching the expiry worker exactly
(its sweep filters on status and stockReservedUntil, never on paymentStatus, so
it overwrites FAILED the same way). EXPIRED rather than a new enum value
because `paymentStatus` answers only "what happened to the money", and the
answer is identical either way: the window closed, nothing arrived. WHO closed
it is already recorded distinctly in two places — the OrderEvent
('status_changed'/'staff' vs 'reservation_expired'/'system') and the
InventoryMovement reason ('order_cancelled' vs 'order_expired') — so encoding
it a third time would give a reader two fields that can disagree.
Narrowed to PENDING/FAILED, which is what makes it safe: PAID is never touched
(a cancel after payment is a refund, and the money DID arrive — overwriting it
would be the same class of lie markFailed's precondition prevents), and COD is
never touched (a cash order has no gateway window to close).
## The expiry sweep was unbounded
`expireReservations()` read every matching order with no `take`. That holds in
steady state and stops holding in exactly the situation the worker exists for:
if the worker is down, or BullMQ stalls, or a promotion drives a burst of
abandoned checkouts, the first run back reads the entire backlog into memory
and processes it one transaction at a time. Now capped at 500, ordered
oldest-first so a truncated run drains the FRONT of the backlog, and it logs
when it hits the cap — a run that truncates silently is indistinguishable from
a healthy one.
## Both sweeps were sequential scans
They run platform-wide with no tenant filter, and the only index that could
have helped is ("tenantId", status) — unusable by a query that does not
constrain its leading column. So both scanned the whole orders table, every
run, every minute. That cost grows with total orders ever placed while the rows
either sweep wants shrink toward zero: work per run grows precisely because the
platform is succeeding.
Two partial indexes on `status = 'PENDING' AND "stockReservedUntil" IS NOT
NULL` — a tiny, self-draining slice, since every row leaves it within 15
minutes. `paymentStatus` is deliberately left out of the reconciliation index's
predicate: it is the one filter of the four that CHANGES while a row sits in
the set, and a mutable column in a partial predicate moves rows out of and back
into the index.
Tests: 5 on the cancel behaviour (including both must-not-touch cases,
mutation-tested by widening the write — PAID and COD both fail), and 3 on the
indexes. The index tests assert usability via EXPLAIN with enable_seqscan off,
not mere existence: a partial index whose predicate does not match the query is
dead weight at any size and nothing about its presence would reveal that.
Mutation-tested by changing the predicate to 'CANCELLED' — the usability test
fails while the existence test still passes.
Full gate 27/27, 558 API tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tUfx41TMxana567RboGMb
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
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.
Four commits of post-P3 payment follow-ups. Two close a live bug in the redirect path; two build the merchant-facing alarm for money that arrived without an order behind it.
fee4246— shoppers were redirected to one global storefrontEvery tenant's payment redirect was built from a single
PAYMENTS_STOREFRONT_BASE_URL, so a shopper paying at store B could land on store A's confirmation page after checkout. The URL is now derived per tenant, andOrderForPayment.storefrontBaseUrlis required rather than optional.The env var was removed outright rather than kept as a dev fallback. A dev-only fallback would have preserved exactly the property that made this bug invisible for so long — the code path works fine until it's multi-tenant, and then it silently sends people to the wrong merchant.
02352a0— a typo'd scheme override failed at payment time, not bootSTOREFRONT_PUBLIC_SCHEMEoverrides the http/https inference. Misspelling it used to do nothing until a real shopper hit a real redirect. It's now declared in the env schema so it fails loudly at boot.4754112— surface paid-but-unsettleable paymentsWhen a gateway confirms a payment the settle path can't apply to an order, the shopper has been charged and has nothing to show for it. That outcome was only visible in a server log. This adds
GET /v1/admin/payment-alertsand a/pagos-por-revisarpage so the merchant actually finds out.WebhookEventbecomes tenant-readable through aFOR SELECT-only RLS policy — tenants can read their own events and nothing else. Writes stay owner-only: the audit trail of what a gateway said is not something tenant-scoped code should be able to amend.71f093e— give the alarm an off switch, per-cause guidance, and a trustworthy amountThe first cut shipped no way to clear an alert, on the assumption these events would be rare. That assumption was wrong: ePayco supports PSE (Colombian bank transfer), whose
Pendiente → Aceptadaprogression is asynchronous, and the stock reservation TTL is 15 minutes. A transfer outrunning the hold is structurally expected, not a tail event — so a merchant with steady PSE volume would accumulate a permanent, unclearable banner and learn to ignore that whole region of the page.Per-cause guidance. The page previously asserted one cause for a result that has two, with opposite remedies. A
DELIVERED/PAIDorder was rendering under prose saying the customer has no order and should be refunded — a money-losing instruction. Guidance is now derived from the resolved order'spaymentStatus: a cancelled order says the customer was charged and has no order; an already-settled one says the customer does have their order, don't cancel it, refund only the duplicate.Copy no longer promises things the panel can't do. It used to say "toma el pedido de nuevo" when no order-creation endpoint or UI exists.
Order links must be verified. Alert rows resolved via
providerRefnow requireproviderRefSource: 'verified'; a'hint'orNULLsource fails closed. This matters because of the per-cause guidance above — whoever controls the link controls the instruction, so a planted reference on aDELIVERED/PAIDorder could have rendered "refund the duplicate" against a legitimate single payment. Cost: Mercado Pago alerts now list with a null order and the gateway event id intact.Review record. New
WebhookEventReviewtable, append-only at the database level —ventia_appgetsSELECT, INSERTwithUPDATE/DELETErevoked, and the RLS policies areFOR SELECT/FOR INSERTonly, so a mistakenly restored grant still matches zero rows. TheREVOKEis load-bearing rather than decorative: the original RLS migration'sALTER DEFAULT PRIVILEGESgrants all four verbs to every table created after it.Undo is reversible by appending, not by mutating — a mis-click files a
reopenedrow and current state is derived as the latest row per event. Refusing undo would itself have been a way to lose an alert silently, which is the exact failure the no-dismiss rule exists to prevent.Banner failures are visible. It previously swallowed all errors, so a persistent breakage looked identical to a healthy system.
Verification
pnpm turbo run lint typecheck build test: 27/27, exit 0payments.service.ts,reconciliation.worker.ts, and all ofpackages/payments;webhooks.controller.tschanges are comment-only, verified by filtering the diff to non-comment lineswhereclause, Postgres RLS with the app layer bypassed, and aWITH CHECKtest showing an INSERT claiming another tenant is refused by the databaseKnown, not addressed here
p2-dod.spec.tsCOD checkout doesn't reach the confirmation URL. Reproduced identically on a pristine tree, so it predates this branch — flagged for its own look rather than folded in.HTTPS_PROXY, sonext/fontcan't reach Google Fonts and the storefront build fails underturbowhile passing standalone. Worked around with--env-mode=loose; the real fix is adding the proxy vars toturbo.json'sglobalPassThroughEnv.Generated by Claude Code