Vault balance update is a read-modify-write race: concurrent deposits lose funds #421

Labels / Complexity: bug, financial, database, Backend, reliability · Extremely High — 500

Problem

DepositDBConnector.add_vault_balance in quantara/web_app/db/crud/deposit.py is a non-atomic read-modify-write:

vault = self.get_vault(wallet_id, symbol)        # 1. read current amount
...
new_amount = Decimal(vault.amount) + Decimal(amount)   # 2. add in Python
db.query(Vault).filter_by(id=vault.id).update(amount=str(new_amount))  # 3. write back
db.commit()

Two concurrent requests for the same (wallet_id, symbol) both read the same vault.amount, each compute old + their_amount, and the second write overwrites the first — a lost update. On a money ledger this silently discards one of the deposits.

A related defect is in the data model and the deposit path: create_vault (quantara/web_app/db/crud/deposit.py) always inserts a new Vault row on every /api/vault/deposit, and the Vault table (quantara/web_app/db/models.py) has no unique constraint on (user_id, symbol). get_vault returns .first(), so once more than one row exists for a user+symbol, add_vault_balance updates an arbitrary row while others accumulate stale balances — the total is never the sum the user actually deposited.

The codebase already contains the correct pattern for exactly this situation: add_extra_deposit_to_position in quantara/web_app/db/crud/position.py uses a PostgreSQL insert(...).on_conflict_do_update(...) with cast(amount, Numeric) + cast(amount, Numeric) to atomically upsert. The vault path does not reuse it.

Root cause

vault = self.get_vault(wallet_id, symbol)          # ← read outside the write transaction
new_amount = Decimal(vault.amount) + Decimal(amount)  # ← lost-update window
db.query(Vault).filter_by(id=vault.id).update(amount=str(new_amount))

Why this is architecturally hard

1. The shortcut — SELECT ... FOR UPDATE — requires a single transaction that spans the read and the write, but get_vault opens its own Session and closes it before returning, so the contributor must restructure the method into one session/transaction rather than bolting a lock onto the current split.
2. The deeper fix is an atomic SQL upsert (INSERT ... ON CONFLICT (user_id, symbol) DO UPDATE SET amount = amount + excluded.amount), which requires adding the missing unique constraint to the Vault model — an Alembic migration and a data-cleanup decision for any existing duplicate rows.
3. amount is stored as a String (Vault.amount and Position.amount are String), so arithmetic must go through Decimal/NUMERIC casting inside SQL, exactly as add_extra_deposit_to_position already does — the contributor must reconcile string storage with atomic numeric addition.
4. create_vault and add_vault_balance are two divergent write paths (one inserts, one updates-first-row); unifying them into one upsert is the real fix, and the API endpoints in quantara/web_app/api/vault.py must both route through it.

Proposed design

-- one atomic upsert, mirroring ExtraDeposit's pattern
INSERT INTO vault (user_id, symbol, amount)
VALUES (:user_id, :symbol, :amount)
ON CONFLICT (user_id, symbol)
DO UPDATE SET amount = cast(vault.amount AS numeric) + cast(EXCLUDED.amount AS numeric);

The maintainer decision is whether to add a unique constraint on (user_id, symbol) and migrate existing rows, or to key the upsert by an explicit natural key.

Downstream impact

Adding a unique constraint on (user_id, symbol) requires an Alembic migration (quantara/web_app/alembic/) and reconciliation of any existing duplicate Vault rows. The response shape of /api/vault/deposit and /api/vault/add_balance need not change.

Acceptance criteria

Database

• A unique constraint (or equivalent natural key) exists on (user_id, symbol) for Vault.
• add_vault_balance (or its replacement) performs the increment atomically in SQL.

Service

• Concurrent increments to the same (wallet_id, symbol) are not lost: N concurrent deposits of X produce a final balance of N*X.
• create_vault and add_vault_balance converge on a single upsert path.

Tests

• A concurrency test (or a transaction-isolation test) demonstrates no lost update.
• Tests run via cd quantara && poetry run pytest web_app/tests -k vault.

Out of scope

Do not migrate Position.amount/ExtraDeposit.amount storage in this issue; only fix the Vault balance path.

Getting started

Files in scope: quantara/web_app/db/crud/deposit.py, quantara/web_app/db/models.py, quantara/web_app/api/vault.py, plus an Alembic migration. Verify with:

cd quantara && poetry run pytest web_app/tests -k vault

Good first files to read: quantara/web_app/db/crud/deposit.py, quantara/web_app/db/crud/position.py (the on_conflict_do_update pattern), quantara/web_app/db/models.py.