commit 285d3cb452836dfb025cfa1ea9d4e6971082f930 equwal <truex@equwal.com> 2026-09-26 22:18:14 -0700 Make the caption credit atomic in the database captions.add read seconds_left with the ORM and wrote back the sum. On Postgres (burst mode, READ COMMITTED), another transaction can commit between that read and that write. Then a credit or a captions.spend of that transaction for the same account is lost. Two credits for an account without a row can also both insert the row, and one of them fails with a unique violation. captions.add now uses two statements. INSERT ... ON CONFLICT DO NOTHING makes a missing row without a race. UPDATE ... SET seconds_left = seconds_left + n adds the seconds, as captions.spend and captions.refund do. The ORM UPDATE also keeps a loaded row in the session up to date, as in captions.spend. The test suite runs on SQLite, so the tests cannot show the race on Postgres. SQLite has one writer at a time, and each caller of captions.add on main writes before the call (payments.fulfil adds the purchase, accounts.merge moves the jobs), so production on SQLite is safe today. The new Hypothesis test makes the Postgres order of events on SQLite: it calls captions.add in a new transaction, and another session gives or spends seconds just before the first write of captions.add. Without the fix, Hypothesis finds two failures: a lost credit, and a unique violation when the row is missing. A lost spend fails the same assertion as a lost credit.
backend/captions.py | 27 ++++++++++++++++++----- tests/test_captions.py | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/backend/captions.py b/backend/captions.py index da43c0d..e6297a6 100644 --- a/backend/captions.py +++ b/backend/captions.py @@ -41,6 +41,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query, Request from fastapi.concurrency import run_in_threadpool from pydantic import BaseModel from sqlalchemy import ForeignKey, Integer, String, update +from sqlalchemy.dialects import postgresql, sqlite from sqlalchemy.orm import Mapped, Session, mapped_column from .api import get_account, get_session @@ -60,6 +61,9 @@ PACKS: list[Plan] = [ ] HOURS = {"captions20": 20, "captions80": 80, "captions200": 200} +# INSERT ... ON CONFLICT is not standard SQL, so SQLAlchemy has one for each database. +_INSERT = {"postgresql": postgresql.insert, "sqlite": sqlite.insert} + class CaptionBalance(Base): """The seconds of cloud captions that an account has.""" @@ -83,12 +87,23 @@ def seconds_left(session: Session, account_id: str) -> int: def add(session: Session, account_id: str, seconds: int) -> None: - """Give `seconds` to the account. The caller commits.""" - row = session.get(CaptionBalance, account_id) - if row is None: - row = CaptionBalance(account_id=account_id, seconds_left=0, seconds_used=0) - session.add(row) - row.seconds_left += seconds + """Give `seconds` to the account. The caller commits. + + The database does each step in one statement, as in spend: the INSERT + makes a missing row, and the UPDATE adds to the row. A sum made in Python + could lose a credit or a spend of another transaction on Postgres. + """ + insert = _INSERT[session.get_bind().dialect.name] + session.execute( + insert(CaptionBalance) + .values(account_id=account_id, seconds_left=0, seconds_used=0) + .on_conflict_do_nothing(index_elements=[CaptionBalance.account_id]) + ) + session.execute( + update(CaptionBalance) + .where(CaptionBalance.account_id == account_id) + .values(seconds_left=CaptionBalance.seconds_left + seconds) + ) def spend(session: Session, account_id: str, seconds: int) -> bool: diff --git a/tests/test_captions.py b/tests/test_captions.py index 75cd38f..0ebd0fa 100644 --- a/tests/test_captions.py +++ b/tests/test_captions.py @@ -4,12 +4,14 @@ from __future__ import annotations import io import wave +from contextlib import contextmanager import pytest import stripe from hypothesis import given from hypothesis import settings as hsettings from hypothesis import strategies as st +from sqlalchemy import event from backend import accounts, captions from backend.db import Account, SessionLocal @@ -168,6 +170,64 @@ def test_the_balance_never_goes_below_zero_and_no_second_is_lost(given_seconds, assert row.seconds_left + row.seconds_used == given_seconds +@contextmanager +def before_the_first_write(action): + """Run `action` in another session, and commit it, just before the first + INSERT or UPDATE of caption_balances. + + Postgres lets another transaction commit between a SELECT and a later + write of a transaction (READ COMMITTED). SQLite lets it only while the + transaction has not written yet, so a test must call add in a new + transaction to see that race on SQLite. + """ + engine = SessionLocal.kw["bind"] + results = [] + + def before(conn, cursor, statement, parameters, context, executemany): + if not results and statement.startswith( + ("INSERT INTO caption_balances", "UPDATE caption_balances")): + results.append(None) # the other session writes too + with SessionLocal() as other: + results[0] = action(other) + other.commit() + + event.listen(engine, "before_cursor_execute", before) + try: + yield results + finally: + event.remove(engine, "before_cursor_execute", before) + + +@hsettings(max_examples=50, deadline=None) +@given(start=st.none() | st.integers(0, 100), ours=st.integers(1, 100), + theirs=st.integers(1, 100), spend=st.booleans()) +def test_a_credit_loses_no_credit_or_spend_of_another_session(start, ours, theirs, spend): + """Another session gives or spends `theirs` seconds while add gives `ours`. + With `start` None, the account has no row yet.""" + with SessionLocal() as s: + acct = accounts.new_account(s).id + s.commit() + if start is not None: + captions.add(s, acct, start) + s.commit() + + def other(session): + if spend: + return captions.spend(session, acct, theirs) + return captions.add(session, acct, theirs) + + with SessionLocal() as s, before_the_first_write(other) as done: + captions.add(s, acct, ours) + s.commit() + + assert done + spent = theirs if spend and done == [True] else 0 + credited = (start or 0) + ours + (0 if spend else theirs) + with SessionLocal() as s: + row = s.get(captions.CaptionBalance, acct) + assert (row.seconds_left, row.seconds_used) == (credited - spent, spent) + + @given(st.binary(max_size=4 * SECOND).filter(lambda b: len(b) % 2 == 0)) def test_wav_holds_the_pcm_unchanged(pcm): with wave.open(io.BytesIO(captions.wav(pcm))) as w: