fix(neo4j): assert background thread completion in OptimisticLockingSpec - #16072
fix(neo4j): assert background thread completion in OptimisticLockingSpec#16072borinquenkid wants to merge 1 commit into
Conversation
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens the OptimisticLockingSpec Neo4j test to avoid silently continuing after a timed Thread.join(timeout) that may return before the background write completes, making slow CI runners fail deterministically instead of racing subsequent assertions.
Changes:
- Capture the background update thread in
Test optimistic locking disabled with 'version false'. - Replace
Thread.start { ... }.join(2000)with a stored thread,join(5000), and an explicit!isAlive()completion assertion. - Stop swallowing potential
InterruptedExceptionfrom the join path (now an interrupt will fail the test rather than masking it).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/gorm-registry-core-impl #16072 +/- ##
======================================================================
+ Coverage 52.1558% 52.1662% +0.0104%
- Complexity 18408 18412 +4
======================================================================
Files 2054 2054
Lines 96622 96622
Branches 16846 16846
======================================================================
+ Hits 50394 50404 +10
+ Misses 38835 38825 -10
Partials 7393 7393 🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: cd0502f Learn more about TestLens at testlens.app. |
| // finishes; assert completion explicitly so a slow runner fails loudly instead of | ||
| // silently racing the assertions below. | ||
| backgroundUpdate.join(5000) | ||
| assert !backgroundUpdate.isAlive() |
There was a problem hiding this comment.
assert !backgroundUpdate.isAlive() verifies termination, not completion. A thread whose closure throws is equally not-alive (the default uncaught-exception handler just prints to stderr), so this assert passes when the background update crashed - the same silent outcome the comment says it prevents. And that crash path is live here (see the notes on lines 135 and 137). To actually assert completion, capture the closure's outcome and check it on the test thread, e.g.:
def failure = new AtomicReference<Throwable>()
def backgroundUpdate = Thread.start {
try {
OptLockNotVersioned.withNewSession {
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(5000)
assert !backgroundUpdate.isAlive() && failure.get() == nullplus the assert reloaded guard the sibling test already has (line 93).
| // ignore | ||
| def backgroundUpdate = Thread.start { | ||
| OptLockNotVersioned.withNewSession { s -> | ||
| def reloaded = OptLockNotVersioned.get(o.id) |
There was a problem hiding this comment.
This get(o.id) returns null on the background thread, so the closure dies with an NPE on the next line before ever saving - silently, since nothing observes the thread's exception. The node created in the given: block exists only inside the main session's transaction: GormDatastoreSpec.setup() calls session.beginTransaction() and nothing in this feature commits it, while the background thread gets its own bolt session/transaction that cannot see the uncommitted CREATE. The sibling test works around exactly this by committing before spawning its thread (session.transaction.commit(); session.transaction.nativeTransaction.close(), lines 74-75). Without the same commit here, the concurrent update this test is named for never happens, and the new join(5000) + isAlive() check passes regardless.
| OptLockNotVersioned.withNewSession { s -> | ||
| def reloaded = OptLockNotVersioned.get(o.id) | ||
| reloaded.name += ' in new session' | ||
| reloaded.save(flush: true) |
There was a problem hiding this comment.
Even with the visibility problem fixed, this save(flush: true) is never committed. Bare withNewSession only binds/unbinds a session; the flush runs in a lazily started default transaction (Neo4jSession.assertTransaction() -> startDefaultTransaction()), and on unbind Neo4jSession.disconnect() calls Neo4jTransaction.close(), which closes the native bolt transaction without committing - the driver rolls it back. The only committing path is Neo4jTransaction.commit(), driven by withTransaction. The sibling test wraps its background save in OptLockVersioned.withTransaction { ... } (line 91) for this reason; this closure needs the same (and the unused s -> parameter can be dropped at the same time).
| } | ||
| // 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. |
There was a problem hiding this comment.
The assertions below cannot actually race the background update - they are unfalsifiable by it. o is loaded before the thread starts (line 131, name locked), line 148 appends in main session, the save is a blind SET n += $props with no version predicate (version false), and the re-read at line 159 goes through the main session's still-open transaction, which sees its own write (and holds the node's write lock, so a late background SET cannot land between lines 151 and 159). Both ex == null and o.name == 'locked in main session' hold even if lines 133-146 are deleted. Contrast the sibling's o.name == 'locked in new session' (line 121), which does verify its background write. Re-reading in a fresh session after the join and asserting the name is locked in new session before the main save would make the background update load-bearing and turn this into a real last-write-wins test.
| // 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) |
There was a problem hiding this comment.
This converts a previously non-failing wait into a hard 5s cliff. Before, a background thread finishing in more than 2s was tolerated: join(2000) returns silently on timeout and the following sleep 5000 absorbed the overrun (~7s of effective grace). Now a single run over 5s fails at line 144, and the sleep 5000 headroom sits after the assert, so it no longer contributes anything to the completion check. With grails-data-neo4j/build.gradle setting retry { maxRetries = 2; failOnPassedAfterRetry = true }, one slow run reds the build even when the retry passes. Every equivalent test in the tree - the sibling above (line 98), the TCK copy in grails-datamapping-tck, and the hibernate5/7 variants - uses an unbounded .join(), which cannot return early and has no cliff; converging on that is both simpler and stronger. If the bounded form stays, consider a larger budget and an assert message that includes backgroundUpdate.state so a CI-only timeout is triageable.
| }.join(2000) | ||
| } catch (InterruptedException e) { | ||
| // ignore | ||
| def backgroundUpdate = Thread.start { |
There was a problem hiding this comment.
Merge hazard: feat/neo4j-gorm-registry-migration (branched from the same base commit) deletes this module and relocates this spec to grails-data-neo4j/core/src/test/groovy/grails/gorm/tests/OptimisticLockingSpec.groovy - still containing the old join(2000) block, plus a stray session. -> manager.session. substitution inside the string literals (' in new manager.session'). A git merge-tree of the two heads reports a content conflict on this exact block, and the likely resolution (taking the migrated file wholesale) silently drops this fix. Worth coordinating with that branch, and/or applying the change to the TCK copy (grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OptimisticLockingSpec.groovy, which still has .join() + sleep(2000)), which survives the migration.
| backgroundUpdate.join(5000) | ||
| assert !backgroundUpdate.isAlive() | ||
| // Same headroom rationale as "Test optimistic locking" above. | ||
| sleep 5000 |
There was a problem hiding this comment.
Optional, pre-existing: this fixed sleep 5000 (and the sibling's at line 103) costs 5s on every green run. Once the background write actually lands (see the notes above), the deterministic replacement is a spock.util.concurrent.PollingConditions poll on the observable state - already the idiom in DirtyCheckingAfterListenerSpec and HibernateUpdateFromListenerSpec, and spock-core is on this module's test classpath. It returns as soon as the write is visible and fails with the actual observed value on timeout, instead of sleeping a fixed budget and hoping.
Summary
Thread.start { ... }.join(2000)can return on timeout without the background thread having actually finished, so the "same headroom rationale" comment added in test(neo4j): give OptimisticLockingSpec's cross-thread heisenbug more headroom #16070 was inaccurate there.!isAlive()after the boundedjoin(5000)so a slow runner fails loudly instead of silently racing the assertions that follow, instead of swallowing a potentialInterruptedExceptionand hoping for the best.Test plan
grails.gorm.tests.OptimisticLockingSpecingrails-data-neo4j(could not execute this module's tests locally — it's pinned tograils-gradle-plugins:6.1.2, which 404s fromrepo.grails.organd isn't in local caches, a pre-existing environment gap unrelated to this change)🤖 Generated with Claude Code