Repository navigation
Keep the embedded MongoDB tests from hanging CI jobs for six hours - #16338
codeconsole merged 10 commits into
Conversation
An ordinary shutdown stops the embedded server twice: the lifecycle bean stops it when the application context closes, and the shutdown hook the initializer registers stops it again as the JVM exits. The flapdoodle backend already treats the second stop as nothing to do, but the in-memory backend called MongoServer.shutdownNow() every time. A second shutdownNow() can wait forever. It closes the clients mongo-java-server still tracks and waits, with no timeout, for them to close. A client that connected just as the first shutdown began can be registered on a Netty event loop after that loop has closed its channels; the loop then terminates with the client still tracked, and closing it through that loop never completes. Waiting on that from the shutdown hook, the JVM never exits. This module's own test worker hung that way on CI until GitHub cancelled the job at six hours, six times on 8.0.x between 2026-08-26 and 2026-09-11. Stopping a server that is already stopped now returns at once, and stop() and restart() are synchronized, since the context's shutdown hook and the initializer's run at the same time on the way out. A new feature runs an application in a JVM of its own that lets a client connect, stops the server as its context closes, and returns from main. Without this change that JVM never exited in 8 of 8 runs, so the feature fails after 30 seconds rather than hanging; with it the JVM exits in about a second. The application is written in Java because, started through Groovy and Spock, the same steps almost never hang even unfixed. The existing feature that stops a server twice now waits a bounded time for the second stop, so a regression fails instead of hanging the build.
Each feature closed its application context in cleanup, but a context that was never refreshed is not active, and closing a context that is not active does nothing. The four mongods the specification started were left running for the JVM shutdown hook to stop as the test worker exited, and when that hook hung they were still running when CI killed the job. Stop each server through the lifecycle bean the initializer registers, as EmbeddedMongoInitializerSpec and EmbeddedReplicaSetSpec already do.
mongodb-test-config.gradle skips its test tasks when a job selects another datastore with -PonlyHibernate5Tests, -PonlyHibernate7Tests, -PonlyRedisTests and the rest, but not with -PonlyNeo4jTests. The three Neo4j jobs therefore ran every MongoDB suite that config covers, which the five MongoDB jobs already run, and four of the six hangs of the embedded MongoDB tests on 8.0.x cancelled a Neo4j job. grails-data-mongodb-core applies mongodb-forked-test-config.gradle, whose list has the same gap, but the data TCK config already keeps that module out of the Neo4j jobs, so its list is left as it is.
A test JVM that never exits keeps its job running until GitHub cancels it at six hours, long after every other job has reported. Give each test job a timeout of twice the slowest run that finished on its own, passed or failed, over the 160 CI runs from 2026-09-03 to 2026-09-13, rounded up to the half hour: functional 150 min (slowest 63) security 150 min (slowest 71) securityConfig 90 min (slowest 39) casSecurityConfig 90 min (slowest 35) redis 180 min (slowest 86) mongodbFunctional 150 min (slowest 70) hibernate5Functional 120 min (slowest 58) hibernate7Functional 210 min (slowest 104, a failure; the slowest pass took 86) neo4jFunctional 180 min (slowest 90) The build jobs are left without one: the macOS build has passed after more than five hours.
hibernate5-test-config.gradle, hibernate7-test-config.gradle and redis-test-config.gradle skip their test tasks for every other datastore's -Ponly flag except -PonlyNeo4jTests, as mongodb-test-config.gradle did. The three Neo4j jobs therefore also ran nineteen Hibernate 5 and Hibernate 7 test tasks - the plugin, Spring Boot and database migration suites - which the Hibernate jobs already run. The core modules were already kept out by the data TCK config, and grails-redis by test-config.gradle, so the Redis list changes nothing today; it now names the Neo4j jobs itself instead of relying on another config to.
Its class comment blamed a server thread that is not a daemon: a Gradle test worker, it said, cannot exit while one runs. The worker exits through System.exit, which does not wait for such threads. It waits for every JVM shutdown hook to finish, and a hook stopping a server that hangs is what kept builds whose tests had all passed from finishing.
grails-redis applied test-config.gradle as well as redis-test-config.gradle, and a test task runs only when every config applied to it lets it. test-config.gradle skips its suites under -PonlyRedisTests, and redis-test-config.gradle skips them under -PonlyCoreTests and every other selection flag, so between the two the module's test and integrationTest tasks ran in no CI job: the Redis jobs reported only grails-test-examples-redis. Apply redis-test-config.gradle alone, as the Hibernate and MongoDB modules apply only their own config. It already forwards -PredisContainerVersion, and with it alone the tests pass against Redis 7.4 and 8.0: 4 unit and 31 integration features, besides the two RedisTagLibSpec features the spec itself skips.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16338 +/- ##
==================================================
- Coverage 55.5923% 55.5824% -0.0099%
+ Complexity 21122 21117 -5
==================================================
Files 2113 2113
Lines 101782 101784 +2
Branches 18096 18097 +1
==================================================
- Hits 56583 56574 -9
- Misses 37085 37097 +12
+ Partials 8114 8113 -1
🚀 New features to boost your workflow:
|
matrei
left a comment
There was a problem hiding this comment.
Review Findings
Head 38f32adee7 on 8.0.x base 3067d0a855 (the current tip of 8.0.x, so the merge is clean). Seven non-merge commits, each one a separate concern: the in-memory double-stop fix, the replica set spec cleanup, the -PonlyNeo4jTests gaps in three test configs, job timeouts, a corrected class comment, and grails-redis dropping test-config.gradle.
Verdict: approve. The backend fix is right, small and matches what the flapdoodle backend already does. One finding worth acting on before merge is about what the new regression test actually proves; the rest are small.
Verified on this head (JDK 21, Linux):
| Run | Result |
|---|---|
:grails-data-mongodb-embedded:test :grails-data-mongodb-embedded:codeStyle --rerun-tasks |
51 tests, 0 failures, 0 skipped; codeStyle clean |
:grails-redis:check -PonlyRedisTests -PredisContainerVersion=8.0 --rerun-tasks |
test 4/0/0, integrationTest 33 run, 0 failed, 2 skipped (the two @Ignored RedisTagLibSpec features) |
Control: InMemoryMongoBackend.java restored to the base version, the two double-stop features re-run |
see finding 1 |
The grails-redis figures match the commit message, and on the base both tasks were indeed skipped under -PonlyRedisTests because test-config.gradle lists that flag. I also confirmed that no Neo4j module applies hibernate5-test-config, hibernate7-test-config, mongodb-test-config or redis-test-config, so adding onlyNeo4jTests to those lists cannot skip anything the Neo4j jobs are meant to run.
Findings
1. Medium: the out-of-process regression test does not reproduce the hang here, and the in-process one does
ContextStoppedServerApplication and the first commit message say the child JVM "never exited in 8 of 8 runs" without the fix, and that "started through Groovy and Spock, the same steps almost never hang even unfixed". On this machine it is the other way round. With InMemoryMongoBackend.java restored to the base version:
stopping a server that is already stopped is what a shutdown hook does FAIL 30.6 s (TimeoutException from the bounded second stop)
an application whose context stopped its in-memory server still exits ok 1.6 s
and running the child application by hand against the unfixed classes, eight times in a row:
run 1..8 exit=0 1255-1273 ms
So the race that leaves a client channel tracked on a terminated event loop is timing-dependent, and which of the two features hits it depends on the machine. That is fine as far as the fix goes: the fix is correct regardless, and between them the two features did catch the regression here. But the class comment on ContextStoppedServerApplication and the commit message present the Java child process as the deterministic reproducer and the Spock feature as the one that "proves nothing", which a reader on a machine like this one will find contradicted.
Suggestions, none blocking:
- Soften the comment on
ContextStoppedServerApplication(and the PR description) to say the hang is a race that reproduced reliably as a separate JVM on the author's machine, rather than that the in-process steps "almost never" hang. - Consider widening the net in
stopping a server that is already stopped is what a shutdown hook does: loop the start / connect / stop / bounded-stop sequence a handful of times (say 10, on ports that do not collide with the other features) so that a regression has more chances to hit the window on any machine. The boundedCompletableFuture...get(30, SECONDS)already turns a hit into a failure rather than a hung build, which is the important part.
References: grails-data-mongodb/embedded/src/test/java/org/grails/datastore/gorm/mongodb/embedded/ContextStoppedServerApplication.java:28-38, EmbeddedMongoLifecycleSpec.groovy:280-311.
2. Low: spring-security-test-config.gradle has the same -PonlyNeo4jTests gap
The PR adds onlyNeo4jTests to the Hibernate 5, Hibernate 7, MongoDB and Redis configs so their suites stay out of the Neo4j jobs. gradle/spring-security-test-config.gradle:27-35 has the identical list without onlyNeo4jTests, and it is applied by twelve grails-spring-security/* modules, of which plugin (28 specs), rest/core (19), ui/plugin (4) and three others have tests. The three Neo4j jobs run bootJar check -PonlyNeo4jTests, so they still run those suites, which the security jobs already run. One more line in that list closes it; alternatively leave it for a follow-up, but then the commit message "every other datastore's -Ponly flag except -PonlyNeo4jTests" describes only four of the five configs.
3. Low: the idempotent stop() is now relied on, but the interface does not say so
EmbeddedMongoInitializer.stopEverythingAtExit() calls stop() on every started server without checking isRunning(), and EmbeddedMongoLifecycle.stop() checks it but races with that hook. Both backends now make a second stop() a no-op, and the whole PR hinges on that. RunningEmbeddedMongo.stop()'s javadoc (RunningEmbeddedMongo.java:36-39) only says "Called from a JVM shutdown hook, so it must not throw". Adding "and it may be called again after the server has stopped, which must do nothing" turns the fix into a contract a third backend would have to honour, rather than a property of the two shipped ones.
4. Low: what grails-redis loses with test-config.gradle
Dropping test-config.gradle is the right fix for the tasks never running, and the module passes without it. For the record, it also loses the --add-opens JVM arguments, the byte-buddy and objenesis test runtime dependencies (nothing in grails-redis mocks a class, so unused today), forkEvery/maxParallelForks/maxHeapSize, the **/*$*.class exclusion, and the CI build-cache opt-out. mongodb-test-config.gradle does without all of these too, so this is consistent with the other datastore configs; just worth knowing if a future Spock Mock(SomeClass) in this module fails with a missing byte-buddy.
5. Nits
- The commit "Stop the servers the replica set specification starts" cites
EmbeddedReplicaSetSpecas prior art; no such class exists. It meansEmbeddedMongoInitializerSpec, which has the identicalcleanup(). EmbeddedMongoLifecycleSpec.startApplicationredirects the child's output to a log and prints it on failure, but the test runtime bindsslf4j-nop, so the log will contain only JVM and Netty stderr, never the initializer's "Embedded MongoDB started at" lines. If the log is meant to help diagnose a hang on CI,slf4j-simpleintestRuntimeOnlywould make it say something.- The timeout values in
gradle.ymlare stated as twice the slowest observed run over 160 CI runs. I cannot verify those figures locally; the ratio is generous enough that a slow runner will not trip them, and the comment explaining the method is a good precedent.
What I checked and found no problem with
RunningInMemoryMongo.stop()andrestart()aresynchronized,runningandserverstayvolatilefor the unsynchronizedisRunning()reader, andrunningis set only aftershutdownNow()returns, so a first stop that throws leaves the server marked running for a retry, the same as flapdoodle'sfinally.- The mechanism described in the javadoc matches mongo-java-server 1.47.0:
shutdownNow()callscloseClients(), which ischannelGroup.close().syncUninterruptibly()with no timeout, andMongoDatabaseHandler.channelActiveadds the channel to the group. EmbeddedMongoReplicaSetSpec: each feature'scleanup:block (closing session and client) runs before the spec-levelcleanup()stops the server, and a feature whoseinitialize()throws registers no lifecycle bean, which thecontainsBeanguard handles.- The argument file for the child JVM quotes and escapes per the JDK
@argfilesrules, and port 27969 is not used anywhere else in the repository. - No user-facing behaviour changes, so no
grails-docupdate is needed.
jdaugherty
left a comment
There was a problem hiding this comment.
Once Mattias feedback is addressed, I'm good to merge.
Exercise the bounded double-stop regression ten times on separate ports and clarify that both the child JVM and Spock reproductions depend on timing. Enable SLF4J diagnostics in the child process and document the idempotent stop contract, with stopped-state assertions for both backends. Keep Spring Security test tasks out of Neo4j jobs, matching the other test configs. Verify that all 27 security test tasks across 12 modules are skipped with onlyNeo4jTests and remain enabled with onlySpringSecurityTests. The earlier replica set cleanup commit's reference to EmbeddedReplicaSetSpec was mistaken; the existing cleanup example is EmbeddedMongoInitializerSpec.
|
Addressed the Review Findings in b4aa3c6.
|
Problem
Neo4j and MongoDB functional jobs on
8.0.xwere cancelled at GitHub's six-hour limit six times between 2026-08-26 and 2026-09-11, most recently this Neo4j job. Each hung in:grails-data-mongodb-embedded:test: three after every spec had passed, and three whilestopping a server that is already stopped is what a shutdown hook doeswas running.An ordinary shutdown stops the embedded server twice.
EmbeddedMongoLifecyclestops it when the application context closes, and the shutdown hookEmbeddedMongoInitializerregisters stops it again as the JVM exits. The in-memory backend calledMongoServer.shutdownNow()both times:A second
shutdownNow()can wait forever. A client that connected just as the first shutdown began can be registered by Netty after its event loop has closed its channels, so the loop terminates with the client still in mongo-java-server's channel group, andcloseClients()waits, with no timeout, on a close the terminated loop never completes. Gradle's test worker exits throughSystem.exit, which waits for every shutdown hook, so the task never finished.The Neo4j jobs ran these tests at all because
mongodb-test-config.gradledid not skip its suites under-PonlyNeo4jTests.Fix
RunningInMemoryMongo.stop()returns when the server is already stopped, andstop()andrestart()are synchronized, since the context's shutdown hook and the initializer's run at the same time on the way out:The rest is the test and CI configuration this exposed:
EmbeddedMongoReplicaSetSpecstops its servers through the lifecycle bean. Its cleanup now followsEmbeddedMongoInitializerSpec. It calledclose()on contexts that were never refreshed, which does nothing, so its four mongods were left for the shutdown hook.mongodb-test-config.gradle,hibernate5-test-config.gradle,hibernate7-test-config.gradle,redis-test-config.gradleandspring-security-test-config.gradleskip their suites under-PonlyNeo4jTests. The Neo4j jobs had been running the MongoDB suites, 19 Hibernate test tasks, and Spring Security suites that their dedicated jobs already run.grails-redisapplies onlyredis-test-config.gradle, as the Hibernate and MongoDB modules apply only their own config. Withtest-config.gradleapplied as well, itstestandintegrationTesttasks ran in no CI job, the Redis jobs included.timeout-minutesto twice the slowest run that finished on its own, passed or failed, over the 160 CI runs from 2026-09-03 to 2026-09-13, rounded up to the half hour: 90 minutes for the Spring Security config and CAS jobs, 120 to 210 for the rest. The build jobs keep GitHub's default, since the macOS build has passed after more than five hours.The unbounded wait itself is fixed in mongo-java-server by bwaldvogel/mongo-java-server#256.
Worth calling out:
grails-redisno longer takes the--add-opensJVM arguments,byte-buddyandobjenesistest runtime dependencies,forkEvery/maxParallelForks/maxHeapSize, the**/*$*.classexclusion, or the CI build-cache opt-out fromtest-config.gradle. Its current tests do not mock classes and pass without these settings against Redis 7.4 and 8.0. Future class mocks may need those runtime dependencies added explicitly.Tests
an application whose context stopped its in-memory server still exitsruns an application in a JVM of its own that lets a client connect, stops the server as its context closes, and returns frommain. The hang is a timing-dependent race. On the author’s machine, the Java child application,ContextStoppedServerApplication, failed to exit in 8 of 8 runs without the fix. On the reviewer’s Linux machine, the unfixed child exited in all eight runs while the in-process double-stop feature reproduced the hang. Both tests exercise the race; neither is guaranteed to reproduce it on every machine. The child feature fails after 30 seconds if the JVM does not exit.The in-process double-stop feature repeats the start / connect / stop / bounded-stop sequence ten times on separate ports. Each second stop has a 30-second bound, so a reproduced regression fails instead of hanging the build. The child JVM uses
slf4j-simpleso its captured log includes initializer diagnostics.RunningEmbeddedMongo.stop()now documents that stopping an already stopped server must do nothing.:grails-data-mongodb-embedded:test, and:grails-redis:testwith:grails-redis:integrationTest, are green.