Fix fx-engine cache growth, DLQ header loss, asset audit gaps, and IndexedEvent null dedup - #640
Merged
therealjhay merged 4 commits intoSep 1, 2026
Conversation
…etta-Pay#615) resolveRate() already refused to *serve* a computed pair-rate entry past RATE_TTL_MS (it recomputes live once now - computedAt >= ttlMs), but nothing ever removed the stale entry itself. computedRateCache is only cleared wholesale on a successful updateBaseRates() call, so during a prolonged upstream outage (refreshTick keeps failing) the map just keeps growing — every pair ever queried sits in memory forever. Add pruneExpiredComputedRates(), run once per refresh tick, to reclaim entries older than RATE_TTL_MS regardless of whether the tick's own fetch succeeded. Note: the cache in question (computedRateCache) is an in-memory Map, not Redis — issue Betta-Pay#615 described this as a missing Redis EXPIRE, but there is no Redis-backed rate cache in this service to apply one to. This fixes the underlying unbounded-growth risk the issue was really pointing at, on the cache that actually exists.
…bhooks (Betta-Pay#614) POST /api/admin/webhooks/dead-letter/:id/replay re-enqueued the job with only url, event, and signingSecret, dropping job.data.headers — the merchant-configured Authorization / X-Idempotency-Key headers that travel with every webhook delivery from dispatchPendingWebhookDeliveries onward and get carried into the DLQ job via the "failed" handler's `{ ...job.data, ... }` spread. A delivery that lands in the DLQ after exhausting retries and then gets manually replayed would arrive at the merchant without its auth header and get rejected. Note: issue Betta-Pay#614 pointed at services/settlement-engine's own retry-a-settlement route, but that path already copies webhookHeaders onto the retried Settlement row and rebuilds the delivery from it (Betta-Pay#569) — it was already correct. The actual gap was here, in the indexer's separate IndexedEventWebhookDelivery/DLQ replay path. Added dlq-replay-headers.test.ts (mirrors the source-text-assertion style already used by replay.test.ts for this same route-in-closure code) asserting the re-enqueue forwards headers alongside the other fields.
…ries (Betta-Pay#613) PATCH and DELETE /api/admin/assets/:code already wrote an AuditLog row on every change (asset.updated / asset.deleted, gated behind serviceAuth) — issue Betta-Pay#613 described the audit trail as entirely missing, which wasn't accurate. The real gap: both routes always logged `{ before: null, after: ... }`, so forensics could see the new state but never what changed — e.g. whether isActive actually flipped true -> false, or what a deleted row's fields were. - PATCH now does a findUnique before the update and logs it as `before`. - DELETE now logs prisma's own delete() return value (the deleted row) as `before`, instead of null — no extra query needed. Also fixes a build-breaking bug unrelated to Betta-Pay#613 that blocked testing it: normalizeAndValidateEmail (added in 269addb, Betta-Pay#468) was declared with a top-level `export` while physically nested inside buildApp()'s body, which is invalid syntax (TS1184 "Modifiers cannot appear here") and left the whole service unable to build or even parse under ts-node/esm. google-oauth-domain.test.ts already imports it as a named export from './index.js', so the fix hoists the function to true module top level (it only touches its own params and the module-level `z` import, no closure state) rather than just dropping `export`. Added supported-asset-audit.test.ts covering both routes' before/after diff and the GET /api/admin/audit-log listing.
…-Pay#612) IndexedEvent.stellarId is nullable (Soroban contract events don't always carry one), and the composite unique index was a plain @@unique([stellarId, contractId, ledger]). Postgres treats NULL as distinct from NULL, so that index never rejected two rows that both had stellarId = NULL for the same (contractId, ledger) — the indexer could ingest the same stellarId-less event twice and the existing P2002-based dedup guard in persistEvent() never fired for this case. Migration 20260901000000_dedupe_null_stellarid_indexed_events: - Deletes existing offenders first (keeps the earliest row per (contractId, ledger, stellarId IS NULL) group by indexedAt/id, removes the rest — safe, since IndexedEventWebhookDelivery. indexedEventId is a plain string column with no FK constraint). - Recreates the index with NULLS NOT DISTINCT (Postgres 15+; this repo runs postgres:16-alpine), which does what the plain composite unique index could not. The Prisma schema DSL can't express NULLS NOT DISTINCT, so schema.prisma keeps @@unique([stellarId, contractId, ledger]) as-is with a comment pointing at the migration — same pattern already used in this repo for other DB-level-only changes (see the encrypt_signing_secrets and *_comment migrations). No application code changes needed: persistEvent()'s existing P2002 catch-and-skip now also covers this case for free. Verified locally against a scratch postgres:16-compatible instance (Homebrew postgres 16.14): pre-existing duplicate null-stellarId rows are deduplicated by the migration without touching unrelated rows, a fresh duplicate insert is rejected by the new index, and `prisma migrate diff --to-migrations prisma/migrations --exit-code` reports no difference both immediately after applying and after simulating the CI rollback-and-reapply job — the same checks .github/workflows/ci.yml runs.
|
@praizehimm Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
Fixes four issues, each verified against actual current code/behavior (not just the issue text) before implementing:
#615 — FX engine rate refresh writes to Redis without a TTL so stale rates persist after upstream outage
resolveRate()'s computed pair-rate cache (computedRateCache) is an in-memoryMap, not Redis — there's no Redis-backed rate cache infx-engineto add anEXPIREto. The read path already refuses to serve an expired entry (now - computedAt >= ttlMstriggers a live recompute). What was actually missing: nothing ever removed an expired entry, so during a prolonged upstream outage (refresh keeps failing,updateBaseRates()'scomputedRateCache.clear()never runs) the map grows unbounded — every pair ever queried sits in memory forever. AddedpruneExpiredComputedRates(), run once per refresh tick, to reclaim entries pastRATE_TTL_MSregardless of whether that tick's fetch succeeded.#614 — Settlement webhook retry does not preserve custom headers from original webhookHeaders
The file/route the issue pointed at (
services/settlement-engine's settlement-retry route) already copieswebhookHeadersonto the retriedSettlementrow and rebuilds the delivery from it (fixed previously under #569) — that path was already correct. The real gap was in a different service:services/indexer'sPOST /api/admin/webhooks/dead-letter/:id/replayre-enqueued a dead-lettered delivery with onlyurl,event, andsigningSecret, droppingjob.data.headers— the merchant'sAuthorization/X-Idempotency-Keyheaders that travel with every delivery fromdispatchPendingWebhookDeliveriesonward. A delivery that exhausts retries, lands in the DLQ, and gets manually replayed would arrive at the merchant without its auth header and get rejected — the exact failure mode #614 described, just in the indexer's DLQ path rather than settlement-engine.#613 — SupportedAsset isActive toggle has no audit log entry
PATCH/DELETE /api/admin/assets/:codealready wrote anAuditLogrow on every change (asset.updated/asset.deleted, gated behindserviceAuth) — audit logging wasn't missing. The actual gap: both routes always logged{ before: null, after: ... }, so forensics could see the new state but never what changed (e.g. whetherisActiveactually flippedtrue → false). PATCH now does afindUniquebefore the update and logs it asbefore; DELETE now logs Prisma's owndelete()return value (the deleted row) instead ofnull, at no extra query cost.While verifying this one, found and fixed an unrelated build-breaking bug:
normalizeAndValidateEmail(added in 269addb, #468) was declared with a top-levelexportwhile physically nested insidebuildApp()'s body — invalid syntax (TS1184: Modifiers cannot appear here) that left the whole service unable to build or even parse underts-node/esm.google-oauth-domain.test.tsalready imports it as a named export from./index.js, so the fix hoists the function to true module top level rather than just droppingexport.#612 — Prisma IndexedEvent unique constraint on (stellarId, contractId, ledger) allows null stellarId duplicates
Confirmed as described: Postgres treats
NULL <> NULL, so the plain@@unique([stellarId, contractId, ledger])never rejected two stellarId-less rows sharing a(contractId, ledger). New migration deduplicates existing offenders (keeps the earliest row per group) and recreates the index withNULLS NOT DISTINCT(Postgres 15+; this repo runspostgres:16-alpine). Prisma's schema DSL can't expressNULLS NOT DISTINCT, soschema.prismakeeps the@@uniqueas-is with a comment pointing at the migration — the same pattern this repo already uses for other DB-only changes (encrypt_signing_secrets, the*_commentmigrations). No application code changes needed:persistEvent()'s existingP2002catch-and-skip now covers this case for free.Test plan
fx-computed-cache-ttl.test.ts(new) — expired entries evicted, fresh entries kept, boundary case, unbounded-growth scenario reclaimeddlq-replay-headers.test.ts(new) — DLQ replay forwardsheadersalongsideurl/event/signingSecretsupported-asset-audit.test.ts(new) — PATCH/DELETE log real before/after diffs, visible viaGET /api/admin/audit-log?entityType=SupportedAssetpostgres:16-compatible instance: pre-existing duplicate null-stellarIdrows deduplicated without touching unrelated rows; fresh duplicate insert rejected by the new index;prisma migrate diff --to-migrations prisma/migrations --exit-codereports no difference both immediately after applying and after simulating the CI rollback-and-reapply job (same checks.github/workflows/ci.ymlruns)tsc --noEmitconfirms theTS1184build-breaker is gone in api-gatewayCloses #612
Closes #613
Closes #614
Closes #615