fix(insurance): atomic payouts and transactional multi-tier coverage - #20
Merged
ameeribro4-sudo merged 2 commits intoAug 20, 2026
Conversation
…sactional Payouts previously used a lock-free read-modify-write, so concurrent liquidations could pass the balance check twice and double-spend a fund. recordTransaction now applies every balance change as a single conditional UPDATE (balance = balance - :amount WHERE balance >= :amount) with an affected-row check, and coverShortfall debits tiers in fixed order inside one transaction so concurrent liquidations serialize on row locks instead of deadlocking. Partial coverage is reported explicitly (status PARTIAL + cascadePrevented: false event) instead of being swallowed by catch-all error handling; health and events run only after commit. Co-Authored-By: Xhristin3 <208627422+Xhristin3@users.noreply.github.com>
6 tasks
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
Closes #13
Insurance fund payouts can no longer double-spend a balance.
recordTransaction()applies every payout as a single conditionalUPDATE insurance_funds SET balance = balance - :amount WHERE id = :id AND balance >= :amountand rejects the call when zero rows are affected, andcoverShortfall()debits tiers in fixed priority order inside oneDataSourcetransaction, so concurrent liquidations serialize on fund row locks instead of racing a read-modify-write. The single most important design decision: the issue offered two acceptable primitives (pessimistic_writelocks vs an atomic conditional UPDATE) — this PR uses the atomic UPDATE, because it is driver-agnostic (SQLite — the repo's test harness — rejectspessimistic_write), never holds a row lock across the Stellar network call, and is exactly the primitive the two concurrent-payout acceptance criteria demand.Why
The old path did
getFund()→ checkbalance >= amountin application memory →fund.balance = balanceAfter; save(fund)— a full-row overwrite with no lock and no transaction. Two concurrent liquidations (the engine polls every 5s) could both read the same balance, both pass the check, and both overwrite it, paying out the same funds twice.coverShortfall()made it worse by wrapping each payout intry { ... } catch { continue; }, silently turningInsufficient fund balance for payoutinto a partially-covered liquidation with no signal. This fix closes the race at the database (the guard is evaluated under the row lock) and converts partial coverage into an explicit outcome (event + status field), and it deliberately reuses the existing margin-module discipline (dataSource.transaction(...)) rather than introducing a second mechanism.What was built
src/protection/services/:insurance-fund.service.tsbalance -/+ :amountwith an affected-row check), a manager-awarerecordTransaction(fundId, type, amount, options, manager?)that joins an outer transaction when one is passed and otherwise runs the debit + audit row + health refresh in a transaction of its own, plusgetFund(fundId, manager?)for race retries. Matching tests:insurance-fund.service.spec.ts.liquidation-protection.service.tscoverShortfall()rewritten: advisory fund reads before the transaction, per-tier guarded debits with one bounded retry after a re-read of the committed balance, single-transaction persistence of the LiquidationEvent, post-commit health recalculation and events, and a narrowNotFoundExceptionskip for uninitialized tiers (everything else propagates). Matching tests:liquidation-protection.service.spec.ts.New tests:
insurance-fund-concurrency.spec.tsBadRequestException+ balance 500 + exactly one payout record, and that two concurrentcoverShortfall(5000)calls against 10000 total capacity both succeed with no deadlock and no negative balance.Integration changes outside
services/src/protection/insurance-fund.integration.spec.ts— rewritten from hand-rolled in-memory mocks to a real SQLite DataSource (TypeOrmModule), preserving the lifecycle and health-alert tests and adding a real-DB partial-coverage test. This was unavoidable: the old mock harness could not exercise real transactions or the atomic UPDATE path.README.md— new "Insurance fund concurrency model" section documenting where locks live and why (per the issue's documentation criterion).src/margin/services/liquidation-engine.service.ts— listed in the issue's "files in scope" but intentionally untouched: it already logs coverage failures, and none of the acceptance criteria require engine changes.Acceptance criteria coverage
Service
insurance-fund-concurrency.spec.ts— double-spend test; the same test fails against the base implementation, where both payouts succeed)coverShortfalldebits multiple tiers atomically and does not deadlock under concurrent liquidations. (singledataSource.transaction+ fixedTIER_PRIORITYlock order;insurance-fund-concurrency.spec.ts— concurrent coverShortfall test with both calls covered, balances ending at 0, none negative)status: 'PARTIAL'+liquidation.shortfallevent withcascadePrevented: false; the blanketcatch { continue; }is replaced by a narrowNotFoundExceptionskip — every other failure propagates)Tests
recordTransactionpayouts against the same fund and asserts the fund is never double-spent. (insurance-fund-concurrency.spec.ts— balance 1500, two concurrent 1000 payouts, asserts exactly one success, one rejection, final balance 500, one payout record)insurance-fund.integration.spec.tsreal-DB test: LOW 3000 + MEDIUM 1000, 10000 shortfall → covered 4000, remaining 6000, PARTIAL, events emitted; plus the mock-suite insufficient-payout rejection test)Documentation
README.md— "Insurance fund concurrency model"; also doc-comments onrecordTransactionandcoverShortfall)Deliberately deferred
liquidatePosition()committing andcoverShortfall()running leaves a liquidated position without coverage and no retry path. Closing that requires either moving coverage inside the margin position transaction or a durable reconciliation job — a stateful change to the margin module that none of this issue's acceptance criteria require. The atomic, transactional coverage built here is the prerequisite. Happy to align on the approach (same-transaction vs reconciliation) before a follow-up PR.Test plan
npm run build— succeedsnpm test— 454/506 passing, 52 failing; the 52 failing are byte-identical to the base branch (pre-existing ts-jest resolution of absolutesrc/...imports — verified by diffing the failing-suite lists with the changes stashed). 4 net new tests added and passing.npx eslinton changed files — strictly fewer errors than base:insurance-fund.service.spec.ts13→11,liquidation-protection.service.spec.ts2→2,insurance-fund.integration.spec.ts32→6, new concurrency spec 0, both service files 0npx madge --circular— same 3 pre-existing cycles as base, none involving protectionnpx jest src/protection— 20/20 passing (incl. concurrency suite run 5× for timing stability)Env vars / Notes
No new env vars or config keys. The concurrency spec writes a throwaway SQLite file under
os.tmpdir()and deletes it inafterAll. One sqlite-specific finding worth recording: a read inside an open transaction before any write makes SQLite returnSQLITE_BUSYimmediately when another writer is active (both rollback-journal and WAL);coverShortfalltherefore performs its advisory reads before the transaction, which also matches Postgres' MVCC behavior. Postgres row locks make the same interleaving safe in production.