Multi-statement writes are not atomic; a shared JDBC connection blocks adding transactions

[dmccoystephenson]

Summary

Several write paths in this service issue multiple dependent SQL statements that are not
atomic. A failure partway through leaves the database in a state that no single request can
produce deliberately, and no rollback occurs.

This was observed while reviewing PR #193 and is filed for visibility rather than as a
regression from that change — the gap predates it.

Where this occurs

• EntityRepositoryImpl.deleteById (src/main/java/preponderous/viron/repositories/EntityRepositoryImpl.java:97-98)
  clears the entity's viron.entity_location row and then deletes the entity. If the second
  statement fails, the entity survives but has silently lost its placement.
• EnvironmentController.deleteEnvironment (src/main/java/preponderous/viron/controllers/EnvironmentController.java:87-125)
  performs a much longer cascade — association rows, then entities, then locations, then
  grids — throwing ServiceException from the middle of the loop on the first failure.
  Every deletion already performed is retained, leaving a partially deleted environment.
• LocationController.addEntityToLocation (src/main/java/preponderous/viron/controllers/LocationController.java:78-94)
  reads the location, the entity and the current placement before writing. 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 enforces the invariant,
  so no corruption results, but the loser receives a 500 rather than the 409 the guard is
  meant to produce.

Why this is not a one-line fix

@Transactional is not used anywhere in the codebase, and no transaction manager is
configured. Adding one is complicated by how DbInteractions is built
(src/main/java/preponderous/viron/database/DbInteractions.java:26-33): a single
java.sql.Connection is opened in the constructor and held as state on an application-scoped
@Component, and setAutoCommit(false) is never called.

That single shared connection means transaction boundaries cannot simply be switched on.
Calling setAutoCommit(false) on a connection shared by every concurrent request would
enclose unrelated requests in the same transaction, so one request's rollback would discard
another's committed-looking work. The shared mutable connection is also a thread-safety
concern in its own right, since a JDBC Connection is not required to be thread-safe.

Proposed work

The prerequisite should be addressed first, then the boundaries:

1. DbInteractions should obtain connections from a pooled DataSource (Spring Boot
   auto-configures HikariCP when spring-boot-starter-jdbc is present) rather than holding
   one long-lived connection, so each request gets its own.
2. A transaction manager should be configured and the multi-statement paths above annotated
   @Transactional, so a partial cascade rolls back.
3. Once the placement insert and its guard run in one transaction, the race in
   addEntityToLocation can be closed and the duplicate-placement loser can be reported as a
   409 rather than a 500.

Steps 1 and 2 are worth splitting into separate changes if the connection-management rework
proves large.

Notes

No data corruption is reachable through the API today — the schema constraints hold in every
case examined. The impact is partial deletes that require manual cleanup, and a misleading
500 on a narrow concurrent path.

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

[dmccoystephenson]

Partial progress in PR #198: the prerequisite described under "Proposed work" step 1 has landed.

DbInteractions no longer holds a single java.sql.Connection as state on an application-scoped
component. Connections now come from a HikariCP pool built from DbConfig in the new
config/DataSourceConfig, and each query/queryOne/update borrows and releases one through
DataSourceUtils — so a later @Transactional boundary will be joined by these calls without
this class needing to change again. The unused connect() and close() methods were removed.
The thread-safety concern raised in the issue body is resolved by that change; connection release
on every path (including the error paths) and concurrent callers each completing their own write
are both covered by new tests in DbInteractionsTest.

One side effect is worth recording: introducing a DataSource bean makes Actuator register a
database health contributor, which would have flipped /actuator/health to DOWN during a
database outage. It was disabled (management.health.db.enabled=false, commented in
application.properties) so the endpoint keeps its current liveness meaning rather than acquiring
readiness semantics as a side effect of an unrelated change. Enabling it deliberately is a
reasonable separate decision.

Still open, and why this issue is not closed:

• Step 2 — transaction boundaries on the multi-statement write paths. This is more than an
  annotation: EnvironmentController.deleteEnvironment performs its cascade in the controller,
  and the services/ package in this repository holds the outbound HTTP client SDKs rather than
  business logic, so there is no service layer to annotate. Extracting the cascade is the bulk of
  the work. DbInteractions.update also swallows SQLException and returns false, so rollback
  depends on callers continuing to throw ServiceException on a false return.
• Step 3 — reporting the duplicate-placement loser in LocationController.addEntityToLocation as
  a 409 rather than a 500, which the issue notes depends on step 2.

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

[dmccoystephenson]

Step 2 is addressed by #199: a DataSourceTransactionManager is declared alongside the pool, and the environment cascade delete, the entity delete and environment creation now run inside transaction boundaries, so a failure partway through rolls the sequence back.

Step 3 — reporting the duplicate-placement loser in addEntityToLocation as a 409 rather than a 500 — was split out into #200, since a boundary alone does not close that race and distinguishing a unique-key violation requires DbInteractions.update to stop swallowing the SQLException. The reasoning is recorded there.

This issue is left open until #200 is resolved.

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

---

drafted by Claude on behalf of Daniel Stephenson