Release: develop -> main#4368
Merged
Merged
Conversation
…umer batch (#4361) * fix(accounting): skip an already-booked row in the forward BankTxConsumer batch This cron re-bootstraps a fresh process every cycle. If a run's bookTx transaction commits but the process is killed before the batch's single end-of-loop watermark write persists, the watermark stays behind an already-booked row. The next run re-selects it and a raw re-insert collides on the ledger_tx UNIQUE(sourceType, sourceId, seq) constraint — forever, since nothing about that condition self-heals (observed live on bank_tx 207714, retrying every cycle since it wedged). Guard the forward booker with the existing hasAnyTxAt check (already used by the content-change scan and reversal paths for the same "was this ever booked" question) so an already-booked row is treated as done instead of re-inserted. The content-change scan remains the correctness backstop if the already-committed legs ever need correcting. * fix(accounting): check alreadyBooked before rebuilding legs, matching sibling consumers Move the crash-recovery guard to the top of book(), before buildSeq0Input, and extract it into an alreadyBooked helper — the convention already used by buy-crypto.consumer.ts and crypto-input.consumer.ts. Checking first closes a gap the previous placement left open: buildSeq0Input can itself throw (e.g. a gate-block or a missing ledger account), which would have re-wedged the watermark on an already-booked row for a different reason than the original duplicate-key failure. * test(accounting): pin the alreadyBooked-before-buildSeq0Input ordering The existing crash-recovery tests use EXTRAORDINARY_EXPENSES on valid data, for which buildSeq0Input never throws — so they pass identically regardless of whether the guard runs before or after it, giving no regression protection for the previous fix commit. Add a case where buildSeq0Input would throw (an unpriced BUY_CRYPTO outside doAmlCheck eligibility) on an already-booked row, asserting the throwing path is never reached. Verified this test fails against the prior guard placement and passes against the current one.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
July 24, 2026 13:23
) * feat(storage): fail open on the secondary store during dual-write Dual-write was fully fail-closed: a failure against either store failed the whole upload. During the migration that turns a problem in the store nobody reads from into a customer-visible upload failure, which is a self-inflicted outage for an internal concern. The rule is now asymmetric. The write against the store that is currently the read source stays fail-closed, so the guarantee that a successful upload means a readable document is preserved. The write against the other store is fail-open: the failure is logged at error level with the greppable marker DUAL_WRITE_SECONDARY_FAILURE, including container, object name, which store failed and which store is the read source, and the call succeeds. Under read source azure that means Azure must succeed and a failing MinIO write is only logged; under read source s3 it is the other way round, so the invariant holds through the whole migration rather than only in its first phase. The returned URL always points at a store that actually holds the object: it stays the canonical S3 URL when that write succeeded, and falls back to the Azure URL when the secondary S3 write failed. The divergence this admits is exactly what the mandatory reconciler gate exists for, and it must reach zero before the read source is ever flipped. copyBlobs follows the same rule, and the existing warning on differing result counts still fires. Consequence worth naming: during the azure read-source phase the Object-Lock store is the secondary one, so a failed secondary write leaves a compliance record in Azure only, without immutability, until the reconciler heals it into the locked bucket. That matches the pre-migration situation, where those records were written to Azure alone and had no Object Lock at all, and it is bounded by the gate that must be green before the read flip. * fix(storage): return the read-source result from dual copyBlobs Review of the previous commit raised three points. copyBlobs still returned the S3 result unconditionally. Under read source azure a failed secondary S3 copy set that result to an empty list, so the method reported that nothing had been copied although the objects had been copied in Azure and were readable from the read source. That broke the same return-value invariant the other two write paths had just been fixed for. The final return now follows the read source. Two pre-existing tests had encoded the old behaviour as correct and were corrected with it. The Object-Lock consequence was invisible in the code. Under read source azure the store carrying Object Lock is the secondary one, so a failed secondary write leaves the compliance record without immutability until the reconciler heals it into the locked bucket — qualitatively different from an ordinary sync failure. Both the comment and the log text for that branch now say so, with the stable marker preserved so existing alert rules keep matching. The mirrored branch notes that Azure enforces no Object Lock, so no protection is lost there. Finally, the decision to keep the object key in the failure log is now documented at the call site: the key is needed to locate and heal the affected object and may contain a user-supplied file name, while no contents, metadata or credentials are logged.
…4296) * fix(kyc): match Postgres duplicate-key error in DfxApproval recovery The graceful recovery in checkDfxApproval still checked for the MSSQL error text ('Cannot insert duplicate key'), which Postgres never emits. The catch never matched, so concurrent callers (account merge, KYC review cron, mail-confirm merge link) crashed instead of adopting the concurrent winner's OnHold step - leaving DfxApproval steps stuck OnHold indefinitely and failing the calling flows. Align the check with the 'duplicate key' pattern used by every other duplicate-key catch in this codebase. * test(kyc): pin DfxApproval recovery lookup to the account id The recovery-path assertion matched the findOneBy user filter with expect.anything(), so a regression scoping the lookup to the wrong account would still pass. Assert the concrete id instead - same-user scoping is the security property this recovery depends on. * fix(kyc): gate DfxApproval race recovery on the kyc_step unique index Recover only on SQLSTATE 23505 from the kyc_step composite unique index (name derived from entity metadata), adopt the concurrent winner's step, treat a winner already in review as a no-op and rethrow the original error when no valid winner is found.
…e outages at warn (#4298) * fix(pricing): retry transient CoinGecko connect failures and log price outages at warn A connect-level failure to CoinGecko (ETIMEDOUT/ECONNRESET, typically lasting seconds) currently produces one error log per fetch plus one more in every consumer cron that re-reports the same root cause. - CoinGeckoService: one bounded retry (2 attempts, 500 ms delay) on connection failures only; rate-limit and request errors still fail immediately - isConnectionFailure: also recognize code-only errors and multi-address AggregateErrors (empty message, system-error code on the members) - TradingRuleService/FeeService refresh crons: log typed price-source unavailability at warn - the provider already logs the outage at error and the next cycle refreshes the value; unexpected failures stay at error * fix(pricing): gate the fee-refresh warn on fee freshness and drop unreachable exception arm Review findings: PriceInvalidException also wraps permanent conditions (e.g. missing price rule), so FeeService only downgrades to warn while the stored fee is fresh (< 30 min) and escalates back to error once it goes stale. The PriceInvalidException arm in TradingRuleService was unreachable (the trading path bypasses the rule-based pricing layer) and is removed together with its test. Spec import ordering aligned. * fix(pricing): tie the fee-refresh warn window to FeeValidityMinutes The downgrade window and the stored-fee validity window are the same concept; referencing the existing constant keeps them from drifting apart. * fix(pricing): narrow the fee-refresh warn gate to transient price-source outages PricingService.getPrice wraps every failure of the price path in PriceInvalidException, so gating the fee-refresh downgrade on that type would also silence real failures (DB, config, programming errors) for up to FeeValidityMinutes. Introduce PriceUnavailableException (a PriceInvalidException subclass carrying the cause) thrown only for connection-class failures, and downgrade only that one while the stored fee is fresh. Also cover the simpleTokenPrice contract path of the CoinGecko retry. * fix(pricing): preserve provider error so outage warn actually fires The bounded CoinGecko retry was fine, but the "log price outages at warn" half never took effect and could hide DB failures: - CoinGeckoService rethrows preserving the original error's message/code/cause instead of a causeless generic exception. - Classification happens in one place only, getPriceFrom, narrowly scoped to the provider call, so DB/repository errors can no longer be mistaken for a price outage. - getAssetPrice keys on a PriceUnavailableException marker walked from the cause chain instead of blindly re-running isConnectionFailure on any error. - TradingRuleService downgrades on PriceUnavailableException, not the generic ServiceUnavailableException. - Add an end-to-end outage test through the real retry path and fix the specs that asserted the wrong downgrade-on-DB-error behaviour. * style: format new pricing outage tests with prettier * fix(pricing): classify AggregateError outages via the error cause chain A multi-address AggregateError (empty message, code only on the member errors) - the dominant CoinGecko connect-failure shape - was wrapped into a ServiceUnavailableException carrying neither a matching message nor code, so isConnectionFailure returned false and the outage still logged at ERROR. isConnectionFailure now also walks the error cause chain (cycle-guarded), so the wrapped exception is recognised through its cause and correctly downgraded to WARN. DB/repository failures stay at ERROR - getAssetPrice keeps classifying via the PriceUnavailableException marker only. Adds cause-walk unit tests and an end-to-end AggregateError outage test. --------- Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
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
2 participants
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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist