BUG: AtomicBoolean Misuse in PreparedStatementExecutor - Batch Error State Can Be Lost

[minguyen9988]

Summary

PreparedStatementExecutor.executePreparedStatement() uses an AtomicBoolean to track the success/failure of batch execution across lambda iterations in Lists.partition().forEach(). However, the AtomicBoolean is only set to true on success (line 241) and never reset to false after a failure in a subsequent batch partition. This means if an early partition succeeds but a later partition fails, result.get() at line 295 returns true (the value from the successful partition), masking the failure.

Location

• File: sink-connector/src/main/java/com/altinity/clickhouse/sink/connector/db/batch/PreparedStatementExecutor.java
• Lines: 151, 241, 295

Current Code

// Line 151: AtomicBoolean initialized to false
AtomicBoolean result = new AtomicBoolean(false);

// Inside forEach lambda (line 155):
Lists.partition(entry.getValue(), (int)maxRecordsInBatch).forEach(batch -> {
    // ...
    try (PreparedStatement ps = metadata.getPreparedStatement(conn, insertQuery)) {
        // ... process batch ...
        int[] batchResult = ps.executeBatch();
        result.set(true);    // Line 241: Set to true on success
        // ...
    } catch (RuntimeException e) {
        // Adds to failedRecords but does NOT set result to false
        throw e;             // This exits the lambda but forEach continues
    } catch (Exception e) {
        // Adds to failedRecords but does NOT set result to false  
        throw new RuntimeException(e);
    }
});

return result.get();          // Line 295: May return true even if later batches failed

Problem

1. The AtomicBoolean is only set to true on success but never explicitly set to false on failure - it relies on the initial value of false, which is overwritten by any successful partition.
2. While the current code does throw RuntimeException on failure which would prevent forEach from continuing to the next partition (since RuntimeException propagates out of the lambda), the use of AtomicBoolean here is semantically incorrect and fragile - any refactoring that catches the exception or uses a different iteration pattern could expose this bug.
3. The AtomicBoolean is unnecessary for this pattern since the lambda already throws on failure. A simple boolean would suffice, but the real issue is that the method conflates two error-handling strategies (exception throwing AND boolean return value).

Impact

• Severity: Medium (P2)
• Effect: Currently mitigated by the RuntimeException throw, but fragile - any future refactoring could cause silent data loss where failed batch inserts are reported as successful
• Scope: All batch insert operations

Suggested Fix

Replace the AtomicBoolean pattern with a simple for-loop and boolean, making the control flow explicit:

private boolean executePreparedStatement(...) throws Exception {
    List<List<ClickHouseStruct>> partitions = 
        Lists.partition(entry.getValue(), (int)maxRecordsInBatch);
    
    for (List<ClickHouseStruct> batch : partitions) {
        try (PreparedStatement ps = metadata.getPreparedStatement(conn, insertQuery)) {
            // ... process batch ...
            ps.executeBatch();
        } catch (Exception e) {
            // Handle failure, return false
            return false;
        }
    }
    return true;
}

References

• doc/issues/CONCURRENCY-BUGS.md (BUG-CONC-3: AtomicBoolean Misuse)
• doc/issues/ANTI-PATTERNS.md (related to error handling patterns)