Add optimistic-concurrency (409 review_version_conflict) tests #9
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
Optimistic concurrency via
review_versionis the mechanism that stops two reviewerssilently overwriting each other. The plan lists silent overwrite as a top risk and asks
for 409 paths to be tested early. The UI already renders a conflict banner, but the
backend guarantee behind it is untested.
Scope
Tests proving stale writes are rejected on every mutating endpoint.
Implementation notes
PATCH notes,PATCH flags,POST decisions,POST evidence/manual,and
POST evidence/supersede: read a photo, mutate it through a second client, thensubmit the first client's now-stale
review_versionand assert 409.review_version— the frontend's "ReloadLatest" flow depends on it, so an unlabelled 409 is not enough.
no audit event. A 409 that partially applied would be worse than an overwrite.
review_versionentirely.Done when
References
backend/app/api/routes/photos.pybackend/app/services/projections.pydocs/circa-phase1-plan.mdsection 11.3docs/circa-api-spec.md(review_version_conflict)Depends on: the backend test harness.
Amended by the audit of 2026-07-28.
These tests as written would pass against a broken implementation. The
review_versionguardis check-then-write with the comparison in Python (
photos.py:75-78), then a separate incrementand commit — so two requests can both read version 3, both pass the check, and both commit 4.
A stale-value replay (read, mutate elsewhere, submit old version) catches the easy case. Add a
genuine two-session interleaving test that exercises the race window.
The enforcement fix is #77. Also note #80:
supersede_evidencetakes noreview_versionat alland bumps nothing, so it currently sits outside the concurrency protocol entirely.
Done in
c396bb6.backend/tests/test_concurrency_protocol.py, 34 cases.What was already there vs. what this adds
#77 landed the enforcement and
test_review_version.pyproves the mechanism — including the genuine two-session interleaving the audit comment above asked for. What was missing is the question this issue actually asks: does every write use it, and does a refused write really change nothing.Both are parametrized over one list of the five versioned endpoints, because "every mutating endpoint" is a claim about a set, and a set spelled out by hand is only ever true of the routes someone remembered.
Per endpoint — notes, flags, decisions, manual evidence, supersede
current_version"Exactly one" matters in both directions: advancing by two would make a concurrent reader stale for no reason, advancing by none would leave the next stale write undetectable.
A rejected write leaves no trace
Evidence, decision, note-revision and audit-event counts plus every projection field — status, notes, effective date range, precision, current decision, flags — compared before and after the 409. A 409 that partially applied would be worse than the overwrite it exists to prevent: the reviewer is told their edit was refused, and it was not. The ledger matters most, because its triggers refuse deletion (#67), so an event written by a rejected write could never be removed afterwards.
Supersede gets its own assertion for
is_active, because that is not a counted row: a supersede that took effect and then 409'd leaves every count identical and the evidence silently retired — and retired rows are still rendered, at 40% opacity, so it is easy to miss.TestNoWriteEscapesTheProtocolThe part that keeps the above true. Every write route in
ROUTE_ACCESS(from #8) must either carryreview_versionor appear in an exemption list with a reason:POST /api/auth/logout— ends the caller's own session, touches no photoPOST /api/ingest— creates a photo; there is no prior version to be stale againstPOST,DELETE) — append-only side channel, carries no date, feeds nothing intoprojections.fold; two reviewers commenting at once is a conversation, not a conflictai-rerun/ocr-rerun— enqueue only; the evidence they produce goes through the normal append path, which does bump#80 is exactly what the absence of this check looks like:
supersede_evidencemutated a photo's evidentiary basis and sat outside the protocol entirely, for as long as it existed, because nothing anywhere stated that such a write had to participate.Verified load-bearing
Disabling
_conflict_checkfails 20 of the 34 cases. The audit's warning — that these tests as written would pass against a broken implementation — was about the stale-value replay specifically; that remains true of the replay, which is why the interleaved case stays inTestGenuineRaceand this file says so where the arrival-order test could be mistaken for it.Done when
957 passed, 8 skipped; ruff clean; CI green.