Clear the database appender buffer when committing fails

log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java

@@ -18,10 +18,13 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.ArgumentMatchers.same;
 import static org.mockito.BDDMockito.then;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.spy;
@@ -227,6 +230,61 @@ void testBuffering04() throws Exception {
         then(manager).shouldHaveNoMoreInteractions();
     }
 
+    @Test
+    void testBufferedEventsAreDiscardedWhenCommitFails() throws Exception {
+        setUp("name", 10);
+
+        final LogEvent event1 = mock(LogEvent.class);
+        final LogEvent event2 = mock(LogEvent.class);
+
+        when(event1.toImmutable()).thenReturn(mock(LogEvent.class));
+        when(event2.toImmutable()).thenReturn(mock(LogEvent.class));
+
+        manager.startup();
+        manager.write(event1, null);
+        manager.write(event2, null);
+
+        // The first flush fails while committing the transaction, the next one succeeds.
+        doThrow(new DbAppenderLoggingException("Failed to commit the transaction"))
+                .doReturn(true)
+                .when(manager)
+                .commitAndClose();
+
+        assertThrows(DbAppenderLoggingException.class, manager::flush);
+
+        manager.flush();
+
+        // Events of a transaction that failed to commit must not be sent again.
+        verify(manager, times(2)).writeInternal(any(LogEvent.class), isNull());
+    }
+
+    @Test
+    void testBufferedEventsAreDiscardedWhenConnectFails() throws Exception {
+        setUp("name", 10);
+
+        final LogEvent event1 = mock(LogEvent.class);
+        final LogEvent event2 = mock(LogEvent.class);
+
+        when(event1.toImmutable()).thenReturn(mock(LogEvent.class));
+        when(event2.toImmutable()).thenReturn(mock(LogEvent.class));
+
+        manager.startup();
+        manager.write(event1, null);
+        manager.write(event2, null);
+
+        doThrow(new DbAppenderLoggingException("Failed to connect"))
+                .doNothing()
+                .when(manager)
+                .connectAndStart();
+
+        assertThrows(DbAppenderLoggingException.class, manager::flush);
+
+        manager.flush();
+
+        // Events must not be retried after a failed connection attempt.
+        verify(manager, times(0)).writeInternal(any(LogEvent.class), isNull());
+    }
+
     @Test
     void testStartupShutdown01() throws Exception {
         setUp("testName01", 0);

log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManager.java

@@ -186,14 +186,18 @@ protected void buffer(final LogEvent event) {
     @Override
     public final synchronized void flush() {
         if (this.isRunning() && isBuffered()) {
-            this.connectAndStart();
             try {
-                for (final LogEvent event : this.buffer) {
-                    this.writeInternal(event, layout != null ? layout.toSerializable(event) : null);
+                this.connectAndStart();
+                try {
+                    for (final LogEvent event : this.buffer) {
+                        this.writeInternal(event, layout != null ? layout.toSerializable(event) : null);
+                    }
+                } finally {
+                    this.commitAndClose();
                 }
             } finally {
-                this.commitAndClose();
-                // not sure if this should be done when writing the events failed
+                // The events must not be kept when connecting, writing or committing fails: the next flush would
+                // send them again and the buffer would grow without bound while the failure persists.
                 this.buffer.clear();
             }
         }

src/changelog/.2.x.x/4318_fix_database_appender_buffer_on_failed_commit.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns"
+       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xsi:schemaLocation="
+           https://logging.apache.org/xml/ns
+           https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+    <issue id="4318" link="https://github.com/apache/logging-log4j2/issues/4318"/>
+    <description format="asciidoc">
+        Clear the buffer of a buffered database appender when connecting or committing a transaction fails.
+        Previously the events were kept, so the next flush sent them again and the buffer grew without bound
+        while the database failure persisted.
+    </description>
+</entry>