Review follow-ups: PR #516 — fix(pos): cancelling must not void a payment the buyer already made (Gitea #753) #761

New Issue

2026-09-21T02:41:06Z

gambit-admin commented

2026-09-21 02:41:06 +00:00

Non-blocking findings from the auto code review of PR #516 (fix(pos): cancelling must not void a payment the buyer already made (Gitea #753)) — captured for batch cleanup rather than blocking the PR.
Commit 99ef405 · https://github.com/Gambit-Inc/gambit/pull/516

SHOULD-FIX:

backend/app/routes/transactions.py:5422-5449 — The new guard has a TOCTOU hole that reproduces the bug it fixes.
- The guard's SELECT takes no lock. The Stripe webhook (billing.py:466, _record_paid_tender) does a bare db.get(TransactionTender, …) and flips pending to authorized. It never takes the transaction row lock that cancel_transaction holds (lock=True), so the two don't serialize.
- The guard sees the tender as pending and passes. The webhook then commits authorized. The locking tender SELECT at line 5443 still has status.in_(["pending", "authorized"]), so it picks up the freshly authorized row and voids it (line 5456). The sale is cancelled with the buyer's money captured.
- Fix: make the locked tender fetch the source of truth. Take with_for_update() on the tenders, raise 409 if any locked row is authorized, and only void pending. Alternatively, drop "authorized" from that IN list and raise if the locked set contains it.
- Nothing in the tests covers this ordering.
backend/app/routes/transactions.py:4006 — This is outside the diff but is the same money-taken/sale-dropped outcome by another route. void_tender still lets a cashier void an authorized (already captured) Stripe tender with no refund, and the tender can then be cancelled. If that is deliberate recovery/admin behaviour, it needs an explicit guard or a documented decision. Otherwise this PR only closes one of two paths to the same outcome. File it as a follow-up.
frontend/app/hq/pos/PosPage.jsx:928-929 — When cancel now returns this 409, the catch only sets the error. The 409 fires precisely when the webhook landed after cancelSale's getTransaction at line 917, so the rail still shows the stale pending Stripe tender. The message says "finish it instead", but the UI still shows the tender as unpaid. The void-tender handler (around line 899) already refetches the aggregate on error; do the same here.

NITS:
none

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #516** (fix(pos): cancelling must not void a payment the buyer already made (Gitea #753)) — captured for batch cleanup rather than blocking the PR. Commit `99ef405` · https://github.com/Gambit-Inc/gambit/pull/516 SHOULD-FIX: 1. `backend/app/routes/transactions.py:5422-5449` — The new guard has a TOCTOU hole that reproduces the bug it fixes. - The guard's SELECT takes no lock. The Stripe webhook (`billing.py:466`, `_record_paid_tender`) does a bare `db.get(TransactionTender, …)` and flips `pending` to `authorized`. It never takes the transaction row lock that `cancel_transaction` holds (`lock=True`), so the two don't serialize. - The guard sees the tender as `pending` and passes. The webhook then commits `authorized`. The locking tender SELECT at line 5443 still has `status.in_(["pending", "authorized"])`, so it picks up the freshly authorized row and voids it (line 5456). The sale is cancelled with the buyer's money captured. - Fix: make the locked tender fetch the source of truth. Take `with_for_update()` on the tenders, raise 409 if any locked row is `authorized`, and only void `pending`. Alternatively, drop `"authorized"` from that `IN` list and raise if the locked set contains it. - Nothing in the tests covers this ordering. 2. `backend/app/routes/transactions.py:4006` — This is outside the diff but is the same money-taken/sale-dropped outcome by another route. `void_tender` still lets a cashier void an `authorized` (already captured) Stripe tender with no refund, and the tender can then be cancelled. If that is deliberate recovery/admin behaviour, it needs an explicit guard or a documented decision. Otherwise this PR only closes one of two paths to the same outcome. File it as a follow-up. 3. `frontend/app/hq/pos/PosPage.jsx:928-929` — When cancel now returns this 409, the catch only sets the error. The 409 fires precisely when the webhook landed after `cancelSale`'s `getTransaction` at line 917, so the rail still shows the stale `pending` Stripe tender. The message says "finish it instead", but the UI still shows the tender as unpaid. The `void-tender` handler (around line 899) already refetches the aggregate on error; do the same here. NITS: none --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-21 02:41:06 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#761