Skip to content

fix(velocity): fail loud when global macro library load fails (#35601)#35768

Open
dsolistorres wants to merge 2 commits into
mainfrom
issue-35601-velocity-fail-loud
Open

fix(velocity): fail loud when global macro library load fails (#35601)#35768
dsolistorres wants to merge 2 commits into
mainfrom
issue-35601-velocity-fail-loud

Conversation

@dsolistorres
Copy link
Copy Markdown
Member

Summary

Closes #35601 (follow-up implementation from spike #35329).

VelocimacroFactory silently swallowed ResourceNotFoundException when loading the configured velocimacro.library files at engine init, allowing engine.init() to return successfully with no global macros registered. After K8s pod restarts with transient I/O errors (notably on EFS-backed volumes), #renderMarks($element) and #editContentlet rendered as literal text on every public page until the JVM was restarted.

Changes

  • VelocimacroFactory.initVelocimacro() — track loaded/failed libraries through the load loop. The ResourceNotFoundException catch now logs at ERROR with the failed library file name. After the loop, an INFO summary line lists loaded and failed libraries (greppable for ops). When any library failed, throw VelocityException with an actionable message naming the failed library and the opt-out flag.
  • New flag velocimacro.library.fail-on-missing (default true) preserves the legacy silent-warn behavior for self-hosted operators who intentionally configure optional libraries.
  • RuntimeConstants.VM_LIBRARY_FAIL_ON_MISSING — new constant.
  • system.properties — explicit default with a one-line comment.
  • VelocimacroFactoryTest (new) — three unit tests covering fail-on-missing=true, fail-on-missing=false, and the default.

Why this is enough

engine.init() throws → VelocityUtil.getEngine() (VelocityUtil.java:65) rethrows as DotRuntimeException → propagates out of InitServlet.init() at line 199 → dotcms.started.up is never set → existing ServletContainerHealthCheck already gates /readyz on that flag → K8s readiness probe fails → no traffic routed → eventual pod restart gives EFS time to settle.

Companion ticket #35602 (VelocityHealthCheck) is still valuable as defense-in-depth — it catches the opt-out case (fail-on-missing=false) and any future post-init macro corruption that this flag wouldn't see.

Test plan

  • VelocimacroFactoryTest — 3/3 pass (fail-on-missing=true throws, false succeeds, default throws). Runtime ERROR/INFO log lines verified in test output.
  • VelocityUtilTest — 20/20 pass (no regression).
  • Local smoke test on real dotCMS startup: temporarily added bogus-smoke-test.vm to velocimacro.library, ran just dev-run. Observed:
    • ERROR runtime.VelocimacroFactory - Velocimacro : VM library not found : bogus-smoke-test.vm : ...
    • INFO runtime.VelocimacroFactory - Velocimacro libraries loaded: [VM_global_library.vm, dotCMS_library.vm, dotCMS_library_ext.vm] ; failed: [bogus-smoke-test.vm]
    • VelocityException: Velocimacro : required VM libraries failed to load: [bogus-smoke-test.vm]. Set velocimacro.library.fail-on-missing=false to allow startup with missing libraries.
    • Stack trace propagation through InitServlet.init(InitServlet.java:199) and rethrow as DotRuntimeException
    • InitServlet init Completed absent from log (init aborted before dotcms.started.up=true)
    • /dotmgt/readyz returns HTTP 503
    • /dotmgt/livez returns HTTP 503

🤖 Generated with Claude Code

VelocimacroFactory silently swallowed ResourceNotFoundException when loading
velocimacro.library files at engine init, allowing init to "succeed" with no
global macros registered. After K8s pod restarts with transient I/O errors
(notably on EFS-backed volumes), #renderMarks and #editContentlet rendered as
literal text on every page until the JVM was restarted.

The catch now logs the failed library name at ERROR and tracks failures so
init can throw VelocityException when any required library failed. A new
flag velocimacro.library.fail-on-missing (default true) lets self-hosted
operators preserve the legacy silent-warn behavior if they intentionally
configure optional libraries.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 20, 2026

Claude finished @dsolistorres's task in 2m 48s —— View job


dotCMS Backend Review

  • Load dotCMS conventions
  • Get Java diff for PR (167 lines, 3 files)
  • Run Security, Database, Java Standards, REST sub-agents in parallel
  • Consolidate findings and post review comment (marker comment updated)
  • Submit formal GitHub review — blocked: this action lacks permission to submit formal reviews (FAQ)

Result

All four sub-agents (Security, Database, Java Standards, REST) returned NO_FINDINGS for the Java diff against origin/main.

The earlier 🟡 Medium diamond-operator nit was already addressed in commit fbb7c2a2. The current changes are clean:

  • RuntimeConstants.java — new property-key constant only
  • VelocimacroFactory.java — fail-loud catch-block + summary log + opt-out flag, no new security/DB/REST surface
  • VelocimacroFactoryTest.java — standard JUnit 4 + Mockito test, Java 11 compatible
    · branch issue-35601-velocity-fail-loud

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 20, 2026

dotCMS Backend Review: no issues found.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dsilvam dsilvam added this pull request to the merge queue May 20, 2026
@wezell wezell removed this pull request from the merge queue due to a manual request May 20, 2026
# When true (default), engine init throws VelocityException if any velocimacro.library
# file fails to load — prevents silent macro-rendering failures on pod restart (issue #35601).
# Set to false only if you intentionally configure optional/missing library files.
velocimacro.library.fail-on-missing=true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a feature flag and should use `Config.getBooleanProperty("XXXX")``

"Velocimacro libraries loaded: " + loadedLibraries
+ " ; failed: " + failedLibraries);

if (failOnMissing && !failedLibraries.isEmpty())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change - who knows how many dotCMS customers would fail this and their sites stop rendering when we updated them? Before we change default behavior, we need to make sure we understand what we're doing, if it could result in breakage and and gate it properly. This needs to default to off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Velocity: fail loud when global macro library load fails (follow-up to spike #35329)

3 participants