Skip to content

count only the current date's backups when RollingFileAppender starts - #334

Open
Snotface wants to merge 3 commits into
apache:masterfrom
Snotface:feature/apache-date-aware-backup-index
Open

Snotface wants to merge 3 commits into
apache:masterfrom
Snotface:feature/apache-date-aware-backup-index

Conversation

@Snotface

@Snotface Snotface commented Oct 4, 2026

Copy link
Copy Markdown

On start-up, RollingFileAppender counts the size backups already on disk so the next roll continues from the highest one. It reads the number after the last . of any matching file, which goes wrong in three ways:

  1. Dates read as backup numbers. A date pattern without separators (.yyyyMMdd) makes log.txt.20261003 look like backup 20261003. Counting up, the next backup is named .20261004. Counting down, every size roll walks the names from 20 million down to 1: stock 2.0.5 made about 1.8 million rename attempts on one roll before I stopped it.
  2. Backups lost on restart. With StaticLogFileName false, a date pattern that supplies the extension (.yyyy-MM-dd'.log') and PreserveLogFileNameExtension, the current date's backups (log.2026-10-04.1.log) are skipped as "from a different date period". The count restarts at 0 and the next size roll deletes the existing .1. In a simulation of four app starts over two days, 114 of 160 lines survived, in both 2.0.5 and 3.5.1.
  3. Earlier dates counted as today's. With a static file name, backups already rolled to an earlier date (log.txt.20261003.3) raise the current date's count, which leaves gaps in the numbering.

Change

GetBackupIndex now builds the name the current date's backups must have, the same way the appender names them (the base file, plus today's date when StaticLogFileName is false, with the extension handled as PreserveLogFileNameExtension does). It counts only files with that name plus .N. With a static file name, a suffix that parses as a date in DatePattern (DateTime.TryParseExact) is a dated file, not a backup.

Short date patterns

With a static file name, a date pattern that formats as a short number (.dd, .MM, .HH, .yyyy) names the dated file log.txt.10 exactly like backup 10, so one overwrites the other. No file name check can tell them apart. Numbered backups are made by size rolls, and at start-up when AppendToFile is false. So when the file name is static, the appender rolls by date, MaxSizeRollBackups isn't 0, and the pattern can format as a dot plus at most four digits, it makes no numbered backups:

  • size rolling is switched off and MaximumFileSize is ignored
  • when AppendToFile is false, the existing file is appended to at start-up instead of being rolled out of the way
  • the reason is written as a FATAL line at the top of each new file, past the appender's filters and threshold, and as a log4net internal warning

Logging carries on and nothing throws. A configuration that never makes numbered backups (Date style with AppendToFile true) is not affected.

The new IgnoreDateWarnings property (default false) keeps the configured behaviour, exactly as before this change, for those who accept the risk.

The RollingFileAppender manual page has a new "Short date patterns" section, and there's a changelog entry for 3.5.1.

Testing

  • log4net.Tests: net462 470 passed, 2 skipped; net10.0 454 passed, 1 skipped. log4net.Ext.Mail.Tests: 52 passed. The Release build of the solution has 0 warnings.
  • New tests cover:
    • .yyyyMMdd with limited and unlimited backups, in both count directions
    • counting up past the backup limit
    • ignoring earlier dates
    • a date pattern that supplies the extension
    • a short pattern with a non-static name
    • a short pattern switching size rolling off
    • appending instead of rolling at start-up (Date and Composite)
    • IgnoreDateWarnings
  • Each scenario was also simulated through the built DLL, with app restarts and date changes on a fake clock. Every configuration kept every line, and IgnoreDateWarnings produced the same files as stock 2.0.5.

🤖 Generated with Claude Code

Snotface and others added 3 commits October 4, 2026 20:31
On start-up the appender counts the size backups already on disk and
continues from the highest. It read the number after the last "." of any
matching file, which went wrong in three ways:

- a date pattern without separators (.yyyyMMdd) was read as a backup
  number: counting up the next backup was named .20261004, counting down
  every size roll walked twenty million names
- with staticLogFileName false, a date pattern that supplies the extension
  (.yyyy-MM-dd'.log') and preserveLogFileNameExtension, the current date's
  backups were skipped as "from a different date period", so the count
  restarted at 0 and the next roll deleted the existing backups
- with a static file name, backups already rolled to an earlier date
  raised the current date's count

GetBackupIndex now builds the name the current date's backups must have,
as the appender names them, and counts only files of that name plus .N.
With a static file name, a suffix that parses as a date in DatePattern is
a dated file, not a backup.

A date pattern that formats as a short number (.dd, .MM, .HH, .yyyy)
names the dated file log.txt.10 exactly like backup 10, so one overwrites
the other. For such configurations no numbered backups are made: size
rolling is switched off, the existing file is appended to at start-up when
appendToFile is false, and the reason is written as a FATAL line at the
top of each file and as an internal warning. The new IgnoreDateWarnings
property keeps the configured behaviour for those who accept the risk.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
FreeAndNil added a commit that referenced this pull request Oct 4, 2026
…334

InitializeRollBackups took dated files and other dates' backups for
backups of the current roll point:

* A date without separators (.yyyyMMdd) parsed as a backup index, with a
  static or a non-static name, so the count jumped to 20261004.
* With a static name, log.txt.20261003.1 counted as backup 1.
* A date pattern supplying the extension (.yyyy-MM-dd'.log' with
  PreserveLogFileNameExtension) hid the current date's backups.

Dates of up to four characters (.dd) still count as indexes, they cannot
be told apart. Tests by @Snotface.
FreeAndNil added a commit that referenced this pull request Oct 4, 2026
InitializeRollBackups took dated files and other dates' backups for
backups of the current roll point:

* A date without separators (.yyyyMMdd) parsed as a backup index, with a
  static or a non-static name, so the count jumped to 20261004.
* With a static name, log.txt.20261003.1 counted as backup 1.
* A date pattern supplying the extension (.yyyy-MM-dd'.log' with
  PreserveLogFileNameExtension) hid the current date's backups.

Dates of up to four characters (.dd) still count as indexes, they cannot
be told apart. Tests by @Snotface.
@FreeAndNil

Copy link
Copy Markdown
Contributor

Thanks @Snotface, the backup counting bugs you found are real and your InitializeRollBackups tests pin them down well.

The PR is more than we want in RollingFileAppender for a patch release though:

  • a new public property
  • size rolling switched off
  • a FATAL line in the user's log
  • false positives: .yyyy with maxSizeRollBackups 10 can never meet backup 2024

Here is a smaller fix for the same bugs on top of your branch, with your tests and a changelog entry: ba511c5

Its diff shows what changed against your version. Could you merge or cherry-pick it into your branch and push? It applies without conflicts, and we squash when merging the PR.

git switch feature/apache-date-aware-backup-index
git pull https://github.com/apache/logging-log4net.git Feature/334-rollingfileappender-backup-index
git push

The short date collision (.dd) is existing behaviour and deserves its own issue.

One more request: please keep PR descriptions and changelog entries short. A few lines on what is broken and how it is fixed are much easier to review than a long write-up.

@FreeAndNil

Copy link
Copy Markdown
Contributor

@Snotface are you still interested in merging this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants