From f943cc23682b0c3cde04cd910ac2b707f0423882 Mon Sep 17 00:00:00 2001 From: Robert Tupelo-Schneck Date: Tue, 28 Jul 2026 18:18:31 -0400 Subject: [PATCH 1/2] Name direct write cron files after the rollover period they cover An appender configured without a `fileName` writes directly to the file its pattern resolves to. `CronTriggeringPolicy.initialize()` records the start of the current rollover period as the pattern processor's current file time, but `DirectWriteRolloverStrategy.getCurrentFileName()` overwrote it with the current time, so the file was named after the moment the appender started rather than after the period it covers. For a weekly schedule and a `%d{yyyyMMdd}` pattern that means every restart on a new day opens another file, leaving a week with as many files as there were restart days rather than the single file the schedule implies. That override (LOG4J2-3339) compensated for `rollover()` passing the previous roll date as the new file's time, which named each new file after the file just rolled. Pass the roll time instead, so the replacement file is named after the period it opens, and let `getCurrentFileName()` use the recorded period start. Policies that do not track a period, such as `TimeBasedTriggeringPolicy`, leave the current file time at 0, and `PatternProcessor.formatFileName()` already falls back to the current time in that case, so their behaviour is unchanged. Note that this changes direct write file names for cron based appenders. Co-Authored-By: Claude Opus 5 --- .../rolling/CronTriggeringPolicyTest.java | 28 +++++++++++++++++++ .../rolling/CronTriggeringPolicy.java | 4 ++- .../rolling/DirectWriteRolloverStrategy.java | 6 ++-- ...ix_direct_write_cron_current_file_name.xml | 14 ++++++++++ 4 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java index 65f3ccb910f..37a97c6cd66 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java @@ -16,13 +16,17 @@ */ package org.apache.logging.log4j.core.appender.rolling; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.text.SimpleDateFormat; +import java.util.Date; import org.apache.logging.log4j.core.appender.RollingFileAppender; import org.apache.logging.log4j.core.appender.RollingRandomAccessFileAppender; import org.apache.logging.log4j.core.config.Configurator; import org.apache.logging.log4j.core.config.NullConfiguration; import org.apache.logging.log4j.core.layout.PatternLayout; +import org.apache.logging.log4j.core.util.CronExpression; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -72,6 +76,30 @@ void testBuilderOnce() { testBuilder(); } + /** + * Without a {@code fileName} the appender writes directly to the file its pattern resolves to. + * That file covers the whole rollover period, so it must be named after the period's start + * rather than after the moment the appender happened to start. Otherwise a restart later in + * the period opens a second file for a period that is supposed to have only one. + */ + @Test + void testDirectWriteFileNameUsesPeriodStart() throws Exception { + final String schedule = "0 0 0 ? * SUN"; + // @formatter:off + final RollingFileAppender raf = RollingFileAppender.newBuilder() + .setName("test5") + .setFilePattern("target/testcmd5.log-%d{yyyyMMdd}") + .setPolicy(CronTriggeringPolicy.createPolicy(configuration, Boolean.FALSE.toString(), schedule)) + .setConfiguration(configuration) + .build(); + // @formatter:on + assertNotNull(raf); + + final Date periodStart = new CronExpression(schedule).getPrevFireTime(new Date()); + final String expected = "target/testcmd5.log-" + new SimpleDateFormat("yyyyMMdd").format(periodStart); + assertEquals(expected, raf.getManager().getFileName()); + } + /** * Tests LOG4J2-1740 Add CronTriggeringPolicy programmatically leads to NPE */ diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java index 7ec778764e1..cd99b371762 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java @@ -150,7 +150,9 @@ private static CronExpression getSchedule(final String expression) { private void rollover() { // If possible, use the time rollover was supposed to occur, not the actual time. final Date rollTime = future != null ? future.getFireTime() : new Date(); - manager.rollover(cronExpression.getPrevFireTime(rollTime), lastRollDate); + // The file being rolled covers the period that ends at `rollTime`, so it is named after + // that period's start. The file replacing it opens the period beginning at `rollTime`. + manager.rollover(cronExpression.getPrevFireTime(rollTime), rollTime); if (future != null) { lastRollDate = future.getFireTime(); } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java index e105fa455b1..59600b69e74 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java @@ -390,8 +390,10 @@ public String getCurrentFileName(final RollingFileManager manager) { final SortedMap eligibleFiles = getEligibleFiles(manager); final int fileIndex = eligibleFiles.size() > 0 ? (nextIndex > 0 ? nextIndex : eligibleFiles.lastKey()) : 1; final StringBuilder buf = new StringBuilder(255); - // LOG4J2-3339 - Always use the current time for new direct write files. - manager.getPatternProcessor().setCurrentFileTime(System.currentTimeMillis()); + // Name the file after the start of the rollover period it belongs to, which the + // triggering policy records as the pattern processor's current file time. Policies + // that do not track a period leave that value at 0, and `formatFileName()` then + // falls back to the current time on its own. manager.getPatternProcessor().formatFileName(strSubstitutor, buf, true, fileIndex); final int suffixLength = suffixLength(buf.toString()); final String name = suffixLength > 0 ? buf.substring(0, buf.length() - suffixLength) : buf.toString(); diff --git a/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml b/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml new file mode 100644 index 00000000000..7746e5e7cd5 --- /dev/null +++ b/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml @@ -0,0 +1,14 @@ + + + + Fix the file a `CronTriggeringPolicy` appender without a `fileName` writes to. The file is now named after + the start of the rollover period it covers, instead of after the moment the appender started, so restarting + within a period reopens that period's file rather than creating another one. Note that this changes the + names of direct write files for such appenders. + + From 6da4b6c4a9e2aa19fba91a17fdbbc64424aa506d Mon Sep 17 00:00:00 2001 From: Robert Tupelo-Schneck Date: Tue, 28 Jul 2026 18:44:16 -0400 Subject: [PATCH 2/2] Reference PR 4227 in the changelog entry Co-Authored-By: Claude Opus 5 --- src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml b/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml index 7746e5e7cd5..cb4271a1329 100644 --- a/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml +++ b/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml @@ -5,6 +5,7 @@ https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd" type="fixed"> + Fix the file a `CronTriggeringPolicy` appender without a `fileName` writes to. The file is now named after the start of the rollover period it covers, instead of after the moment the appender started, so restarting