Skip to content

use a dated file name when the date pattern names files like backups - #336

Merged
FreeAndNil merged 3 commits into
masterfrom
Feature/335-short-date-pattern-backup-collision
Oct 11, 2026
Merged

FreeAndNil merged 3 commits into
masterfrom
Feature/335-short-date-pattern-backup-collision

Conversation

@FreeAndNil

Copy link
Copy Markdown
Contributor

Fixes #335. With StaticLogFileName, a DatePattern such as .dd names the file of the 10th log.txt.10, which is also backup 10, and a roll overwrote one with the other.

  • warns at activation and sets StaticLogFileName to false, so the live file is the dated one and its backups are numbered after the date
  • no report for a count-down limit below the smallest dated name (.dd with 5 backups)
  • existing static file and its backups left untouched
  • startup scan of a dated name requires a dot after the name, so with .d the files of the 11th no longer count as backups of the 1st

…335

With StaticLogFileName, .dd names the file of the 10th like backup 10,
and a roll overwrote one with the other.

* warn at activation and set StaticLogFileName to false
* no report for a count-down limit below the smallest dated name
* existing static file and its backups left untouched
* startup scan of a dated name requires a dot after the name (.d)
* AGENTS.md: rule and example
* InitializeFromOneFile: IsLogFile, IsBaseFile, IsBackupOfDatedFile
@FreeAndNil FreeAndNil added this to the 3.5.1 milestone Oct 10, 2026

@gdziadkiewicz gdziadkiewicz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, asking Copilot to also take a look

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The single-date heuristic produces false positives and still misses longer numeric patterns that can collide with unbounded count-up backups.

3 open findings
What changed in this PR

Fixes short date patterns colliding with numbered rolling-file backups.

Changes:

  • Detects collisions and switches to dated filenames.
  • Tightens dated-backup scanning and adds regression tests.
  • Updates documentation, changelog, and contributor guidance.
File Description
AGENTS.md Documents conditional-logic style.
RollingFileAppender.cs Adds collision handling and scan fixes.
RollingFileAppenderShortDatePatternTest.cs Tests collision and startup behavior.
rollingfileappender.adoc Documents short-pattern handling.
335-short-date-pattern-collides-with-backup-names.xml Records the fix.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/log4net/Appender/RollingFileAppender.cs
Comment thread src/changelog/3.5.1/335-short-date-pattern-collides-with-backup-names.xml Outdated
Count-up backup indices are unbounded, so a numeric .yyyyM meets backup
20261 one day.

* warning, changelog and manual name the default .yyyy-MM-dd
@FreeAndNil
FreeAndNil merged commit 353443f into master Oct 11, 2026
3 checks passed
@FreeAndNil
FreeAndNil deleted the Feature/335-short-date-pattern-backup-collision branch October 11, 2026 18:37
@FreeAndNil

FreeAndNil commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor Author

Implementation plan for #335, kept here for the record. It covers the change, the tests, the
review round on this pull request and the verification steps.

Plan

Plan for #335: short date pattern collides with size backup names

Context

Issue #335 (split out of #334, reported by @Snotface): with StaticLogFileName true, a
DatePattern that formats as a dot and up to four digits (.dd, .MM, .HH, .d) names dated
files like numbered size backups: log.txt.10 is both the file for the 10th and backup 10.

Why files get destroyed (src/log4net/Appender/RollingFileAppender.cs, at d0efa655):

  • RollFile deletes its target before File.Move, so a size roll onto log.txt.10 destroys the
    dated file, and a time roll onto log.txt.10 destroys backup 10.
  • DetermineCurSizeRollBackups counts the dated files as backups, so RollOverRenameFiles
    deletes the "oldest backup" log.txt.10 and RollOverTime moves log.txt.1..Cur as backups.
  • Affected: Composite, and Date with AppendToFile false (the startup roll-out in
    ExistingInit makes numbered backups when MaxSizeRollBackups is not 0).

Decision: report at activation and switch to a dated file name, that is set
StaticLogFileName to false, which is what the warning tells the deployer to do anyway. Size
rolling and the backup limit keep working, the live file is log.txt.10 and its backups are
log.txt.10.1 and so on, nothing is lost and the file stays bounded. Disabling backups instead
would leave the file growing until the next date roll, and refusing to activate would stop logging
for a configuration that worked yesterday. The cost is the changed live file name, which the
warning states, and old backups of the static name that stay in the folder untouched.

Not a collision, and must not be reported: the default count-down setup with a finite limit. With
CountDirection -1 and MaxSizeRollBackups N only .1 to .N exist, and the dated names of a
k-digit sample start at 10^(k-1) (.dd gives .01, which is not .1). So .dd with 5 backups
is fine, .dd with 10 collides, .d always collides, CountDirection 0 or 1 and
MaxSizeRollBackups -1 always collide. Five or more digits are not detected: that is where
IsDateSuffix (#334) stops counting dated files as backups, so detection and counting agree; the
theoretical collision of a numeric .yyyyMMdd with backup 20260101 under count-up is accepted,
and the advice names a separator, never "more digits".

Known and accepted: the 1970 sample is the narrowest formatting of every numeric field except
12-hour h, which is unusable without tt; .Md samples as .11 and over-reports a count-down
limit of exactly 10, one warning for an ambiguous pattern. A second ActivateOptions on the same
instance after the pattern is corrected keeps StaticLogFileName false; configurators build a new
instance.

A second defect, pre-existing from #334, surfaces as soon as .d users get a dated name: the
non-static start-up scan in InitializeFromOneFile accepts every file that starts with the dated
name, so on the 1st the files of the 11th to 19th and their backups count as backups of the 1st.
Fixed here, since the switch sends static .d users there.

Steps

0. Branch

git switch -c Feature/335-short-date-pattern-backup-collision master at d0efa655.

1. RollingFileAppender: detect, report, switch

In ActivateOptions, after the ComputeCheckPeriod block and before SecurityContext ??=:

if (DoesDatePatternCollideWithBackupNames())
{
  // LogLog, not ErrorHandler: OnlyOnceErrorHandler would report nothing after this one
  LogLog.Warn(_declaringType, $"""
    DatePattern [{DatePattern}] of [{Name}] names dated files like numbered backups, so the
    appender uses a dated file name as with StaticLogFileName false. Use a DatePattern with
    a separator, such as the default .yyyy-MM-dd, to keep the static name.
    """);
  StaticLogFileName = false;
}

LogLog.Warn, not ErrorHandler.Error: the default OnlyOnceErrorHandler reports only its first
error, so the activation report would mute a later write failure; LogLog reaches the
ConfigureAndWatch message list just the same.

Detection, a private helper before ComputeCheckPeriod, which already formats _sDate1970. Its
conditions are named local functions at the end of the body, so the if reads as a sentence
(Hierarchy.cs:610 is the precedent for local functions; the rule goes into AGENTS.md):

private bool DoesDatePatternCollideWithBackupNames()
{
  if (DatePattern is not string datePattern || !IsBaseFileRolledByDate() || !HasNumberedBackups())
  {
    return false;
  }

  string sample = _sDate1970.ToString(datePattern, DateTimeFormatInfo.InvariantInfo);
  return IsDotAndUpToFourDigits(sample) && CanBackupNumberEqualDate(sample);

  bool IsBaseFileRolledByDate() => _rollDate && StaticLogFileName;

  bool HasNumberedBackups() => MaxSizeRollBackups != 0 && DoesRollBySizeOrAtStart();

  bool DoesRollBySizeOrAtStart() => _rollSize || !AppendToFile;

  static bool IsDotAndUpToFourDigits(string sample)
    => sample.Length is >= 2 and <= 5 && sample[0] == '.'
      && sample.Skip(1).All(c => c is >= '0' and <= '9');

  // 1970-01-01 00:00 writes every field at its narrowest, so dated names start at 10^(digits - 1)
  bool CanBackupNumberEqualDate(string sample)
    => CountDirection >= 0 || MaxSizeRollBackups < 0
      || MaxSizeRollBackups >= (int)Math.Pow(10, sample.Length - 2);
}

The DatePattern null test stays in the method as a pattern, because a local function cannot
narrow the property for the ToString call. Skip/All allocate two small objects once per
activation, accepted; a slice pattern would need System.Range, which net462 lacks.

Docs in the source: one <para> on the DatePattern remarks (reported when activated, then
reads StaticLogFileName as false, live file is the dated one, backups numbered after the date),
and one sentence in the class caution about changing StaticLogFileName, saying the appender
does it itself in this case.

2. InitializeFromOneFile: count only the backups of the current dated file

Require a dot after the dated name in the non-static scan, and name the guards:

if (!IsLogFile() || IsBaseFile())
{
  return;
}

// Only look for files in the current roll point
if (_rollDate && !StaticLogFileName && !IsBackupOfDatedFile(Path.GetFileName(curFileName)))
{
  return;
}

with IsLogFile, IsBaseFile and IsBackupOfDatedFile(string fileName) as local functions after
the catch. IsBackupOfDatedFile holds the dated/prefix/suffix locals, tests
fileName.StartsWith(prefix + ".") and EndsWith(suffix), logs the "different date period"
debug line, and returns fileName.Length > dated.Length so the dated file itself is excluded.
With PreserveLogFileNameExtension the prefix is log.1 and log.1.2.txt still matches.

3. Tests, new fixture src/log4net.Tests/Appender/RollingFileAppenderShortDatePatternTest.cs

A separate fixture: adding to RollingFileAppenderTest.cs trips CA1506 (class coupling). Shape
from RemoteSyslogAppenderTest.IdentityWithControlCharactersIsReported: List<LogLog> messages,
LogLog.ExecuteWithoutEmittingInternalMessages around using LogLog.LogReceivedAdapter _ = new(messages); plus the activation, assert on messages.ConvertAll(m => m.Message) with
Has.Exactly(n).Contains(...). [NonParallelizable] on every test that hooks the adapter. Folder
from using AutoTempFolder folder = new();, appender closed before the scope ends. Helpers:
_mockedNow 2026-10-04 09:00, CreateShortDatePatternAppender(file, datePattern, maxSizeRollBackups, countDirection, staticLogFileName = true, rollingStyle = Composite, appendToFile = true) with Integration.MockDateTime, ActivateCapturingInternalMessages,
CreateEvent. No string.Contains(string, StringComparison): it does not exist on net462,
which the Windows CI leg builds. No added line over 100 columns.

  1. ShortDatePatternUsesDatedFileName: Composite, .dd, static, 10 backups, MaxFileSize
    100, five events of 200 characters. One warning containing DatePattern [.dd]; log.txt is
    absent, log.txt.04 and log.txt.04.1 exist (full paths, Does.Contain).
  2. ShortDatePatternIsReportedOnlyWhenBackupsCollide, [TestCase]s shortest to longest source
    line, (pattern, static, maxBackups, countDirection, reported): .d 5 -1 true; .dd 5 1 true;
    .dd 5 -1 false; .dd 10 -1 true; .dd -1 -1 true; .dd static false 10 -1 false;
    .yyyyMMdd 10 1 false; .yyyy-MM-dd 10 -1 false. Not .yyyy: ActivateOptions throws
    ArgumentException for it before the check runs.
  3. DateRollingWithAppendIsNotReported: Date, .d, static, 10 backups, append true, no
    messages at all.
  4. ShortDatePatternRestartWithoutAppendKeepsExistingFile: Date, .d, static, 3 backups,
    AppendToFile false, an existing log.txt whose last-write time is the mocked clock (same
    period, so no date roll at start-up). One warning DatePattern [.d]; the folder holds exactly
    log.txt and log.txt.4 (Is.EquivalentTo), and log.txt still reads "previous run".
  5. DatedNameCountsOnlyItsOwnBackups: non-static .d, counting up, clock on the 1st, feed
    InitializeRollBackups("log.txt", ["log.txt.1", "log.txt.1.2", "log.txt.11", "log.txt.11.7"])
    and expect CurrentSizeRollBackups 2. No folder, no adapter.

Tests 1, 4, the four reported-true cases and test 5 fail at d0efa655; test 5 fails with a count
of 11 as long as the scan has no dot.

4. Changelog

src/changelog/3.5.1/335-short-date-pattern-collides-with-backup-names.xml, type="fixed",
<issue id="335" link="https://github.com/apache/logging-log4net/issues/335"/>. Prose, about
five source lines: what collided, that such an appender is now reported when activated and uses
the dated file name as with StaticLogFileName false, backups numbered after the date, an
existing static file left untouched including its numbered backups, and the opt-out: a
DatePattern with a separator, such as the default .yyyy-MM-dd. Close with
(reported by @Snotface, fixed by @FreeAndNil).

5. Manual

src/site/antora/modules/ROOT/pages/manual/configuration/appenders/rollingfileappender.adoc: a
[NOTE] block after the Composite example, one sentence per line, same content as the changelog,
same opt-out.

6. AGENTS.md

Under "Language usage": in a method whose conditions are hard to follow, give their parts names as
small local functions, even one-liners and even chained, so the if reads as a sentence; see
RollingFileAppender.DoesDatePatternCollideWithBackupNames; in review, do not suggest inlining
them; a simple guard stays inline.

7. Commits and pull request

Drafts in .git/plans/335/, subject with the issue number, one short intro, up to five bullets
without full sentences, no attribution line (as #334). The user stages and commits; nothing is
staged by the agent unless asked, and a partial stage is built from the committed file into the
index without touching the working tree.

  1. commit.txt: "use a dated file name when the date pattern names files like backups RollingFileAppender: short date pattern collides with size backup names #335":
    warn at activation and set StaticLogFileName to false; no report for a count-down limit
    below the smallest dated name; existing static file and its backups left untouched; startup
    scan of a dated name requires a dot after the name (.d). Appender, tests, changelog, manual.
  2. commit-2.txt: "name the parts of hard conditions with local functions": AGENTS.md rule and
    example; InitializeFromOneFile: IsLogFile, IsBaseFile, IsBackupOfDatedFile.
  3. pr.txt: title as commit 1, description "Fixes RollingFileAppender: short date pattern collides with size backup names #335." plus the same four bullets.

8. Review feedback on #336

gdziadkiewicz approves; Copilot leaves three threads: the 1970 sample does not establish the
smallest dated name (.Md), and five digits do not protect count-up (.yyyyM meets backup
20261), so the changelog and the manual should recommend a separator. No code change (see the
accepted points in the context). Three texts change: the warning, the changelog and the manual
say "a separator, such as the default .yyyy-MM-dd" and drop "or more digits" and .yyyyMMdd.
commit-3.txt: "recommend a separator in the date pattern, not more digits #336", one intro
line, one bullet. The user commits and pushes.

Thread replies are shown as drafts in the chat and posted only after the user says go, then the
threads are resolved (gh api .../pulls/336/comments/<id>/replies, resolveReviewThread):

  1. Sample thread: "The 1970 sample is the narrowest formatting of every numeric field except
    12-hour h, which is unusable without tt. .Md over-reports a count-down limit of exactly
    10, accepted for an ambiguous pattern. The five-digit threshold is where IsDateSuffix stops
    counting dated files as backups, so detection and counting agree. The advice now names a
    separator only, see ."
  2. Changelog thread and manual thread: "Changed to recommend a separator, with the default
    .yyyy-MM-dd as the example, see ."

Verification

S=/tmp/claude-1002/-apache-logging-log4net/bae2ed16-61bf-4ff2-908d-7be0718e13b1/scratchpad
dotnet build src/log4net.sln > $S/build.txt 2>&1          # read the whole file, 0 warnings
dotnet test src/log4net.Tests/log4net.Tests.csproj --no-build --filter "FullyQualifiedName~RollingFileAppender" > $S/test.txt 2>&1
  • Revert to fail without touching the index: copy the appender to $S, `git show d0efa65:

    , run --filter "FullyQualifiedNameShortDatePattern|FullyQualifiedNameDateRollingWithAppend", confirm the six expected failures, copy the file back. For the dot: sedthe dot out, runDatedNameCountsOnlyItsOwnBackups`, expect 11, restore.

  • Before a commit of a partial stage, export the index with git write-tree and git archive into
    $S/staged, build and test there.
  • awk 'length($0) > 100' over the added C# lines.
  • After the review round: reviewThreads on use a dated file name when the date pattern names files like backups #336 shows all three threads resolved and the
    approval in place.

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.

RollingFileAppender: short date pattern collides with size backup names

3 participants