Summary
LocationController.addEntityToLocation (src/main/java/preponderous/viron/controllers/LocationController.java:76-95) reads the location, the entity and the current placement before inserting. Two concurrent requests for the same unplaced entity can both observe no placement and both attempt the insert. The composite primary key on viron.entity_location still holds, so no corruption results, but the losing request receives a 500 where the guard immediately above it is meant to produce a 409.
This is step 3 of #194. Steps 1 (connection pooling, #198) and 2 (transaction boundaries, #199) are complete; this was split out of #199 rather than bundled with it.
Why a transaction boundary alone does not close it
Wrapping the method in a transaction was considered while #199 was written and rejected as insufficient:
- Under the default READ COMMITTED isolation, two transactions can still each read no placement and then race on the insert. A boundary changes when the loser fails, not whether it fails.
SELECT ... FOR UPDATE on the placement does not help either: there is no row to lock when the entity is unplaced, which is precisely the case that races.- On Postgres a unique-key violation aborts the whole transaction, so re-reading the placement afterwards to decide "this was a conflict, not an error" fails inside the same boundary.
Why it is not a small change
DbInteractions.update (src/main/java/preponderous/viron/database/DbInteractions.java:126-137) logs the SQLException and returns false. A duplicate-key violation is therefore indistinguishable from any other write failure by the time the controller sees it, which is what forces the 500. Reporting a 409 needs the constraint violation to be distinguishable, and each of the available routes has a cost worth weighing before one is picked:
- Have
update surface the SQLException (or a translated DataAccessException). This is the most faithful option but changes the error contract for every repository call in the service, so it needs its own review.
- Make the insert conditional in SQL —
INSERT ... ON CONFLICT DO NOTHING — so the losing request affects no rows and returns false without aborting the transaction. The controller can then re-read the placement and answer 409. This is narrower, but it moves the invariant into Postgres-specific SQL.
- Translate at the boundary only, by catching the violation in the repository method that owns the insert.
Acceptance
- Two concurrent placements of the same unplaced entity leave exactly one placed, and the loser receives 409 with a message naming the location the entity ended up at.
- A genuine write failure is still reported as 500, not misreported as a conflict.
- The behaviour is covered by a test that exercises the concurrent path rather than only the sequential guard.
This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).
drafted by Claude on behalf of Daniel Stephenson
Summary
LocationController.addEntityToLocation(src/main/java/preponderous/viron/controllers/LocationController.java:76-95) reads the location, the entity and the current placement before inserting. Two concurrent requests for the same unplaced entity can both observe no placement and both attempt the insert. The composite primary key onviron.entity_locationstill holds, so no corruption results, but the losing request receives a 500 where the guard immediately above it is meant to produce a 409.This is step 3 of #194. Steps 1 (connection pooling, #198) and 2 (transaction boundaries, #199) are complete; this was split out of #199 rather than bundled with it.
Why a transaction boundary alone does not close it
Wrapping the method in a transaction was considered while #199 was written and rejected as insufficient:
SELECT ... FOR UPDATEon the placement does not help either: there is no row to lock when the entity is unplaced, which is precisely the case that races.Why it is not a small change
DbInteractions.update(src/main/java/preponderous/viron/database/DbInteractions.java:126-137) logs theSQLExceptionand returnsfalse. A duplicate-key violation is therefore indistinguishable from any other write failure by the time the controller sees it, which is what forces the 500. Reporting a 409 needs the constraint violation to be distinguishable, and each of the available routes has a cost worth weighing before one is picked:updatesurface theSQLException(or a translatedDataAccessException). This is the most faithful option but changes the error contract for every repository call in the service, so it needs its own review.INSERT ... ON CONFLICT DO NOTHING— so the losing request affects no rows and returnsfalsewithout aborting the transaction. The controller can then re-read the placement and answer 409. This is narrower, but it moves the invariant into Postgres-specific SQL.Acceptance
This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).
drafted by Claude on behalf of Daniel Stephenson