fix: settle the move collision check against concurrent moves

[dmccoystephenson]

Summary

• PUT /api/v1/locations/{locationId}/entity/{entityId}/move refused a move into an occupied location by reading occupancy and then writing on the strength of that read, with no boundary between the two. Two moves into the same empty location both read it empty and both committed, so the collision the endpoint's 409 exists to prevent happened anyway (moveEntityToLocation's collision check is a read-then-write race the database cannot settle #203).
• The target location's own row is now locked before its occupancy is read, and the move runs inside a transaction so the lock is held until the write is committed. A second move into the same location waits, then reads the placement the first one left behind and is answered with the 409 it would have received had the two requests arrived in sequence.
• The location row is what gets locked, rather than whatever occupies it, because an empty location has no occupancy rows to lock and emptiness is exactly the state the check depends on. Nothing in the schema settles this the way the primary key on entity_location.entity_id settles the placement race in Report the duplicate-placement loser in addEntityToLocation as 409, not 500 #200: a location is permitted to hold several entities, and addEntityToLocation places one without consulting occupancy at all. The question of whether one-entity-per-location should become a real invariant — option 1 of moveEntityToLocation's collision check is a read-then-write race the database cannot settle #203 — is left untouched by this change, which only makes the rule this one endpoint already applies hold under concurrency; it is carried forward as One entity per location is enforced only by the move endpoint, so a concurrent placement still walks around it #213 so that closing moveEntityToLocation's collision check is a read-then-write race the database cannot settle #203 does not lose it.
• The entity's own placement is locked before the target location, because that is the order EnvironmentController.deleteEnvironment acquires the same two locks in — it clears entity_location before deleting the locations — and the reverse order would let a move and a cascade delete wait on each other in a cycle. Locking the placement first also keeps the position the grid and adjacency checks are made against from moving underneath them.
• DELETE /api/v1/locations/{locationId}/entity/{entityId} answered 500 when the entity named was not placed at the location named, because no row was affected and the failure was reported as a ServiceException. It now answers 404, matching the sibling DELETE /api/v1/locations/entity/{entityId}, which already handled the equivalent case that way (Removing an entity that is not at the given location answers 500, not 404 #210). Its check runs under the same placement lock and transaction, so two removals of the same placement at once do not reintroduce the 500 through the back door.
• The sibling DELETE /api/v1/locations/entity/{entityId} was found during review to have kept exactly the unguarded check-then-act that Removing an entity that is not at the given location answers 500, not 404 #210 removes from the other one: two removals of the same placement both read it present, and the one that wrote second matched no rows and was answered 500 for having lost a race. It now takes the same placement lock inside the same transaction. The lock also makes its preceding read redundant, since a statement that locks no row is precisely the unplaced entity its 404 already reports, so no response of that endpoint changes and its contract is untouched.
• DbInteractions.lock was added for the locking statements. The existing queryOne catches a SQLException, logs it, and returns an empty Optional, which would have made a lock timeout or a broken deadlock indistinguishable from a row that does not exist — and so would have answered 404 for a location that plainly exists. The new method reports the failure as CannotAcquireLockException instead, in the manner updateReportingDuplicateKey already reports a duplicate key.
• The OpenAPI spec and docs/MVP.md are updated for both endpoints whose behaviour changed, and the Python client's message for the delete's 404 no longer names only the location, since that answer now covers the placement too.

Left for a follow-up

• Nothing bounds how long a request waits for one of the new locks: the pool size is HikariCP's default and no lock_timeout is set. That is a deployment-configuration decision that interacts with the cascade delete's statement-per-row loop, so it is filed as Nothing bounds how long a request waits for a row lock, and the pool is not sized against it #212 rather than settled here.
• The move's lock excludes other moves and nothing else, so a concurrent placement can still put a second entity in a location a move has claimed. Deciding whether one-entity-per-location is an invariant of the data or a rule of one endpoint changes the API's contract, so it is filed as One entity per location is enforced only by the move endpoint, so a concurrent placement still walks around it #213 rather than settled here.

Deferred this cycle

The remaining open issues were not picked up alongside this work, and the reasons are recorded here rather than as comments on each:

• Nothing bounds how long a request waits for a row lock, and the pool is not sized against it #212 and One entity per location is enforced only by the move endpoint, so a concurrent placement still walks around it #213 — both are follow-ups this branch created, and both are deliberately left open, for the reasons given above.
• The Python client cannot be installed or imported as a package; its modules import through the repository path #209 (the Python client cannot be installed as a package) — packaging work touching requirements.txt and the client's import paths, unrelated to the concurrency of these two endpoints and better kept out of a fix that has to stay reviewable.
• Wire OpenAPI-first codegen / spec-drift guard (rebuild-plan hygiene gap) #130 (OpenAPI-first codegen / spec-drift guard) — build and CI tooling rather than service behaviour, and large enough to warrant a cycle of its own.

Test plan

• mvn test -B — 443 tests, 0 failures (./mvnw is not usable in this environment; the system Maven was used, and CI runs ./mvnw test -B against the same suite).
• pytest and python3 -m pytest — 108 passed on both interpreters available here.
• CI is green on the head of this branch: build, python-client (3.8), and python-client (3.12) all pass.
• ConcurrentMoveTest was confirmed to catch the defect rather than merely to pass: it fails both with FOR UPDATE removed from the locking statement and with the lock moved to after the occupancy read, and passes only with the lock in place.
• The removal fix was confirmed the same way: with the controller change stashed, three of the four removeEntityFromCurrentLocation cases fail, and all pass with it restored.
• TransactionBoundaryWiringTest covers that all three endpoints' @Transactional is genuinely applied at runtime, since the locks are worthless without it.
• DbInteractionsTest covers that a locking statement reports whether it matched a row and reports a failed statement rather than returning false.
• New LocationControllerTest cases cover the lock ordering on both the move and the removals, a target deleted between the two reads, a placement that cannot be read back after being locked, and both shapes of the delete's new 404.

Closes #203
Closes #210

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Self-review

The diff was reviewed against this repository's conventions, and the branch was re-run in full afterwards: mvn test -B gives 441 tests with 0 failures, and python3 -m pytest gives 108 passed. Four findings were raised; three have been fixed on this branch and one has been filed as a follow-up.

The new race test was checked for teeth rather than assumed to have them. Two mutations were tried: removing FOR UPDATE from the locking statement, and moving the lock to after the occupancy read. ConcurrentMoveTest fails under both and passes only with the lock in place, so it is a regression test rather than a test that happens to pass.

Fixed in c846d38

• src/main/java/preponderous/viron/repositories/LocationRepositoryImpl.java:114 — both lock helpers derived their boolean from DbInteractions.queryOne, which catches a SQLException, logs it, and returns an empty Optional. A lock timeout, a deadlock the database had broken, or a serialization failure was therefore indistinguishable from a row that does not exist, and the controller answered 404 "Location not found with id: N" for a location that plainly exists — telling the client a resource is gone when the truth was that it was momentarily unavailable. Precisely the contention this PR introduces waiting for is what would have triggered it. A DbInteractions.lock was added that reports the failure as CannotAcquireLockException instead, in the manner updateReportingDuplicateKey already reports a duplicate key, and both helpers now go through it.
• src/main/java/preponderous/viron/controllers/LocationController.java:139 — the new placement check in removeEntityFromLocation was itself a check-then-act with nothing held between the two steps. Two removals of the same placement at once both passed the check; the second matched no rows and was answered with the 500 that Removing an entity that is not at the given location answers 500, not 404 #210 set out to remove. The placement is now locked first, inside a transaction, exactly as the move does it, and TransactionBoundaryWiringTest covers that the boundary is genuinely applied.
• src/test/java/preponderous/viron/controllers/LocationControllerTest.java:690 — moveEntityToLocation_EntityNotPlaced stubbed only findByEntityId, a stub that had become dead: the unstubbed lockPlacementOfEntity returned false and the controller threw before findByEntityId was reached, so the test passed for a different reason than it documented and the orElseThrow fallback had no coverage at all. The test now stubs the lock explicitly and asserts that nothing further is read, and a second case covers the fallback.

Fixed in dfc9c0c, before this review

• The target location was originally locked without the entity's placement being locked first, which put the move's two locks in the opposite order to EnvironmentController.deleteEnvironment — it clears entity_location before deleting the locations. A move and a cascade delete touching the same rows could then have waited on each other in a cycle. The placement is now locked first, and the ordering is asserted in moveEntityToLocation_LocksTheTargetBeforeReadingItsOccupancy. EntityRepositoryImpl.deleteById was checked as well and takes the same order.

Filed rather than fixed

• src/main/java/preponderous/viron/config/DataSourceConfig.java:37 — nothing bounds how long a request waits for one of the new locks. maximumPoolSize is left at HikariCP's default of 10, and neither the properties nor the JDBC URL sets lock_timeout or statement_timeout, whose Postgres defaults are to wait indefinitely. A waiter holds a pooled connection for the whole wait, so sufficient contention drains the pool and unrelated endpoints begin failing. This is a deployment-configuration decision, and a blanket statement_timeout would interact with the cascade delete's statement-per-row loop, so it is filed as Nothing bounds how long a request waits for a row lock, and the pool is not sized against it #212 rather than settled here.

Checked and found sound

• Both new statements are parameterized, and no JDBC resource is opened outside DbInteractions.
• CannotAcquireLockException is a Spring DataAccessException and reaches GlobalExceptionHandler's catch-all as a 500; whether it deserves a 409 or 503 of its own is part of Nothing bounds how long a request waits for a row lock, and the pool is not sized against it #212 rather than of this change.
• The fix depends on the default READ COMMITTED isolation, under which the waiter's occupancy read takes a fresh snapshot and sees the winner's committed placement. No isolation level is set anywhere in the service, so that default holds.
• No endpoint's path, request shape, or response shape changed, so OpenApiSpecDriftTest remains green; the spec and docs/MVP.md were updated for the descriptions and the delete's new 404.
• The Python client needed no contract change, since it already treated this delete's 404 as an error; only its message was corrected, as that message now covers the placement as well as the location.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

[dmccoystephenson]

Review of the whole branch (second pass)

The full diff was re-read against this repository's conventions in a later session, after the previous one was interrupted mid-cycle. The rubric is scored below against the diff and against command output rather than against judgement, and two findings were raised that the first pass did not: one has been fixed on this branch and one has been filed.

The external anchor is genuinely green this time. The run on c846d38 was recorded as a failure with all three of its jobs still queued after thirty minutes — an abandoned run rather than a verdict on the diff — and rerunning it was not available to this session, so the fix below was pushed and CI ran clean on 8e1f4c6: build, python-client (3.8), and python-client (3.12) all pass. Locally, mvn test -B gives 443 tests with 0 failures and pytest gives 108 passed on both interpreters present here.

Findings

Fixed in 8e1f4c6

src/main/java/preponderous/viron/controllers/LocationController.java:157 — removeEntityFromCurrentLocation kept exactly the shape #210 removes from its sibling. It read the placement, found it present, and then deleted on the strength of that read, with no lock and no transaction around the pair. Two removals of the same placement therefore both passed the check; the one that wrote second matched no rows, removeEntityFromCurrentLocation returned false, and the request was answered with the same ServiceException-backed 500 that this PR exists to stop reporting for a removal that merely lost a race. The endpoint mattered here in particular because the javadoc this PR adds to the other removal cites it as the sibling that "already answers the equivalent case the same way" — true of the 404 for an unplaced entity, but not of the concurrent case. It now takes lockPlacementOfEntity inside a @Transactional boundary, in the same order and the same manner as its sibling. The preceding findByEntityId was dropped rather than kept, because a locking statement that matches no row is precisely the unplaced entity the 404 reports, so a second read would only widen the window it was there to close. No status code, message, or body of that endpoint changes, so the OpenAPI spec, docs/MVP.md, and the Python client need no amendment for it.

The fix was checked for teeth rather than assumed to have them: with the controller change stashed, three of the four removeEntityFromCurrentLocation cases fail, and all four pass with it restored.

Filed rather than fixed

src/main/java/preponderous/viron/controllers/LocationController.java:85 — the lock this PR adds excludes other moves and nothing else. addEntityToLocation neither consults the target's occupancy nor takes its lock, so a placement and a move aimed at the same empty location can still both succeed and leave two entities there. The PR body already says the invariant question is left open as option 1 of #203, but #203 is closed by this merge, which would have taken the open question with it. It is now #213, with the three concurrent pairs tabulated and the two coherent answers set out. This is not a regression — the state has always been one the schema permits — but the move's documented 409 reads as a guarantee about the location when it is only a guarantee about other moves.

Rubric

• Scope: PASS — all thirteen modified files belong to the move race (moveEntityToLocation's collision check is a read-then-write race the database cannot settle #203), the removal's status code (Removing an entity that is not at the given location answers 500, not 404 #210), or the removal-endpoint consistency finding above; no unrelated formatting or renames appear in git diff origin/main...HEAD.
• Tests-new: PASS — lockPlacementOfEntity, lockLocation, and DbInteractions.lock each have direct cases in LocationRepositoryImplTest and DbInteractionsTest; both new private helpers are exercised through the controller cases.
• Tests-fix: PASS — established empirically, not by reasoning. ConcurrentMoveTest fails with FOR UPDATE removed and with the lock moved after the occupancy read; the removeEntityFromCurrentLocation cases fail with the controller change stashed.
• Sibling structure: PASS — ConcurrentMoveTest follows ConcurrentPlacementTest's H2-per-test shape, and the new repository methods sit with their neighbours in LocationRepositoryImpl.
• Sibling renames: PASS — no identifier in a parallel series was renamed; the two new NotFoundException factories are additions.
• Docs: PASS — the spec and docs/MVP.md carry both changed endpoints; docs/PLANNING.md, tickets.md, README.md, and the Postman collection describe these endpoints only at a level that remains accurate, and were re-checked line by line.
• Issue resolution: PASS — moveEntityToLocation's collision check is a read-then-write race the database cannot settle #203's acceptance is met by ConcurrentMoveTest, and Removing an entity that is not at the given location answers 500, not 404 #210's by the two new 404 cases; neither is claimed while partially resolved.
• CI: PASS — green on 8e1f4c6, all three jobs.
• DTO boundary: PASS — no controller method in the diff returns Entity, Environment, Grid, or Location; the two changed endpoints return 204 No Content.
• Spec alignment: PASS — no path, verb, parameter, or body shape changed. The spec edits are descriptions and the delete's 404 wording, and OpenApiSpecDriftTest is green.
• Java/Python parallelism: PASS — the only client-visible change is the delete's 404 message, which locationService.py and its test carry.
• Override correctness: PASS — every @Override in the diff (LocationRepositoryImpl's two new methods, LockRacer.lockLocation) matches a real interface or superclass method, which the compile step confirms.
• Scope ceiling: PASS — 147 net non-test lines across seven non-test files (git diff --stat origin/main...HEAD -- src/main/java src/main/python docs), well inside the ceiling; the remaining 491 of the diff's 645 added lines are tests.

Observations outside the diff

• tickets.md:165 still lists "Adding entity to new location removes from old location" as acceptance criteria for the location endpoints. addEntityToLocation has since been changed to answer a conflict for an entity placed elsewhere, so that line describes behaviour the service no longer has. It is a record of a completed ticket rather than live documentation, and it predates this branch, so it is left alone here.
• CannotAcquireLockException reaches GlobalExceptionHandler's catch-all as a 500. Whether contention deserves a 503 or a 409 of its own belongs with Nothing bounds how long a request waits for a row lock, and the pool is not sized against it #212, where the timeouts that would produce it are being decided.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Merge readiness — held for the codeowner

Everything this PR needs has been done, and it is being left open on one gate rather than on any remaining work.

Green. CI passes on 8e1f4c6 — build, python-client (3.8), and python-client (3.12). Locally, mvn test -B gives 443 tests with 0 failures and pytest gives 108 passed. The branch is MERGEABLE and CLEAN against main. The regression gate is met for both bug fixes, and both regression tests were confirmed to fail without their fix rather than merely to pass with it. The documentation sources of truth were re-checked line by line against the implementation.

The gate. One modified path is on this repository's do-not-auto-merge list, so autonomous merge is withheld:

• docs/openapi/viron-api.json — the API contract.

What the spec change amounts to is worth stating plainly, since the hold is on the path rather than on the size of the edit. Four lines move: two description fields are added, and two response descriptions are reworded. No path, verb, parameter, request body, response schema, or status code is added or removed, and OpenApiSpecDriftTest is green. What does change behind it is observable, however, and is the reason a human should look: DELETE /api/v1/locations/{locationId}/entity/{entityId} answers 404 where it previously answered 500 for an entity that is not placed at the location named (#210).

No other protected path is touched — no workflow, no pom.xml, no requirements.txt, no deployment or database bootstrap file — and no file in the diff deletes more than six lines.

What is being asked. A codeowner's approval of the contract wording and of the 500 → 404 change on that endpoint. With that, the PR can be squashed as it stands; nothing further is outstanding on the branch.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson