Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity - #16034

These two assert Groovy's own ClassNode equality contract rather than anything about AstPropertyResolveUtils. They hold today — 5.0.7's ClassNode.equals compares getText() and hashCode() delegates to getText().hashCode() — but if Groovy ever moves ClassNode to identity equality this spec fails while the production behaviour it guards is still perfectly correct.

!first.is(second) on line 79 is the precondition the test actually needs. Consider keeping that and demoting the other two to a comment explaining why two same-named nodes used to collide.

Done - trimmed to just !first.is(second), with a comment explaining why the equals()/hashCode() equality matters (it's the exact collision condition a name- or equals()-keyed cache would hit) rather than asserting on Groovy's own ClassNode equality contract.

AstUtils.isDomainClass(ClassNode) is @Memoized (AstUtils.groovy:428), and that memo cache is keyed by ClassNode equality, not identity — ClassNode.equals/hashCode both reduce to getText() (ClassNode.java:1283/1288 in 5.0.7). @Memoized also defaults to maxCacheSize = 0, so it is an unbounded UnlimitedConcurrentCache that never evicts.

That leaves both headline claims in the new javadoc false one level down: two distinct same-named ClassNodes do still collide here, and the first node seen per distinct name is retained for the life of the JVM, pinning its module, compile unit and GroovyClassLoader with it.

Verified against this branch with two hand-built nodes both named probe.MemoWidget, the first plain and the second genuinely carrying @Entity, resolved in that order:

plainProps            = [title]
entityProps           = [title]     <- @Entity node, no id/version
isDomainClass(entity) = false       <- despite the annotation

Control, the same @Entity node under a name nothing else had touched: [id, title, version].

To be fair to the change: the @Memoized predates this PR and you are not introducing it. But this PR does newly route through it. Under the old name-keyed map, a same-named second node returned the first node's cached map and never called isDomainClass at all; now it computes its own properties and consults the memoized verdict. The corruption narrows from the entire property map to the domain-ness boolean, which is a genuine improvement — "no collision is possible" just overstates where it lands.

I am not asking you to fix isDomainClass here; that deserves its own issue rather than growing this PR. Just asking that the javadoc claims match what is actually true.

synchronized (cacheHolder) cannot deliver the safety the javadoc claims for it, because the nodes being locked are not always private to one compilation. QueryStringTransformer.groovy:208 walks a property path by feeding each resolved type back in as the next receiver, so a JDK-typed property makes an interned ClassHelper singleton the cache holder.

Verified on this branch:

propType.is(ClassHelper.STRING_TYPE) = true
STRING_TYPE cache metadata before    = false
STRING_TYPE cache metadata after     = true

ClassNode.getModule() reads that same ListHashMap through getNodeMetaData(ModuleNode.class) without taking the node monitor, so this lock only excludes callers that opt into it. The javadoc frames the leftover exposure as risk that "belongs to ClassNode's metadata storage in general", but before this change the utility never wrote to any ClassNode — it is this PR that makes GORM a writer to those JVM-wide maps.

Gating on cacheHolder.isPrimaryClassNode() and skipping the cache otherwise would confine writes to nodes the compilation actually owns, and the JDK types that get skipped are cheap to recompute.

Separately, computeProperties runs entirely inside this monitor, and for a resolved domain class it reaches ClassPropertyFetcher.forClass(getTypeClass()).getPropertyValue(...), which forces the user class's static initialiser and invokes user-written static getters while the lock is held. Computing outside the lock and publishing afterwards would take alien code out from under it.

Minor: both of these are redirect-less holder nodes, so under the new scheme each becomes its own cache holder and never shares with anything. Line 236 already uses the plain-reference convention — ClassHelper.make(Long.class).getPlainNodeReference() here would redirect to the interned Long node so id/version lookups share a single entry again.

Of everything here, this is the change I would most like reverted. Given the @Memoized isDomainClass collision (see the comment on computeProperties), uniqueness is still required for correctness here — the original warning was right.

The risk is concrete inside this module. DirtyCheckTransformationSpec compiles @DirtyCheck class Book and class Author from strings, which are not domain classes, while QueryStringTransformerSpec compiles @Entity class Book and class Author, which are. forkEvery = 50 on CI (gradle/test-config.gradle:88) means up to fifty spec classes share one JVM, so both can land in the same fork in either order, and whichever runs first wins the memoized isDomainClass answer for that name.

A maintainer who takes this comment at face value and renames ClosureCaptureBook back to Book walks straight into an order-dependent flake of exactly the #16030 character.

Please keep the "must be unique" wording as a requirement. If you want to record the history, the accurate version is that the property map is now per-instance while the domain-class verdict is still name-keyed.

Same point as the equivalent comment in WhereQueryClosureCaptureSpec: "that collision can no longer happen regardless of naming" does not hold while AstUtils.isDomainClass is @Memoized by name, so the uniqueness requirement should stay stated as a requirement rather than a stylistic preference.

This precondition never runs as an assertion. It sits in an and: block continuing given:, and Spock applies implicit conditions only in then:/expect: blocks, so the expression is evaluated and discarded.

Confirmed on this branch by putting false and x == 999 in an and: after given: in a scratch spec — the feature passed.

Needs assert !first.is(second), or to move into an expect: block. Worth correcting because the comment directly above presents this line as the load-bearing premise of the feature.

executor.shutdown() is the last statement of the when: block, so it is skipped on exactly the path where it matters: if any future.get(30, SECONDS) throws, the collect aborts and the pool is never shut down. Executors.newFixedThreadPool threads are non-daemon and do not time out, and with forkEvery = 50 this JVM goes on to run more specs.

barrier.await() also has no timeout, so if one worker dies before reaching the barrier the rest park indefinitely and the only symptom is a 30-second timeout per future.

Suggest a cleanup: block calling executor.shutdownNow() in both concurrency features, plus the timeout overload of await. The GroovyClassLoader in the reflection feature is also never closed — it is AutoCloseable, and closing it would suit a spec whose subject is a classloader leak.