Groovy class loaders and script engine tweaks to reduce leaks

[jmendeza]

Groovy class loaders and script engine tweaks to reduce leaks
https://github.com/craftersoftware/craftercms/issues/1266

[coderabbitai]

Walkthrough

Changes

The PR adds an optional script-factory destruction lifecycle, closes Groovy classloaders during configuration replacement and teardown, and integrates script cleanup into site context destruction and creation failure handling. Context rebuilds now log destruction failures without aborting the rebuild.

Script and site lifecycle cleanup

Layer / File(s)	Summary
Groovy factory lifecycle src/main/java/org/craftercms/engine/scripting/ScriptFactory.java, src/main/java/org/craftercms/engine/scripting/impl/GroovyScriptFactory.java, src/test/java/org/craftercms/engine/scripting/impl/GroovyScriptFactoryTest.java	ScriptFactory adds an optional destroy() method. GroovyScriptFactory closes replaced and active Groovy classloaders, with error logging, and tests perform explicit teardown.
Site context teardown and creation failure cleanup src/main/java/org/craftercms/engine/service/context/SiteContext.java, src/main/java/org/craftercms/engine/service/context/SiteContextFactory.java	Site destruction now closes the script factory before the parent classloader, while failed context creation destroys partially initialized contexts and retains store cleanup.
Context rebuild error handling src/main/java/org/craftercms/engine/service/context/SiteContextManager.java	Context rebuilds catch and log failures from destroying the previous context.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SiteContextManager
  participant SiteContext
  participant GroovyScriptFactory
  participant GroovyClassLoader
  SiteContextManager->>SiteContext: destroy previous context
  SiteContext->>GroovyScriptFactory: destroy()
  GroovyScriptFactory->>GroovyClassLoader: close active loader
  SiteContext->>SiteContext: close parent classLoader
  SiteContextManager->>SiteContextManager: log destruction failure and continue rebuild

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: Groovy loader and script engine cleanup to reduce leaks.
Description check	✅ Passed	The description includes a concise change summary and the linked ticket reference required by the template.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jmendeza]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main/java/org/craftercms/engine/service/context/SiteContextFactory.java (1)

351-364: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Assign resources to siteContext immediately to ensure cleanup on failure.

If an exception is thrown by methods like getApplicationContext or getUrlRewriter, the newly created classLoader, scriptFactory, and appContext will leak. Because they are currently assigned to siteContext at the end of the block, siteContext.destroy() in the catch block will see these fields as null and skip their cleanup.

Assign them immediately after creation so the error-handling block can successfully release them.

🐛 Proposed fix

             URLClassLoader classLoader = getClassLoader(siteContext);
+            siteContext.setClassLoader(classLoader);
             ScriptFactory scriptFactory = getScriptFactory(siteContext, classLoader);
+            siteContext.setScriptFactory(scriptFactory);
             ConfigurableApplicationContext appContext = getApplicationContext(siteContext, classLoader, config,
                                                                               resolvedAppContextPaths, resourceLoader);
+            siteContext.setApplicationContext(appContext);
             UrlRewriter urlRewriter = getUrlRewriter(siteContext, resolvedUrlRewriteConfPaths, resourceLoader);
             HierarchicalConfiguration proxyConfig = getProxyConfig(siteContext, resolvedProxyConfPaths, resourceLoader);
             HierarchicalConfiguration translationConfig =
                     getTranslationConfig(siteContext, resolvedTranslationConfPaths, resourceLoader);
 
-            siteContext.setScriptFactory(scriptFactory);
             siteContext.setConfig(config);
             siteContext.setGlobalApplicationContext(globalApplicationContext);
-            siteContext.setApplicationContext(appContext);
-            siteContext.setClassLoader(classLoader);
             siteContext.setUrlRewriter(urlRewriter);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/java/org/craftercms/engine/service/context/SiteContextFactory.java`
around lines 351 - 364, Update the site-context initialization flow around
getClassLoader, getScriptFactory, and getApplicationContext to assign each newly
created resource to siteContext immediately after creation. Ensure the
catch-path siteContext.destroy() can clean up resources if a later call such as
getUrlRewriter or getProxyConfig fails, and remove or avoid relying on the
deferred assignments at the end of the block.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/main/java/org/craftercms/engine/service/context/SiteContextFactory.java`:
- Around line 351-364: Update the site-context initialization flow around
getClassLoader, getScriptFactory, and getApplicationContext to assign each newly
created resource to siteContext immediately after creation. Ensure the
catch-path siteContext.destroy() can clean up resources if a later call such as
getUrlRewriter or getProxyConfig fails, and remove or avoid relying on the
deferred assignments at the end of the block.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 37aea899-b0d6-4abc-8efc-68479ff2e817

📥 Commits

Reviewing files that changed from the base of the PR and between 6fe29ef and 5203694.

📒 Files selected for processing (6)

• src/main/java/org/craftercms/engine/scripting/ScriptFactory.java
• src/main/java/org/craftercms/engine/scripting/impl/GroovyScriptFactory.java
• src/main/java/org/craftercms/engine/service/context/SiteContext.java
• src/main/java/org/craftercms/engine/service/context/SiteContextFactory.java
• src/main/java/org/craftercms/engine/service/context/SiteContextManager.java
• src/test/java/org/craftercms/engine/scripting/impl/GroovyScriptFactoryTest.java

[jmendeza]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.