fix(accounting): log designed-transient BankTxConsumer states at verbose instead of error#4293
Conversation
…ose instead of error Two designed-transient states in the ledger BankTxConsumer spammed the error logs on every scan cycle: - a fresh BUY_CRYPTO / BUY_CRYPTO_RETURN bank_tx scanned before the async pricing job has set amountInChf on the linked buy_crypto row — now thrown as LedgerGateBlockedException, logged verbose by the forward loop and the content-change scan (which already handles it since #4262) - a bank_tx whose type is still null (not yet classified) hitting the default switch branch — now logged verbose; a NON-null unhandled type stays at error (genuine enum gap, fail-loud) Retry and watermark semantics are unchanged: the gate-block still breaks the forward loop head-of-line (pricing does not bump bank_tx.updated, so that retry is the only booking path), and the null-type skip still advances the watermark (classification bumps updated, the content-change scan re-books). Extends the quiet-retry pattern established in #4262 for the sibling consumers.
…keep fail-loud defaults Narrow the previous commit to the states that provably self-heal: - BUY_CRYPTO: gate-block only while the linked buy_crypto has no amlCheck result yet (doAmlCheck prices amlCheck==null rows). A linked row WITH an amlCheck result and no amountInChf (FAIL via chargebackFillUp or resetAmlCheck) is never re-priced — that stays a fail-loud Error. - BUY_CRYPTO_RETURN: gate-block only the unlinked link race (the chargebackFillUp cron links the return bank_tx). A LINKED chargeback with no amountInChf is permanently unpriced — fail-loud Error. - Revert the type-null default-branch downgrade: reconcileBooking no-ops on never-booked rows, so a skipped null-type row is never booked later — the gap must stay visible at error level for all unhandled types. The forward-loop catch keeps logging LedgerGateBlockedException at verbose; retry and watermark semantics are unchanged throughout. A two-run test pins the head-of-line-retry recovery the gate-block relies on.
… in spec - forward-loop catch comment now names both gate-block feeders (AML pricing pending, chargeback link pending) instead of only the unpriced linked row - drop the resetAmlCheck attribution from the fail-loud comments: it nulls amlCheck and amountInChf together, so it produces the gate-block state, never the fail-loud state - spec uses CheckStatus.FAIL instead of a string literal
- the exception doc now names both forward-path feeders precisely (BUY_CRYPTO linked but not yet AML-priced; BUY_CRYPTO_RETURN not yet linked to its chargeback) instead of 'linked row' - the type-null test comment no longer attributes reconcileBooking to the forward loop: the watermark passed the row and the content-change scan's reconcileBooking no-ops on never-booked rows
- 'is never re-priced' overgeneralized: a manual FAIL→PENDING re-trigger (allowed while the buy_crypto is not complete) re-prices via doAmlCheck's PENDING selector — the comments now say 'not re-priced by the cron' - wrap the forward-catch and exception doc comments to the 120-column convention
…pricing claim The RETURN-side guard comment still carried the categorical 'FAIL rows are never re-priced' phrase that the previous commit corrected on the BUY_CRYPTO side. It now names the actual freezing mechanism: the linking cron completes the row, freezing amlCheck — so a linked chargeback with no CHF is not re-priced by the cron. The mirrored spec comment is aligned the same way.
|
Ready after 6 review passes. Fixed along the way: narrowed the gate-block predicates to provably transient states (AML pricing pending / chargeback link pending), reverted an initially proposed type-null log downgrade after establishing the content-change scan cannot re-book forward-skipped rows, plus several comment-accuracy corrections. |
ReviewThe intent is good — the two genuine happy-race states keep the watermark + retry and other exceptions stay at [high]
Suggested: bind the verbose gate to an explicit "pricing-pending" invariant defined together with (These are static derivations — I couldn't run the suite or a real Postgres, so worth confirming against the actual eligibility logic, but the risk is concrete.) |
…evert the RETURN-leg downgrade BUY_CRYPTO: amlCheck == null alone does not prove the pricing race - doAmlCheck only prices rows that also have amlReason null, input amount/asset set, no user chargeback date and isComplete false. An ineligible unpriced row would wedge the forward watermark at verbose. The gate now uses BuyCrypto.isAmlPricingPending, a mirror of the doAmlCheck selection, with reciprocal keep-in-sync comments. BUY_CRYPTO_RETURN: buyCryptoChargeback == null does not prove the fillUp link race - a bank_tx can be classified BUY_CRYPTO_RETURN without any chargeback ever linking, and chargebackFillUp only links FAIL/incomplete orders with an executed chargebackOutput. No cheap provable invariant exists on the consumer side, so the downgrade is reverted: the leg fails loud again for both sub-cases.
|
Review addressed in 63ee52e: the gate now uses a new BuyCrypto.isAmlPricingPending getter mirroring doAmlCheck's selection branch exactly (with a reciprocal keep-in-sync note at the query), so only rows the AML cron will still price gate-block; every other unpriced state stays a fail-loud error. The BUY_CRYPTO_RETURN downgrade is reverted entirely — an admin-classified return can stay unlinked forever, so unlinked is not provably transient. Negative tests cover each ineligible dimension plus the unlinked return; the getter truth table is pinned on a real entity instance. PR body updated to the final state. |
|
Reviewed — LGTM to merge. Only the designed-transient states are downgraded to verbose, genuine errors stay at One nit: the getter/query sync is enforced by comment only — no test would catch them drifting apart. Not a blocker. |
TaprootFreak
left a comment
TaprootFreak
left a comment
There was a problem hiding this comment.
LGTM. The nit above is non-blocking.
2 participants
Problem
A designed-transient state in the ledger
BankTxConsumerwas logged at ERROR on every cron cycle although it self-heals:AML pricing race: a fresh
BUY_CRYPTObank_tx is scanned beforedoAmlCheckhas priced the linked buy_crypto (amountInChfstill null). Resolves within minutes once pricing lands.Change
The guard now throws the typed
LedgerGateBlockedExceptioninstead of a plainError, and the forward-loop catch logs a gate-block at verbose (Booking gate-blocked on bank_tx N) while every other throw stays at ERROR. The content-change scan already handles this exception type at verbose since #4262 and needs no change.The gate covers only the provably transient predicate:
BUY_CRYPTO: linked andBuyCrypto.isAmlPricingPending— a new entity getter mirroringdoAmlCheck's selection branch exactly (amlChecknull,amlReasonnull, input fields present,chargebackAllowedDateUsernull, not complete), with a reciprocal keep-in-sync comment at thedoAmlCheckquery. A row withamlCheck == nullthat is NOT indoAmlCheck's selection (e.g.amlReasonset, chargeback allowed, completed) stays unpriced forever and keeps its fail-loudError.BUY_CRYPTO_RETURN: unchanged from develop — every unpriced state stays a fail-loudError. An earlier revision gate-blocked the unlinked state as achargebackFillUplink race, but an admin-classified return can legitimately stay unlinked forever (generic type-set path inupdateInternalcreates no chargeback), so "unlinked" is not provably transient; review round 2 reverted it.An unclassified bank_tx (
typenull) deliberately keeps its ERROR: the content-change scan cannot fresh-book a row the forward watermark already passed (reverseAndRebookIfChangedno-ops when nothing is booked), so that error flags a genuinely lost booking, not noise.Why retry semantics are untouched
A gate-block still
breaks the forward loop and leaves the watermark unchanged. Pricing lands on the buy_crypto table and does not bumpbank_tx.updated, so the head-of-line retry in the forward loop is the only path that books the row — the throw+break mechanism is preserved exactly; only the exception type and log level change.Tests
amlReasonset /chargebackAllowedDateUserset /isComplete/inputAmountnull) → ERROR, watermark held (parameterized). Getter truth table pinned on a realBuyCryptoinstance (base + all six single-dimension flips).BUY_CRYPTO→ ERROR. UnlinkedBUY_CRYPTO_RETURN→ ERROR.typenull and unmapped types → ERROR retained, skip-and-advance unchanged.