review_version guard is check-then-write and races

claude-bot commented

2026-07-28 06:00:25 +00:00

Severity: HIGH

The bug

The optimistic concurrency guard is check-then-write, with the check performed in Python:

# backend/app/api/routes/photos.py:75-78
def _conflict_check(photo, expected):
    if photo.review_version != expected:
        raise HTTPException(409, ...)

then later photo.review_version += 1 (repositories/photos.py:101-104) and a commit in the
route. Two requests can both read version 3, both pass the check, and both commit 4 — the second
silently overwriting the first. SQLite WAL serializes the writes but not the read-check-write
window.

The mechanism that exists specifically to prevent silent overwrite does not prevent it.

Two secondary gaps

POST /api/evidence/{id}/supersede (photos.py:333-355) takes no review_version at all and
bumps nothing, despite mutating reviewer-visible state.
apply_evidence_added (services/projections.py:53-64) bumps the version only when the photo
was pending. So evidence changes on an approved or needs-review photo can never produce a
conflict for a reviewer mid-decision — but docs/circa-spec.md §11.1 lists evidence among the
things a stale write must detect.

Fix

Either map review_version to SQLAlchemy's version_id_col on Photo (raises StaleDataError,
which maps to the 409), or use an explicit conditional update and check the row count:

UPDATE photo SET review_version = :v + 1 WHERE id = :id AND review_version = :v

Make every photo-state mutation bump the version, including supersede and all evidence
appends.

Done when

Concurrent writes at the same starting version produce exactly one success and one 409
The guard cannot be bypassed by omitting review_version
Supersede participates in the conflict protocol
Evidence appends bump the version regardless of prior status
A test performs a genuine two-session interleaving, not just a stale-value replay

References

backend/app/api/routes/photos.py:75-78
backend/app/repositories/photos.py:101-104
backend/app/services/projections.py:53-64
docs/circa-spec.md §11.1

Related: #9 tests the 409 path; extend it to cover this race.

## Severity: HIGH ## The bug The optimistic concurrency guard is check-then-write, with the check performed in Python: ```python # backend/app/api/routes/photos.py:75-78 def _conflict_check(photo, expected): if photo.review_version != expected: raise HTTPException(409, ...) ``` then later `photo.review_version += 1` (`repositories/photos.py:101-104`) and a commit in the route. Two requests can both read version 3, both pass the check, and both commit 4 — the second silently overwriting the first. SQLite WAL serializes the *writes* but not the read-check-write window. **The mechanism that exists specifically to prevent silent overwrite does not prevent it.** ## Two secondary gaps - `POST /api/evidence/{id}/supersede` (`photos.py:333-355`) takes no `review_version` at all and bumps nothing, despite mutating reviewer-visible state. - `apply_evidence_added` (`services/projections.py:53-64`) bumps the version only when the photo was `pending`. So evidence changes on an approved or needs-review photo can never produce a conflict for a reviewer mid-decision — but `docs/circa-spec.md` §11.1 lists evidence among the things a stale write must detect. ## Fix Either map `review_version` to SQLAlchemy's `version_id_col` on `Photo` (raises `StaleDataError`, which maps to the 409), or use an explicit conditional update and check the row count: ```sql UPDATE photo SET review_version = :v + 1 WHERE id = :id AND review_version = :v ``` Make **every** photo-state mutation bump the version, including supersede and all evidence appends. ## Done when - [ ] Concurrent writes at the same starting version produce exactly one success and one 409 - [ ] The guard cannot be bypassed by omitting `review_version` - [ ] Supersede participates in the conflict protocol - [ ] Evidence appends bump the version regardless of prior status - [ ] A test performs a genuine two-session interleaving, not just a stale-value replay ## References - `backend/app/api/routes/photos.py:75-78` - `backend/app/repositories/photos.py:101-104` - `backend/app/services/projections.py:53-64` - `docs/circa-spec.md` §11.1 Related: #9 tests the 409 path; extend it to cover this race.

claude-bot added this to the v0.2.0 milestone

2026-07-28 06:00:25 +00:00

claude-bot added the

bug

backend

labels

2026-07-28 06:00:25 +00:00

claude-bot referenced this issue

2026-07-28 06:06:11 +00:00

Business rules sit in the repository layer; introduce a ReviewService #125

claude-bot referenced this issue

2026-07-28 06:07:51 +00:00

Add optimistic-concurrency (409 review_version_conflict) tests #9

rbrooks referenced this issue from a commit

2026-08-01 05:31:06 +00:00

Make the review_version guard actually prevent overwrites

rbrooks closed this issue

2026-08-01 05:31:06 +00:00

rbrooks referenced this issue from a commit

2026-08-01 05:31:06 +00:00

Link the evidence supersession chain and fix reruns

claude-bot commented

2026-08-01 05:52:37 +00:00

Author

Done in 03dbc33.

Photo now maps review_version as SQLAlchemy's version_id_col, so every UPDATE of the row carries AND review_version = <loaded> and raises StaleDataError when nothing matched — mapped to a 409. I verified the semantics against SQLite before building on them rather than trusting recall: two sessions loading the same version, both bumping, and the second genuinely refused with the first's write intact.

Putting it on the mapper rather than in the routes is the point. The route-level _conflict_check stays — it is the fast path and the only one that can name the current version in the response — but it is no longer what makes the protocol safe. A future route that forgets to call it is still protected. There's a test for exactly that: a plain attribute write, no conflict check, no version bump, still refused. That's the property a convention can't give, and the same reasoning as making collection_id a required keyword in #70.

version_id_generator is False because the counter means "the reviewable state changed", and only the service layer knows when that's true.

Evidence appends now bump regardless of prior status. This previously only advanced when the photo was pending, so a reviewer mid-decision on an approved photo could never be told the evidentiary basis had changed underneath them — and §11.1 lists evidence among the things a stale write must detect.

Supersede joined the protocol in #70 (conflict check + version bump); the chain-linking half is #80.

All five "done when" items are covered, 20 tests in backend/tests/test_review_version.py. The headline is TestGenuineRace, which interleaves two real sessions rather than replaying a stale value — a replay only proves the Python pre-check works, which was never the broken part.

One thing found along the way, worth knowing. The API client read data.error, but FastAPI wraps HTTPException.detail, so structured errors have been arriving one level deeper than the client looked. The visible symptom is that the conflict banner has been reporting "Current version is -1" rather than a version — i.e. the 409 path has never actually worked end to end. The client now accepts either shape so this fix is real rather than theoretical; unifying the envelope server-side is still #75.

Done in 03dbc33. `Photo` now maps `review_version` as SQLAlchemy's `version_id_col`, so every `UPDATE` of the row carries `AND review_version = <loaded>` and raises `StaleDataError` when nothing matched — mapped to a 409. I verified the semantics against SQLite before building on them rather than trusting recall: two sessions loading the same version, both bumping, and the second genuinely refused with the first's write intact. **Putting it on the mapper rather than in the routes is the point.** The route-level `_conflict_check` stays — it is the fast path and the only one that can name the current version in the response — but it is no longer what makes the protocol safe. A future route that forgets to call it is still protected. There's a test for exactly that: a plain attribute write, no conflict check, no version bump, still refused. That's the property a convention can't give, and the same reasoning as making `collection_id` a required keyword in #70. `version_id_generator` is `False` because the counter means "the reviewable state changed", and only the service layer knows when that's true. **Evidence appends now bump regardless of prior status.** This previously only advanced when the photo was `pending`, so a reviewer mid-decision on an approved photo could never be told the evidentiary basis had changed underneath them — and §11.1 lists evidence among the things a stale write must detect. Supersede joined the protocol in #70 (conflict check + version bump); the chain-linking half is #80. All five "done when" items are covered, 20 tests in `backend/tests/test_review_version.py`. The headline is `TestGenuineRace`, which interleaves two real sessions rather than replaying a stale value — a replay only proves the Python pre-check works, which was never the broken part. **One thing found along the way, worth knowing.** The API client read `data.error`, but FastAPI wraps `HTTPException.detail`, so structured errors have been arriving one level deeper than the client looked. The visible symptom is that the conflict banner has been reporting **"Current version is -1"** rather than a version — i.e. the 409 path has never actually worked end to end. The client now accepts either shape so this fix is real rather than theoretical; unifying the envelope server-side is still #75.

claude-bot referenced this issue

2026-08-01 05:53:24 +00:00

skip decision overwrites the projection it should leave alone #81

claude-bot referenced this issue

2026-08-01 05:53:52 +00:00

Re-running AI/OCR after a completed job raises IntegrityError #82

rbrooks referenced this issue from a commit

2026-08-01 07:06:45 +00:00

Settle on one error shape and make it reach the client

claude-bot referenced this issue

2026-08-01 07:07:03 +00:00

Adopt FastAPI native error detail shape end to end #75