From cd0502f3bc84a3d62a50b0035b7be33867c36fe7 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Thu, 30 Jul 2026 16:58:47 -0500 Subject: [PATCH 1/2] fix(neo4j): assert background thread completion in OptimisticLockingSpec Follow-up to #16070. Copilot's review flagged that the second test's Thread.start { ... }.join(2000) can return on timeout without the background thread having actually finished, so the "same headroom rationale" comment added in #16070 was inaccurate there: the sleep could still be masking a race with thread completion, unlike the first test where the unbounded join() guarantees it. Capture the thread and assert !isAlive() after the bounded join so a slow runner fails loudly instead of silently racing the assertions that follow. Co-Authored-By: Claude Sonnet 5 --- .../gorm/tests/OptimisticLockingSpec.groovy | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy b/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy index 318bc0eb6f2..e0160d0efa3 100644 --- a/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy +++ b/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy @@ -130,17 +130,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec { when: o = OptLockNotVersioned.get(o.id) - try { - Thread.start { - OptLockNotVersioned.withNewSession { s -> - def reloaded = OptLockNotVersioned.get(o.id) - reloaded.name += ' in new session' - reloaded.save(flush: true) - } - }.join(2000) - } catch (InterruptedException e) { - // ignore + def backgroundUpdate = Thread.start { + OptLockNotVersioned.withNewSession { s -> + def reloaded = OptLockNotVersioned.get(o.id) + reloaded.name += ' in new session' + reloaded.save(flush: true) + } } + // Unlike the unbounded join() above, join(timeout) can return before the thread + // finishes; assert completion explicitly so a slow runner fails loudly instead of + // silently racing the assertions below. + backgroundUpdate.join(5000) + assert !backgroundUpdate.isAlive() // Same headroom rationale as "Test optimistic locking" above. sleep 5000 From 169193b0147747723d017b592a4584e83b3bf6f1 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Tue, 18 Aug 2026 10:12:52 -0500 Subject: [PATCH 2/2] fix(neo4j): fix cross-session commit races and de-flake OptimisticLockingSpec Both background-thread updates were never actually committed/visible: the 'version false' test read/wrote inside a bare withNewSession (no withTransaction, so the save never committed) against an uncommitted given: transaction (so the background session couldn't even see the entity), making its final assertions pass regardless of whether the concurrent write ever happened. The join(5000)/isAlive() completion check was also a false positive - a thread whose closure throws is equally not-alive. Commit the given: transaction before spawning the thread, wrap the background save in withTransaction, capture the background closure's outcome via an AtomicReference instead of trusting isAlive(), and use unbounded join() instead of a hard timeout. Add an intermediate read via a throwaway session so the background write is load-bearing for the test to pass. Replace both tests' fixed sleep(5000) heisenbug workaround with PollingConditions polling on real committed state, so a healthy run finishes in ~0.1-0.3s instead of always paying 5s, and a genuine failure gets a real diagnostic instead of racing past a guessed wait. Co-Authored-By: Claude Sonnet 5 --- .../gorm/tests/OptimisticLockingSpec.groovy | 60 ++++++++++++++----- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy b/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy index e0160d0efa3..af066d406fe 100644 --- a/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy +++ b/grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy @@ -19,6 +19,8 @@ package grails.gorm.tests +import java.util.concurrent.atomic.AtomicReference + import grails.gorm.annotation.Entity import org.grails.datastore.gorm.neo4j.Neo4jTransaction import org.grails.datastore.mapping.core.OptimisticLockingException @@ -27,6 +29,7 @@ import org.grails.datastore.mapping.transactions.SessionHolder import org.neo4j.graphdb.GraphDatabaseService import org.neo4j.graphdb.Transaction import org.springframework.transaction.support.TransactionSynchronizationManager +import spock.util.concurrent.PollingConditions /** * @author Burt Beckwith @@ -96,11 +99,18 @@ class OptimisticLockingSpec extends GormDatastoreSpec { } } }.join() - // The background thread's save is already synchronized via join() above; this sleep is - // headroom for the embedded Neo4j harness's own write durability, not thread completion. - // A noisy/loaded CI runner can push that past a couple of seconds - give it more room - // rather than risk a spurious failure (heisenbug). - sleep 5000 + // The background thread's save is already synchronized via join() above; poll (rather + // than sleep a fixed duration) until an independent session observes it, since the + // embedded Neo4j harness's own write-durability/visibility lag can outlast any fixed + // guess (heisenbug) - a noisy/loaded CI runner has been seen pushing past 2s, and this + // adapts instead of gambling on a bigger number. + new PollingConditions(timeout: 10, initialDelay: 0.1, delay: 0.2).eventually { + def observedName + OptLockVersioned.withNewSession { s -> + observedName = OptLockVersioned.get(o.id).name + } + assert observedName == 'locked in new session' + } o.name += ' in main session' def ex @@ -125,25 +135,41 @@ class OptimisticLockingSpec extends GormDatastoreSpec { given: def o = new OptLockNotVersioned(name: 'locked').save(flush: true) + session.transaction.commit() + session.transaction.nativeTransaction.close() session.clear() when: o = OptLockNotVersioned.get(o.id) + def failure = new AtomicReference() def backgroundUpdate = Thread.start { + try { + OptLockNotVersioned.withNewSession { s -> + OptLockNotVersioned.withTransaction { + def reloaded = OptLockNotVersioned.get(o.id) + assert reloaded + reloaded.name += ' in new session' + reloaded.save(flush: true) + } + } + } catch (Throwable t) { + failure.set(t) + } + } + backgroundUpdate.join() + // A thread that dies from an uncaught exception is also no longer alive, so join() + // alone can't distinguish a completed write from a crashed one; assert the captured + // outcome explicitly. + assert failure.get() == null + // Same cross-session visibility-lag rationale as "Test optimistic locking" above. + def nameAfterBackgroundUpdate + new PollingConditions(timeout: 10, initialDelay: 0.1, delay: 0.2).eventually { OptLockNotVersioned.withNewSession { s -> - def reloaded = OptLockNotVersioned.get(o.id) - reloaded.name += ' in new session' - reloaded.save(flush: true) + nameAfterBackgroundUpdate = OptLockNotVersioned.get(o.id).name } + assert nameAfterBackgroundUpdate == 'locked in new session' } - // Unlike the unbounded join() above, join(timeout) can return before the thread - // finishes; assert completion explicitly so a slow runner fails loudly instead of - // silently racing the assertions below. - backgroundUpdate.join(5000) - assert !backgroundUpdate.isAlive() - // Same headroom rationale as "Test optimistic locking" above. - sleep 5000 o.name += ' in main session' def ex @@ -159,6 +185,10 @@ class OptimisticLockingSpec extends GormDatastoreSpec { o = OptLockNotVersioned.get(o.id) then: + // Proves the background write actually landed before the main session's blind + // overwrite below; without it, these assertions would pass even if the background + // thread never ran. + nameAfterBackgroundUpdate == 'locked in new session' ex == null o.name == 'locked in main session' }