Fix(#1555): stale plugin descriptor for rebuilt reactor plugins

[HarshMehta112]

Problem

When a project builds a Maven plugin in its own reactor (e.g. mvnd verify),
subsequent daemon invocations keep serving a stale plugin descriptor even
after the plugin's code changes. The reported symptom is the threadSafe flag
not being re-read: toggle a Mojo's @Mojo(threadSafe = ...), rebuild, and the
daemon still uses the old value (#877).

Root cause

InvalidatingRealmCacheEventSpy already evicts reactor plugin realms and
extension realms at the end of each build (any realm whose URLs point under
multiModuleProjectDirectory). However, the PluginDescriptor — which carries
the threadSafe flag, @Parameter default values, etc. — is stored in a
separate singleton cache (InvalidatingPluginDescriptorCache) that was never
evicted there.

The timestamp-based invalidation cannot cover this case: with verify/test
(no install), a reactor plugin is resolved to a target/classes directory.
A directory's lastModifiedTime does not change when a .class file inside it
is rewritten, so the cached descriptor's file state looks unchanged and the stale
descriptor is reused. (The existing ModuleAndPluginNativeIT does not hit this
because it uses clean install, which rewrites the jar in the local repo and
bumps its mtime, invalidating everything.)

Fix

Extend InvalidatingRealmCacheEventSpy to also evict the
InvalidatingPluginDescriptorCache and InvalidatingPluginArtifactsCache
entries that refer to artifacts in the build tree (or match the configured
mvnd.pluginRealmEvictPattern) at MavenExecutionResult. Eviction is driven by
CacheRecord.getDependencyPaths(), consistent with the existing realm eviction.

Test

Adds ReactorPluginDescriptorReloadTest, which runs clean package (no install,
so the plugin stays a target/classes directory), changes a @Parameter
defaultValue between two daemon invocations, and asserts the new value takes
effect. This fails before the fix and passes after.

Note: integration-tests test sources currently do not compile on master
(JvmTestClient/MvndTestExtension were not updated after the #970 client API
refactor), so this IT could not be run locally. The daemon change itself
compiles and passes spotless.

Fixes #1555

[gnodet]

The daemon fix is correct and well-motivated — the shouldEvict(CacheRecord) approach using getDependencyPaths() is the right abstraction for path-based eviction, and the removeIf lambda correctly widens the record types.

Test naming convention (medium): The test class uses @MvndNativeTest but is named ReactorPluginDescriptorReloadTest.java. In this project, all 17 @MvndNativeTest classes are named *NativeIT.java (picked up by Failsafe), and all 31 @MvndTest classes are named *Test.java (picked up by Surefire). This naming is 100% consistent. With the current name, Surefire would try to run this test during the test phase, but it requires the native binary. It should be renamed to ReactorPluginDescriptorReloadNativeIT.java (see ModuleAndPluginNativeIT.java in the same package for the pattern).

Silent exception catch (low): The shouldEvict(CacheRecord) catch block swallows exceptions without logging. The existing catch blocks in this class follow the same pattern, so this isn't a consistency violation, but adding a LOG.debug() in the new method would help troubleshooting since it catches broad Exception rather than narrow URISyntaxException.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Excellent root cause analysis — the PR description clearly explains why the existing ModuleAndPluginNativeIT passes (install changes jar mtime) while the reported scenario (verify/package) fails.

However, the primary fix has a critical gap:

Descriptor cache eviction is ineffective (high)

The new shouldEvict(CacheRecord) relies on record.getDependencyPaths(), which for InvalidatingPluginDescriptorCache.Record calls descriptor.getArtifacts(). However, Maven 4's PluginDescriptor copy constructor does not copy the artifacts field — only groupId, artifactId, version, goalPrefix, pluginArtifact (singular), mojos, dependencies, etc. are copied. Since InvalidatingPluginDescriptorCache stores a clone via this constructor, the cached Record always has getArtifacts() == null.

The existing Optional.ofNullable(...) guard in getDependencyPaths() then produces an empty stream → anyMatch() returns false → the descriptor entry is never evicted — defeating the fix.

The plugin artifacts cache eviction works correctly, but evicting only the artifacts cache without the descriptor cache does not fix the stale descriptor bug (old @Parameter defaults, threadSafe flag, etc. are still served).

Suggestion: Consider matching descriptor cache keys against reactor project coordinates using the key parameter (the removeIf(BiPredicate<K, V>) already provides the key — the lambda currently ignores it with (k, r) -> shouldEvict(r)).

Integration test cannot verify the fix (medium)

The test is correctly designed, but as noted in the PR description, the integration-tests module does not compile on master (pre-existing issue from the #970 client API refactor). This means the fix cannot be verified by the test.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Nice work on this fix! The root cause analysis is excellent — clearly identifying why timestamp-based invalidation fails for target/classes directories and why the existing ModuleAndPluginNativeIT test (which uses clean install) doesn't catch this.

The core daemon fix is correct: evicting the plugin descriptor and artifact caches alongside realm caches addresses the root cause. Constructor injection via @Inject is clean and safe with Sisu.

A few items to address:

1. Test naming convention (medium): All 17 existing @MvndNativeTest-annotated classes use the *NativeIT naming convention (e.g., ModuleAndPluginNativeIT). The Surefire/Failsafe configuration depends on this: Surefire runs *Test files, Failsafe runs *IT files under the native profile. The class should be renamed to ReactorPluginDescriptorReloadNativeIT to be picked up by the correct test runner.

2. Wrong issue reference (low): The Javadoc links to #877 (Jansi native library loading) instead of #1555 (reactor plugins not reloaded), which is the issue this PR fixes.

3. Silent exception in shouldEvict (low): The catch block in the new shouldEvict(CacheRecord) method swallows a broad Exception without logging. The existing shouldEvict methods catch the narrow URISyntaxException. Consider adding LOG.debug("Error checking cache entry for eviction, evicting to be safe", e) for troubleshooting.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[gnodet]

Naming convention: All 17 existing @MvndNativeTest-annotated classes use the *NativeIT suffix (e.g., ModuleAndPluginNativeIT). The closely related ModuleAndPluginNativeIT.java uses the same projectDir and follows this pattern.

With the current *Test name, Surefire will pick this up during the test phase (wrong phase, likely missing native infrastructure), while Failsafe will skip it entirely in the native profile.

Suggested rename: ReactorPluginDescriptorReloadNativeIT

A companion ReactorPluginDescriptorReloadTest extending it with @MvndTest (like ModuleAndPluginTest extends ModuleAndPluginNativeIT) would complete the pattern.

[gnodet]

This links to #877 (Jansi native library loading on Synology NAS), but this PR fixes #1555 (reactor plugins not reloaded). The PR title and body correctly reference #1555.

[gnodet]

AI Review — Changes Requested ❌

Thank you for investigating this issue — the root cause analysis in the PR description is excellent and clearly explains why ModuleAndPluginNativeIT passes (uses install which rewrites the jar mtime) while the reported scenario fails.

However, the core eviction mechanism for the plugin descriptor cache does not work as intended:

Critical Issue

shouldEvict(CacheRecord) will never evict plugin descriptor entries. The method relies on record.getDependencyPaths(), which for InvalidatingPluginDescriptorCache.Record calls descriptor.getArtifacts(). However, Maven 4's PluginDescriptor copy constructor does not copy the artifacts field — it only copies groupId, artifactId, version, goalPrefix, pluginArtifact, mojos, dependencies, etc.

Since InvalidatingPluginDescriptorCache stores cloned descriptors (both put() and get() call clone()), the cached Record always has getArtifacts() == null. The Optional.ofNullable(null).orElse(emptyList()) guard produces an empty stream, anyMatch() returns false, and the descriptor is never evicted.

The pluginArtifactsCache eviction (line 120) works correctly, but that alone doesn't fix the stale descriptor bug (old @Parameter defaults, threadSafe flag, etc. remain cached).

Possible fixes:

• (a) Modify Record.getDependencyPaths() to include descriptor.getPluginArtifact().getFile().toPath() (since pluginArtifact IS copied and its file points to target/classes for reactor plugins)
• (b) Match descriptor cache entries against reactor project coordinates using the descriptor's GAV
• (c) Use the cache key's toString() which returns groupId:artifactId:version

Other Issues

1. Test naming convention: All 17 existing @MvndNativeTest classes use *NativeIT.java naming (failsafe). This class uses *Test.java (surefire), which would run during the wrong Maven phase when no native executable is available. Rename to ReactorPluginDescriptorReloadNativeIT.java.

2. Wrong issue reference in Javadoc: Links to #877 (Jansi native library issue) instead of #1555 (reactor plugins not reloaded).

3. Broad exception catch without logging: The catch (Exception e) block silently swallows errors. The existing methods catch the narrower URISyntaxException. Consider adding LOG.debug("Error checking cache entry for eviction", e) for troubleshooting.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet