Race condition in UserAPIImpl.getSystemUser() causes flaky test failures in PublisherTest.testPushPublishWithUniqueField

Summary

The PublisherTest.testPushPublishWithUniqueField test fails intermittently with a NullPointerException due to memory visibility issues and race conditions in the non-synchronized lazy initialization of the system user in UserAPIImpl.getSystemUser().

Root Cause Analysis

Critical Issue: Missing volatile Keyword Causes Memory Visibility Problems

The core problem at line 67:

static User systemUser, anonUser=null;  // ← NOT VOLATILE!

Without volatile, writes to systemUser are not guaranteed to be visible across threads, leading to:

• Thread A sets systemUser = validUser but the write remains in CPU cache
• Thread B reads systemUser and still sees null due to cache coherency issues
• Same thread could even see inconsistent values due to compiler optimizations

Race Condition in Lazy Initialization

Race condition in UserAPIImpl.getSystemUser() at lines 231-236:

public User getSystemUser() throws DotDataException {
    if(this.systemUser==null){          // ← RACE CONDITION HERE
        this.systemUser=_getSystemUser(); // Multiple threads can enter this block
    }
    return this.systemUser;
}

How Memory Visibility Issues Cause Null Returns

Scenario 1: Cross-Thread Visibility Problem

1. Thread A: Calls getSystemUser(), sees systemUser == null
2. Thread A: Calls _getSystemUser(), gets valid user, sets systemUser = validUser
3. Thread B: Calls getSystemUser(), but due to missing volatile, still sees systemUser == null
4. Thread B: Calls _getSystemUser() again (unnecessary work)
5. Result: Both get valid users, but causes performance issues and potential timing problems

Scenario 2: Compiler Optimization Problem

// Original code:
if(this.systemUser==null){
    this.systemUser=_getSystemUser(); 
}
return this.systemUser;

// Compiler might optimize to (without volatile):
User temp = this.systemUser;  // Read once into local variable
if(temp==null){
    this.systemUser=_getSystemUser();  // Sets field but doesn't update temp
}
return temp;  // Returns the old null value!

This explains how getSystemUser() can return null even after _getSystemUser() successfully creates and stores a valid user.

Why System User Remains Null "For So Long"

Memory visibility issues compound over time:

1. Initial write to systemUser may not flush from CPU cache to main memory
2. Subsequent reads from other cores continue seeing stale null value
3. Cache coherency protocols may not sync the value immediately
4. JIT compiler optimizations assume the field won't change without proper synchronization

This creates a persistent state where:

• systemUser field actually contains a valid User object
• Multiple threads continue to see null and repeatedly call _getSystemUser()
• Eventually one thread's read happens to see the updated value, but others still see null

Code Quality Issues

1. Static Field Accessed via Instance Reference (Anti-pattern)

if(this.systemUser==null){  // ← Accessing static field via 'this.'

This anti-pattern:

• Misleads readers about the field's scope and lifecycle
• Hides the shared state nature of static fields
• Makes threading issues less obvious to developers
• Violates Java best practices for static field access

2. Missing Thread Safety Mechanisms

• No volatile for memory visibility guarantee
• No synchronization for atomic read-check-write operations
• Race conditions in lazy initialization

Failure Path

testPushPublishWithUniqueField() → publishContentBundle() → BundlePublisher.process() → 
ContentTypeHandler.handle() → FieldAPIImpl.sendToast() → 
APILocator.systemUser() → UserAPIImpl.getSystemUser() → 
MEMORY VISIBILITY ISSUE → returns null → NPE

The NPE occurs in FieldAPIImpl.sendToast() at line 1166 when calling user.getUserId() on a null user object returned due to memory visibility problems.

Evidence

1. Timing-Dependent Failure Pattern

• Failing run: testPushPublishWithUniqueField runs at 20:44:30.152 and FAILS at 20:44:34.169 with NPE
• Working run: testPushPublishWithUniqueField runs at 23:37:22.733 and SUCCEEDS completely

2. No Exceptions Elsewhere

Critical observation: If APILocator.systemUser() was throwing exceptions due to failed user creation, we would see those exceptions throughout the logs since systemUser() is called in many places without Try.getOrElse() wrappers.

The fact that we only see the single NPE proves that:

• System user creation itself works correctly
• The issue is memory visibility, not logic errors
• Other calls to systemUser() succeed because they happen to see the valid cached value

3. Multiple Failure Attempts in Same Run

The failing test run shows multiple attempts of the same test, all failing with the same memory visibility issue:

• First attempt: 20:31:52.490 (fails)
• Second attempt: 20:44:30.152 - FAILS at 20:44:34.169 with NPE
• Third attempt: 20:44:50.378 - FAILS at 20:44:53.939 with NPE

This persistence across attempts strongly indicates memory visibility problems rather than transient errors.

4. Consistent Startup Behavior

All test runs (both working and failing) show identical startup warnings:

• "LocaleUtil - null is not a valid language id"
• "Error loading user with id system"

This confirms these warnings are normal startup behavior, not the cause of the failure.

Stack Trace

java.lang.NullPointerException: Cannot invoke "com.liferay.portal.model.User.getUserId()" because "user" is null
	at com.dotcms.contenttype.business.FieldAPIImpl.sendToast(FieldAPIImpl.java:1166)
	at com.dotcms.contenttype.business.FieldAPIImpl.save(FieldAPIImpl.java:295)
	at com.dotcms.contenttype.business.ContentTypeAPIImpl.save(ContentTypeAPIImpl.java:378)
	at com.dotcms.enterprise.publishing.remote.handler.ContentTypeHandler.saveOrUpdateContentType(ContentTypeHandler.java:299)
	at com.dotcms.enterprise.publishing.remote.handler.ContentTypeHandler.handleContentTypes(ContentTypeHandler.java:165)
	at com.dotcms.enterprise.publishing.remote.handler.ContentTypeHandler.handle(ContentTypeHandler.java:114)
	at com.dotcms.publisher.receiver.BundlePublisher.process(BundlePublisher.java:225)
	at com.dotcms.publisher.business.PublisherTest.publishContentBundle(PublisherTest.java:736)
	at com.dotcms.publisher.business.PublisherTest.testPushPublishWithUniqueField(PublisherTest.java:594)

Impact

• Flaky test behavior: Test passes or fails depending on CPU cache timing and thread scheduling
• CI/CD pipeline instability: Random test failures block legitimate changes
• Potential production issues: Same memory visibility issues could affect production system user access
• Code maintainability: Anti-patterns make threading issues harder to identify and debug
• Performance degradation: Multiple threads unnecessarily calling _getSystemUser() due to cache misses

Files Involved

• dotCMS/src/main/java/com/dotmarketing/business/UserAPIImpl.java:67 - Missing volatile static field
• dotCMS/src/main/java/com/dotmarketing/business/UserAPIImpl.java:231-236 - Race condition location
• dotCMS/src/main/java/com/dotcms/contenttype/business/FieldAPIImpl.java:1166 - NPE location
• dotcms-integration/src/test/java/com/dotcms/publisher/business/PublisherTest.java:594 - Failing test

Recommended Solution

Fix both memory visibility and race condition issues:

// 1. Add volatile for guaranteed memory visibility
private static volatile User systemUser = null;
private static volatile User anonUser = null;

// 2. Proper thread-safe lazy initialization  
public User getSystemUser() throws DotDataException {
    if(systemUser == null) {  // Remove 'this.' - access static field directly
        synchronized(UserAPIImpl.class) {  // Synchronize on class for static fields
            if(systemUser == null) {
                systemUser = _getSystemUser();
            }
        }
    }
    return systemUser;
}

Key improvements:

1. Add volatile - Guarantees memory visibility across all threads and prevents compiler optimizations that could return stale values
2. Remove this. prefix - Access static field directly to clarify it's shared state
3. Synchronize on class object - Use UserAPIImpl.class for static field synchronization
4. Double-checked locking - Minimize synchronization overhead while ensuring thread safety

Why volatile is critical:

• Memory visibility: Ensures writes are immediately visible to all threads
• Prevents compiler optimizations: Stops compiler from caching values in registers/local variables
• Guarantees atomic reads/writes: Ensures consistent view of the field across threads

Priority

Critical - This affects core system functionality with memory visibility issues that can cause unpredictable system behavior, persistent test instability, and potential production issues.

🤖 Generated with Claude Code

Start with UserAPIImpl.java at lines 67 and 231-236, then run PublisherTest.testPushPublishWithUniqueField in dotcms-integration/src/test/java/com/dotcms/publisher/business/PublisherTest.java. Trace the system-user call through APILocator.systemUser() and FieldAPIImpl.java:1166. Done means the intermittent NPE is no longer reproducible and the relevant test passes reliably under concurrent execution.