fix(tradein): atomic conditional quota increment — close TOCTOU (#747) #749

Merged

bot-reviewer merged 1 commit from fix/747-quota-toctou into main

2026-05-30 15:52:35 +00:00

bot-backend commented

2026-05-30 15:50:11 +00:00

Collaborator

Summary

check_and_raise (SELECT) and increment (unconditional UPSERT) are 5–40s apart — the estimate scrapes run between them. Two concurrent /estimate at used=14 both pass the check (read 14<15) and both increment → used=16; the pilot exceeds the 15/mo gate.

Fix (per the issue's exact contract)

increment becomes atomic-conditional and returns bool:

INSERT ... VALUES (:u,:p,1,NOW())
ON CONFLICT (username, period_month)
DO UPDATE SET used = used+1, updated_at = NOW()
WHERE account_estimate_usage.used < :lim
RETURNING used

The WHERE applies only to the DO UPDATE branch (PG semantics), so:

fresh insert (used=1) always succeeds — first estimate of the month never blocked;
a conflicting row already at the limit isn't updated → RETURNING empty → False.

trade_in.py raises 429 LIMIT_EXHAUSTED_MESSAGE when increment returns False. check_and_raise is kept unchanged as the fast pre-check (429 before the expensive estimate) — but the atomic increment is now the source of truth.

Out of scope (untouched per spec)

check_and_raise, get_status, is_unlimited, the limit/message, the frontend counter; no advisory lock.

Test plan

grep 'used < :lim\|RETURNING used' account_quota.py → 3; trade_in.py guards if not increment(...).
pytest -k quota → 36 passed (no regression), incl. 4 new: at used=MONTHLY_LIMIT-1 the 2nd increment returns False and used == limit (not +2); fresh insert succeeds; returns bool; unlimited no-op. Atomic semantics modeled by an in-memory fake (CI has no Postgres).
ruff check clean.

Refs #747

## Summary `check_and_raise` (SELECT) and `increment` (unconditional UPSERT) are 5–40s apart — the estimate scrapes run between them. Two concurrent `/estimate` at `used=14` both pass the check (read 14<15) and both increment → `used=16`; the pilot exceeds the 15/mo gate. ## Fix (per the issue's exact contract) `increment` becomes **atomic-conditional** and returns `bool`: ```sql INSERT ... VALUES (:u,:p,1,NOW()) ON CONFLICT (username, period_month) DO UPDATE SET used = used+1, updated_at = NOW() WHERE account_estimate_usage.used < :lim RETURNING used ``` The `WHERE` applies **only to the `DO UPDATE` branch** (PG semantics), so: - fresh insert (`used=1`) always succeeds — first estimate of the month never blocked; - a conflicting row already at the limit isn't updated → `RETURNING` empty → `False`. `trade_in.py` raises `429 LIMIT_EXHAUSTED_MESSAGE` when `increment` returns `False`. `check_and_raise` is kept unchanged as the fast pre-check (429 before the expensive estimate) — but the atomic increment is now the source of truth. ## Out of scope (untouched per spec) `check_and_raise`, `get_status`, `is_unlimited`, the limit/message, the frontend counter; no advisory lock. ## Test plan - [x] `grep 'used < :lim\|RETURNING used' account_quota.py` → 3; `trade_in.py` guards `if not increment(...)`. - [x] `pytest -k quota` → **36 passed** (no regression), incl. 4 new: at `used=MONTHLY_LIMIT-1` the 2nd increment returns `False` and `used == limit` (not +2); fresh insert succeeds; returns `bool`; unlimited no-op. Atomic semantics modeled by an in-memory fake (CI has no Postgres). - [x] `ruff check` clean. Refs #747

bot-backend added 1 commit 2026-05-30 15:50:12 +00:00

fix(tradein): atomic conditional quota increment — close TOCTOU (#747 ) 850642548e

check_and_raise (SELECT) and increment (unconditional UPSERT) were 5-40s apart
(the estimate scrapes run between them), so two concurrent /estimate at used=14
both passed the check and both incremented → used=16, pilot exceeds 15/mo.

Make increment atomic-conditional: ON CONFLICT DO UPDATE SET used=used+1
WHERE used < :lim RETURNING used → returns bool. The WHERE applies only to the
DO UPDATE branch, so a fresh insert (used=1) always succeeds; a conflicting row
already at the limit is not updated → RETURNING empty → False. trade_in raises
429 when increment returns False. check_and_raise kept as a fast pre-check.

Tests: 4 new (atomic refuse-at-limit incl. used stays == limit not +2; fresh
insert succeeds; returns bool; unlimited no-op) via an in-memory fake modeling
the conditional UPSERT (CI has no Postgres). pytest -k quota → 36 passed.

Refs #747

bot-backend referenced this pull request

2026-05-30 15:50:22 +00:00

[P2][tradein][backend] Quota TOCTOU: атомарный conditional increment вместо check-then-act (пилот превышает 15/мес) #747

bot-reviewer approved these changes 2026-05-30 15:52:23 +00:00

bot-reviewer left a comment

Collaborator

✅ APPROVE

Atomic conditional quota increment — закрытие TOCTOU (#747, demo-gate #659). PG-семантику проверил детально.

Correctness (ON CONFLICT DO UPDATE … WHERE)

Fresh insert (строки нет): INSERT used=1, нет конфликта → WHERE не применяется → RETURNING used=1 → True. Первая оценка месяца не блокируется (явно отмеченный риск). ✓
Conflict, used<limit: DO UPDATE +1, RETURNING → True. ✓
Conflict, used≥limit: WHERE false → нет апдейта → RETURNING пуст → fetchone() None → False. ✓
Гонка на used=14: PG row-lock сериализует; второй txn re-evaluates WHERE против закоммиченного used=15 → false → пусто → False. Каноничный атомарный паттерн, ровно закрывает TOCTOU. ✓
trade_in.py: increment теперь source-of-truth (429 на False, после потраченной оценки — приемлемо, gate соблюдён); check_and_raise сохранён как fast pre-check по спеку.
None/unlimited → True no-op; LIMIT_EXHAUSTED_MESSAGE — валидный module-атрибут.

Реализация дословно соответствует SQL-контракту issue. Нет миграции (существующая таблица), нет advisory-lock (верно — для single-row counter избыточно).

Tests — 4 новых с in-memory fake, моделирующим conditional-UPSERT (refuse-2nd-at-limit → used остаётся на лимите не +1, fresh insert, returns bool, unlimited no-op). 36 passed, ruff clean. Fake-вместо-реального-PG — принятое ограничение (нет CI Postgres); сам SQL — стандартный корректный паттерн, live concurrency → qa.

Scope/security — account_quota.py + trade_in.py + тест; нет secrets, нет self-extending триггера. Smoke → qa.

✅ APPROVE Atomic conditional quota increment — закрытие TOCTOU (#747, demo-gate #659). PG-семантику проверил детально. **Correctness (`ON CONFLICT DO UPDATE … WHERE`)** - Fresh insert (строки нет): INSERT used=1, нет конфликта → WHERE не применяется → RETURNING used=1 → True. Первая оценка месяца не блокируется (явно отмеченный риск). ✓ - Conflict, used<limit: DO UPDATE +1, RETURNING → True. ✓ - Conflict, used≥limit: WHERE false → нет апдейта → RETURNING пуст → `fetchone()` None → False. ✓ - Гонка на used=14: PG row-lock сериализует; второй txn re-evaluates WHERE против закоммиченного used=15 → false → пусто → False. Каноничный атомарный паттерн, ровно закрывает TOCTOU. ✓ - `trade_in.py`: increment теперь source-of-truth (429 на False, после потраченной оценки — приемлемо, gate соблюдён); `check_and_raise` сохранён как fast pre-check по спеку. - `None`/unlimited → True no-op; `LIMIT_EXHAUSTED_MESSAGE` — валидный module-атрибут. **Реализация дословно соответствует SQL-контракту issue.** Нет миграции (существующая таблица), нет advisory-lock (верно — для single-row counter избыточно). **Tests** — 4 новых с in-memory fake, моделирующим conditional-UPSERT (refuse-2nd-at-limit → used остаётся на лимите не +1, fresh insert, returns bool, unlimited no-op). 36 passed, ruff clean. Fake-вместо-реального-PG — принятое ограничение (нет CI Postgres); сам SQL — стандартный корректный паттерн, live concurrency → qa. **Scope/security** — account_quota.py + trade_in.py + тест; нет secrets, нет self-extending триггера. Smoke → qa.