Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity

[borinquenkid]

Summary

WhereQueryClosureCaptureSpec has two flaky feature methods (~2% flaky per
#16030, 0 hard failures) verifying that closures generated by the
where {} query transform only capture the variables they actually reference.

Root cause

AstPropertyResolveUtils.cachedClassProperties is a static, non-thread-safe HashMap
used by the where-transform's variable-scope recompute path
(DetachedCriteriaTransformer). It was already keyed by ClassNode.getName()
(qualified name), not a bare simple name as first suspected — but ClassNode.equals()/
hashCode() also compare by name, and this spec's own test deliberately compiles
identical source twice into two separate GroovyClassLoaders, producing two
distinct ClassNode instances that are equals()-identical and therefore collide as
the same cache entry. This is reachable by any two classes sharing a name across
separate compilations, not just this test — a prior partial fix (08bd0bf) only
renamed this spec's fixtures to reduce collisions with sibling specs, without fixing
the cache itself.

Fix

• Switched cachedClassProperties to a synchronized IdentityHashMap<ClassNode, ...>
  — keys compared by ==, so two distinct ClassNodes can never collide regardless of
  shared naming.
• Fixed a related concurrency bug in the same method: a new cache entry was previously
  published into the shared map before its population loop finished, letting a
  concurrent reader observe a partially-populated entry. Population now completes into
  a local map first, then publishes atomically.

Testing

• New AstPropertyResolveUtilsSpec.groovy (public-API only): includes a test that
  constructs two distinct ClassNodes sharing the exact same name and asserts each
  resolves independently — a direct, deterministic reproduction of the collision
  mechanism, not a rerun-and-hope test.
• :grails-datamapping-core:test (full module): BUILD SUCCESSFUL, no failures,
  including both previously-flaky feature methods.
• CodeNarc/Checkstyle: clean.

Related: #16030

[Copilot]

Pull request overview

This PR addresses flakiness in WhereQueryClosureCaptureSpec by fixing AstPropertyResolveUtils’s static property-cache to avoid key collisions between distinct ClassNode instances that compare equal by name, and by ensuring cache entries are only published once fully populated.

Changes:

• Switch cachedClassProperties to a synchronized IdentityHashMap keyed by ClassNode identity (==) instead of name/equals().
• Avoid publishing partially-populated cache entries by building into a local map first, then publishing to the shared cache.
• Add a new Spock spec to deterministically reproduce and guard against same-name ClassNode cache collisions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java	Reworks the static cache to use identity-based keys and fixes non-atomic publication of newly created cache entries.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy	Adds tests that exercise same-name ClassNode scenarios to prevent future cache-collision regressions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[borinquenkid]

Good catch on the mechanics — you're right that this test's two ClassNodes use different fully-qualified names (org.example.one.Widget vs org.example.two.Widget), so it wouldn't have collided under the old name-/equals()-keyed cache either, and doesn't by itself guard the regression.

That guard is the next test below, "property lookups for two distinct ClassNode instances with the exact same unqualified name do not corrupt each other" — both ClassNodes there are named plain Widget with no package, so they do compare equal (first == second, matching hashCode()) exactly as the old cache's key would, and the test asserts they still resolve independently under the new identity-keyed cache. That's the one that fails under the old implementation and passes under this fix.

This first test is intentionally a different, narrower check — that two distinct, differently-named classes never get confused with each other, which is a correctness property worth keeping on its own regardless of the collision bug. Leaving both as-is: this one for general non-contamination across genuinely different classes, the next one for the actual same-name collision regression.

[bito-code-review]

The test case "property lookups for two same-named ClassNodes in different packages do not corrupt each other" currently uses different fully-qualified names ('org.example.one.Widget' and 'org.example.two.Widget'). To better exercise the regression mechanism and ensure the fix works for same-named classes, you should update this test to use the same fully-qualified name for both ClassNode instances. This will confirm that the IdentityHashMap correctly distinguishes between distinct instances that share the same name.

grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy

given: 'two distinct ClassNodes with the same simple name declared in different packages'
        ClassNode first = new ClassNode('org.example.Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
        first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null)

        ClassNode second = new ClassNode('org.example.Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
        second.addProperty('weight', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null)

[codecov]

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 51.8557%. Comparing base (6d1acad) to head (c3a4d92).
⚠️ Report is 139 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
...astore/gorm/transform/AstPropertyResolveUtils.java	92.8571%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             8.0.x     #16034         +/-   ##
================================================
+ Coverage         0   51.8557%   +51.8557%     
- Complexity       0      18112      +18112     
================================================
  Files            0       2046       +2046     
  Lines            0      96273      +96273     
  Branches         0      16726      +16726     
================================================
+ Hits             0      49923      +49923     
- Misses           0      38981      +38981     
- Partials         0       7369       +7369     

Files with missing lines	Coverage Δ
...astore/gorm/transform/AstPropertyResolveUtils.java	80.7229% <92.8571%> (ø)

... and 2045 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[borinquenkid]

Thanks for the review! The scenario you're describing — two distinct ClassNode instances sharing the exact same fully-qualified name — is already covered by the test right below this one:

void "property lookups for two distinct ClassNode instances with the exact same unqualified name do not corrupt each other"() {
    given: 'two distinct ClassNode instances - as produced by two separate compilations - sharing an identical unqualified name'
    ClassNode first = new ClassNode('Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
    first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null)

    ClassNode second = new ClassNode('Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
    second.addProperty('weight', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null)

    expect: 'the two ClassNode instances compare equal by name - the exact condition that would collide in a name-keyed or equals()-keyed cache'
    first == second
    first.hashCode() == second.hashCode()
    !first.is(second)
    ...

It goes a step further than the suggested edit by asserting first == second and matching hashCode(), which explicitly proves the collision condition a name-/equals()-keyed cache would hit — exactly the regression this fix guards against.

The test this comment is attached to intentionally covers a different case: two ClassNodes that share a simple name but differ by package (so they're not equal), which is a distinct scenario worth keeping separate. Given the exact-FQN-collision case is already exercised, I'll leave both tests as-is rather than introduce a near-duplicate.

[jdaugherty]

I had AI review this before taking a look, here's it's comments:

Reviewed the identity-keyed cache change. The direction is right — ClassNode.equals/hashCode really do compare by getText() (confirmed in Groovy 5.0.7: equals falls through to ((ClassNode) that).getText().equals(getText())), so a name- or equals()-keyed static cache genuinely conflates distinct compilations, and reordering so an entry is published only once fully populated is a real fix.

Two things I'd want resolved before this lands:

1. Keying a static map by ClassNode turns a bounded cache into a classloader leak. A primary ClassNode transitively pins its CompileUnit and therefore its GroovyClassLoader, and nothing ever evicts. Detail inline — the per-node getNodeMetaData alternative removes the collision and the leak, and drops the static state entirely. This is the part I'm most concerned about, since the whole point of the change is to make a process-wide static cache safe.
2. The stated root cause doesn't explain a flaky failure. The same source compiled twice yields two ClassNodes with identical property sets, so a name collision between just those two can't diverge. The genuinely order-dependent input is classNode.isResolved() at first-lookup time, which this PR doesn't change. Inline.

Smaller items, all inline: the protected field's type change plus new final is a breaking change worth converting into private + an explicit clear hook; the javadoc's concurrency rationale cites parallel test forks, which are separate JVMs and so cannot race on a static field; and the new spec doesn't cover the domain-class branches or the concurrency behaviour it claims to fix.

One follow-up outside the diff: WhereQueryClosureCaptureSpec:35 and WhereQueryEmbeddedBlockTransformSpec:37 both still carry the comment "The domain class names must be unique across the test JVM because AstPropertyResolveUtils caches resolved properties statically by class name". That is no longer true after this change, and the fixture renaming in 08bd0bf that it justifies is now unnecessary. Please update both comments (and note whether the renaming can be reverted) so those specs aren't left documenting the old behaviour.

[jdaugherty]

Keying this static map by ClassNode identity does fix the collision, but it converts a bounded cache into a classloader leak.

Under the old String key the map held at most one entry per distinct class name. With ClassNode keys it holds one entry per instance, and a primary ClassNode transitively pins its entire compilation:

ClassNode.getModule()  ->  ModuleNode.getUnit()  ->  CompileUnit.loader  (GroovyClassLoader)

plus ClassNode.clazz directly for resolved nodes. Since nothing ever removes an entry and the field is now static final, every compilation that runs a GORM AST transform inside a long-lived JVM permanently retains that compilation's classloader and every class it loaded. That is not hypothetical: the Gradle Groovy compiler daemon is reused across compileGroovy tasks and across builds, and dev-mode / GroovyClassLoader-driven recompiles land in the same place. The map values are ClassNodes too, so they pin loaders as well — that part pre-existed, but it was O(distinct class names) and is now unbounded in the number of compilations.

Groovy already stores per-node state exactly this way (ClassNode.getModule() above is itself getNodeMetaData(ModuleNode.class)), so the cleanest fix is to drop the static map and hang the cache off the node:

private static final String PROPERTIES_CACHE_KEY = AstPropertyResolveUtils.class.getName() + ".properties";

private static Map<String, ClassNode> getPropertiesFromCache(ClassNode classNode) {
    return classNode.getNodeMetaData(PROPERTIES_CACHE_KEY, cn -> computeProperties(classNode));
}

getNodeMetaData(Object, Function) is available in Groovy 5, is identity-scoped by construction (so the collision this PR fixes cannot arise at all), needs no global lock, and is collected together with the node. Worth deciding explicitly whether the holder should be classNode or classNode.redirect() — getModule() uses redirect(), and redirected nodes are the one case where the two differ.

If a process-wide map has to stay for some reason, it needs weak identity keys and/or an explicit eviction point, plus a note documenting the expected lifecycle.

[borinquenkid]

Agreed, and went with your suggested direction: the static map is gone entirely, and the cache is now stored as ClassNode metadata (classNode.redirect().getNodeMetaData(key, fn)), so a cache entry is only reachable through the node it describes and is collected with it. See the updated javadoc on cachedClassProperties's replacement (PROPERTIES_CACHE_KEY) for the full writeup, including why keying on redirect() is safe here (setRedirect() throws for a primary node - i.e. every real caller of this utility, since they're all mid-compilation - so redirect() is just this for the node's whole life in practice) and why access ended up needing explicit per-node synchronization (some real callers can resolve to interned singletons like ClassHelper.OBJECT_TYPE for a plain Object/def-typed property, and those are shared across every compilation in the JVM, not scoped to one thread the way a normal node is). Landed in 078ac9d, tightened further in c3a4d92 after an adversarial self-review turned up the synchronization gap.

[jdaugherty]

Two breaking changes to a protected member of a public class on one line: the key type changed (String -> ClassNode) and the field became final. Anything downstream that reassigned it — today the only way to clear this cache, which plugin AST transforms and test harnesses plausibly do — now fails to compile rather than merely behaving differently.

If it's being broken anyway, take it the rest of the way: make it private static final and expose an explicit, documented clearCache(). That shrinks the exposed surface and gives the retention problem above an escape hatch, instead of leaving protected visibility on a field nobody can usefully touch any more.

[borinquenkid]

Went further than private + clearCache() - the static map is gone entirely (078ac9d), so there's no field left to expose or protect. That's still technically a breaking removal for anyone who referenced the old protected field directly, just a cleaner one than a silent type change. Given it's an internal implementation-detail field with no getter/documented extension use, I'm inclined to leave it out of the upgrade guide, but flagging it in case there's a reason to cover it there.

[jdaugherty]

This rationale is inaccurate in a way that will mislead the next reader. maxParallelForks = configuredTestParallel (gradle/test-config.gradle:86) forks separate JVMs, and each JVM gets its own copy of a static field — parallel test forks can therefore never race on this map. JUnit's in-JVM parallel execution isn't enabled anywhere in the build either.

The concurrency exposure that does exist is the compiler itself: multiple compileGroovy tasks running concurrently in a shared Gradle worker, and any embedded compilation driven from more than one thread. Worth rewording to that, otherwise the comment justifies the synchronization with a scenario that can't happen.

[borinquenkid]

You're right, fixed (078ac9d). The rationale no longer claims parallel test forks race on this - confirmed against gradle/test-config.gradle that maxParallelForks really does fork separate JVMs and this build never enables JUnit's in-JVM parallel execution, so that was never a real race. Rewrote the javadoc around the actual exposure instead: interned ClassHelper singletons (OBJECT_TYPE, STRING_TYPE, etc.) that any concurrently-running compilation in the same JVM could resolve to and touch through this cache - which is also why access is now synchronized per-node (c3a4d92) rather than resting on "only one thread ever touches a given node," which turned out not to be universally true.

[jdaugherty]

The reorder-and-publish-last fix is correct, and synchronizedMap does supply the safe publication it depends on: get and put synchronize on the same mutex, so a reader that observes the entry also observes a fully-populated HashMap. Worth stating that explicitly in the inline comment, because "publish only once fully populated" is only sufficient given that happens-before edge — with a bare HashMap the reordering alone wouldn't have been enough.

One remaining wrinkle: the check-then-act across the get on line 120 and the put on line 136 is not atomic, so two concurrent callers can each build a complete map and the later put wins. Benign here (the maps are equivalent and never mutated after publication), but cachedClassProperties.computeIfAbsent(classNode, ...) on the synchronized wrapper would be atomic and shorter — the tradeoff being that it holds the mutex for the whole superclass walk.

[borinquenkid]

getNodeMetaData(key, fn) uses computeIfAbsent internally (078ac9d), and I additionally wrapped the call in synchronized (cacheHolder) (c3a4d92) after an adversarial self-review flagged that the backing ListHashMap is explicitly documented as not thread-safe, and that some real callers can land on interned singleton nodes (ClassHelper.OBJECT_TYPE etc.) that more than one concurrent compilation could reach - the "only one thread per node" assumption doesn't hold for those. See the javadoc for the precise scope of what the synchronization does and doesn't cover.

[jdaugherty]

Identity keying makes the cache immune to name collisions, but I don't think it removes the nondeterminism that a flake like #16030 needs. populatePropertiesForClassNode consults ClassPropertyFetcher only when classNode.isResolved() (line 172), so an entry is a snapshot of whatever resolution state the node happened to be in at the first lookup, and it is never refreshed afterwards. If the first lookup for a node lands at a different compilation phase between runs, the cached property set still differs between runs — identity keys or not.

That also weakens the root-cause story in the description: compiling the same source twice produces two ClassNodes with identical property sets, so a name-keyed collision between just those two is harmless and can't produce divergent bytecode on its own. The collisions that would actually diverge are with a differently-shaped same-named class, or with a same-named node whose entry was cached at a different resolution state.

Can you pin down which one you observed — e.g. the two colliding class names, or the flake reproducing in a loop pre-fix and not post-fix? The change is an improvement regardless, but if the resolution-state snapshot is the real driver then #16030 comes back and this gets recorded as already fixed.

[borinquenkid]

Wanted to actually test this rather than argue about it. I reverted to the pre-fix cache and ran the exact "same source compiled twice" scenario WhereQueryClosureCaptureSpec's second test exercises, 40 times sequentially in one JVM (simulating many test classes sharing one fork's static state) - it never diverged, even with the old buggy cache. So I can't point to a repro that pins the ~1% CI flake specifically to the name/identity-collision mechanism, and you may well be right that it isn't the actual trigger. I'm keeping the identity-safety fix regardless, since it's an independently real, provable bug on its own terms (the spec's second test shows two distinct, differently-shaped ClassNodes that happen to share a name getting their properties conflated under the old cache) - but I want to be upfront that I haven't confirmed it's the flake's cause, only that it's a real bug. I'll be watching the flaky-test dashboard (#16030) after this merges to see whether WhereQueryClosureCaptureSpec actually clears.

On the isResolved() snapshot mechanism specifically, I checked the Groovy 5.0.7 source directly: ClassNode.clazz has no setter anywhere outside the ClassNode(Class) constructor, and setRedirect() throws a GroovyBugError if called on a primary node - which is what every real caller of this utility passes in, since they're all resolving a class mid-compilation. So for the nodes this cache actually serves, redirect() is simply the node itself for its entire life, and isResolved() can't flip after the node is first cached. I don't think the staleness you're describing can occur for this code path as it's actually used, though I agree the "cache once, forever" design would be fragile if it could - documented the reasoning (and the primary-node guarantee it leans on) in the class javadoc.

[jdaugherty]

Good to see this class get its first coverage. As written, though, it only exercises hand-built non-domain ClassNodes, so the branches that actually matter to the transform never execute: AstUtils.isDomainClass is false in all three features, which means the injected id/version entries, the hasMany/belongsTo/hasOne handling in populatePropertiesForInitialExpression, the isResolved() / ClassPropertyFetcher path, and the superclass walk are all untested. This PR rewrites the population loop feeding every one of those, so per the repo rule that a touched class gets its behaviour verified they should be covered here — an @Entity-annotated ClassNode with a hasMany initial expression and a domain superclass would reach most of it.

Also missing: a test for the concurrency fix the PR claims. Several threads calling getPropertyNames concurrently on the same node and on distinct nodes, asserting every returned list is complete, would exercise both the synchronized map and the publish-after-populate ordering. As it stands, reverting either half of the concurrency change leaves this spec green.

[borinquenkid]

Added both. Domain-class coverage now includes identity/version injection and hasMany/belongsTo/hasOne resolved two ways: via AST initial expressions (a hand-built ClassNode with a crafted MapExpression, matching what static hasMany = [...] compiles to) and via reflection on an already-resolved class (compiled through GroovyClassLoader.parseClass then re-wrapped with ClassHelper.make(), since that's the only way to get isResolved() == true for that branch).

Also added two concurrency tests: many threads each resolving their own distinct, identically-named ClassNode (proves the identity-collision-freedom property holds under concurrent load, not just sequentially), and - after an adversarial self-review pointed out the first test can't exercise any real race, since nothing is shared between the threads - a second test where 32 threads resolve the exact same shared ClassNode concurrently, which is what the synchronized fix mentioned above actually protects. Worth being honest about that second test's limits: I checked whether it fails without the synchronized guard, and it didn't, in 8 runs - the cached computation is deterministic and idempotent, so a black-box return-value test can't reliably force the underlying race into an observably wrong result. Said that directly in the test's comment rather than overclaiming what it proves; the synchronization is justified by ListHashMap's own "not thread-safe" documentation, not by this test having caught a live bug.

[jdaugherty]

The name says "and caches", but neither assertion can distinguish a cache hit from a miss — on a miss getPropertyType falls through to classNode.getProperty(propertyName) and returns the same STRING_TYPE, so both lines pass with caching entirely disabled.

To actually pin the caching behaviour: resolve once, then add a second property to the ClassNode and assert getPropertyNames still returns the stale view. That snapshot-at-first-lookup semantics is what this class really guarantees and what the transform relies on, and it's currently unverified.

[borinquenkid]

Replaced it. The new test adds a second property to the ClassNode after the first getPropertyNames lookup and asserts the second lookup doesn't see it - getPropertyNames has no live-fallback path (unlike getPropertyType, which falls through to a direct classNode.getProperty() lookup when the cache doesn't contain the key), so this actually proves the result was cached rather than recomputed. It also asserts, via a direct classNode.getProperty('extra') != null check, that the property really was added to the underlying node - so the test demonstrates staleness specifically, not just a lookup that happens to return nothing.

[jdaugherty]

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.

[borinquenkid]

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.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: c3a4d92
▶️ Tests: 57639 executed
⚪️ Checks: 60/60 completed

---

Learn more about TestLens at testlens.app.