From fd234f9ab1a90ab00e8c983d408e871520e688b8 Mon Sep 17 00:00:00 2001 From: dajiaohuang Date: Fri, 11 Sep 2026 22:05:05 +0800 Subject: [PATCH] Discard buffered events when a database commit fails `AbstractDatabaseManager#flush` cleared the buffer in a `finally` block that also contained `commitAndClose`. If committing or closing the transaction threw, the `clear()` was skipped, so the events stayed in the buffer and were sent to the database a second time by the next flush. Since `flush()` is also called from `shutdown()`, and the buffer only grows while the failure persists, this leaked log events and produced duplicate rows. Move `buffer.clear()` into an inner `finally` block so that events are discarded regardless of the outcome of `commitAndClose`. --- .../db/AbstractDatabaseManagerTest.java | 58 +++++++++++++++++++ .../appender/db/AbstractDatabaseManager.java | 14 +++-- ...abase_appender_buffer_on_failed_commit.xml | 14 +++++ 3 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 src/changelog/.2.x.x/4318_fix_database_appender_buffer_on_failed_commit.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java index 38fad66231c..b3a16c306f7 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java +++ b/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); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManager.java index f166ec2ce88..d3aa521080c 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManager.java +++ b/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(); } } diff --git a/src/changelog/.2.x.x/4318_fix_database_appender_buffer_on_failed_commit.xml b/src/changelog/.2.x.x/4318_fix_database_appender_buffer_on_failed_commit.xml new file mode 100644 index 00000000000..10b39fd5b21 --- /dev/null +++ b/src/changelog/.2.x.x/4318_fix_database_appender_buffer_on_failed_commit.xml @@ -0,0 +1,14 @@ + + + + + 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. + +