file locking: keep the log on a failed roll, fix the mutex name by FreeAndNil · Pull Request #319 · apache/logging-log4net

FreeAndNil · 2026-09-11T22:56:39Z

Four findings from an audit, all in file locking. None is a vulnerability; audit da18b6fd-*
cites them for provenance.

f036: a failed rollover rename reopened the log without appending and destroyed it. It appends
now, and only that rename is retried, once per MaxFileSize of growth, so the file can exceed
MaxFileSize while the rename keeps failing.
f032: the footer, close and open paths released a lock they had failed to take, poisoning
every later acquisition.
f031: the mutex was named before the path was resolved, so a relative and an absolute spelling
of one file excluded nothing.
f010: a name over 255 characters throws on Unix and is hashed now. Windows has no limit and
keeps its names.

The Local\ prefix, per-user scoping and ACL the scan proposed are deliberately absent: measured on
Windows, cross-session exclusion never existed, and a bare Global\ prefix throws for whichever
process starts second.

Also here: a pre-Vista branch in EventLogAppender that could not be false, and two commits from the review.
The new fixtures keep no state between tests, and three older ones stopped keeping temp files in the build output or leaking them.

- A failed rename was reported, then the file reopened without appending, which destroyed it. A backup agent holding a read handle is enough. It appends now, and only that rename is retried, once per MaxFileSize of growth, so the backups are never rotated twice and a retry that succeeds keeps the generation it recovers. - The footer, close and open paths released the file lock even when acquiring it had failed. Only what was taken is released now. audit da18b6f-f036, da18b6f-f032

- InterProcessLock named its mutex before the path was resolved, so a relative and an absolute spelling of one file took two mutexes and excluded nothing. - A name over 255 characters throws on Unix, out of ActivateOptions, so a deep log path took the appender down. Those are hashed now. Windows has no limit, measured, so its names are left alone and keep excluding older versions. - Both mutexes take their name from one helper, which carries why there is no ACL, no Global\ prefix and no user component. audit da18b6f-f031, da18b6f-f010

Windows 7 SP1 is the floor for the net462 build this file compiles into, so the version test could not fail and the 32766 constant behind it was dead. The surviving constant keeps its measured value and loses the superseded lore.

- Release the file lock with _stream?.ReleaseLock() wherever the acquire was already null-conditional. - One CountingWriter property instead of five QuietWriter casts, through EnsureIs<> so a wrong writer names itself. The two sites after a reopen keep their null check: a refused lock leaves no writer there. - The new fixtures keep no state between tests: AutoTempFolder from PeanutButter.Utils and a per-test error handler. Its namespace collides with our test helper Utils, which is internal TestUtils now.

…cking

- SmtpPickupDirAppenderTest kept its pickup directory in a fixture field, shared by every test, and created it inside the build output. - FileAppenderTest leaked two files per run: Path.GetTempFileName creates them and nothing deleted them. - PatternStringTest tracked its config file by hand to delete it in a finally. All three use AutoTempFolder now, which disposes what it made.

Copilot

🟡 Changes recommended

Date-only and startup rollover failures can still bypass recovery or truncate retained log data.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Improves file-locking reliability and rollover recovery following four audit findings.

Changes:

Preserves logs after failed rollovers and corrects lock-release handling.
Derives mutex names from resolved paths and hashes oversized Unix names.
Adds regression tests, temporary-directory cleanup, changelogs, and contributor guidance.

File summaries

File	Description
`CLAUDE.md`	Extends coding and testing guidance.
`src/Directory.Build.props`	Defines the PeanutButter.Utils version.
`src/log4net/Appender/EventLogAppender.cs`	Removes obsolete event-log size branching.
`src/log4net/Appender/FileAppender.cs`	Fixes lock accounting and mutex naming.
`src/log4net/Appender/RollingFileAppender.cs`	Adds failed-roll preservation and retries.
`src/log4net/Util/SystemInfo.cs`	Adds Windows runtime detection.
`src/log4net.Tests/Appender/FileAppenderMutexNameTest.cs`	Tests mutex path and length handling.
`src/log4net.Tests/Appender/FileAppenderTest.cs`	Uses isolated temporary files.
`src/log4net.Tests/Appender/LockingStreamTest.cs`	Tests lock recursion and failed acquisition.
`src/log4net.Tests/Appender/RollingFileAppenderRollFailureTest.cs`	Tests rollover failure recovery.
`src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs`	Isolates pickup directories per test.
`src/log4net.Tests/Context/LogicalThreadContextTest.cs`	Updates renamed test utilities.
`src/log4net.Tests/Context/ThreadContextTest.cs`	Updates renamed test utilities.
`src/log4net.Tests/Layout/PatternLayoutTest.cs`	Updates renamed test utilities.
`src/log4net.Tests/TestUtils.cs`	Renames and restricts the test utility class.
`src/log4net.Tests/Util/PatternStringTest.cs`	Uses an automatically cleaned temporary folder.
`src/log4net.Tests/log4net.Tests.csproj`	Adds PeanutButter.Utils.
`src/changelog/3.5.0/319-lock-level-underflow.xml`	Documents lock-counter recovery.
`src/changelog/3.5.0/319-mutex-name-length.xml`	Documents long mutex-name handling.
`src/changelog/3.5.0/319-mutex-resolved-path.xml`	Documents resolved-path mutex naming.
`src/changelog/3.5.0/319-rollover-keeps-events.xml`	Documents failed-roll preservation.

Review details

Files reviewed: 21/21 changed files
Comments generated: 10
Review effort level: Balanced

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

gdziadkiewicz · 2026-09-17T22:32:16Z

+      if (!TryRollFile(baseFileName, CombinePath(baseFileName, ".1")))
+      {
+        CurrentSizeRollBackups--;
+        RecordFailedBaseRename(baseFileName, CombinePath(baseFileName, ".1"), wasBackupCountReverted: true);


I will try to repro/prepare test for that

gdziadkiewicz · 2026-09-17T22:27:26Z

+      .Replace(":", "_")
+      .Replace("/", "_") + suffix;
+
+    if (maxLength is null || name.Length <= maxLength)


IMO this is reasonable and we should address it

Thanks. I will address that.

gdziadkiewicz · 2026-09-17T22:41:45Z

+      if (_rollSize && (File is not null) && CountingWriter.Count >= MaxFileSize)
      {
-        RollOverSize();
+        if (_pendingRename is null)
+        {
+          RollOverSize();
+        }
+        else if (CountingWriter.Count >= _pendingRename.RetryAtCount)


I will try to repro/prepare test for that

+  stream throws. Nothing was lost by this, because the appender reopens the file on the next
+  event (audit da18b6fd-f032, fixed by @FreeAndNil)</description>


+  that used to throw and exclusion against an older version is nowhere affected (audit
+  da18b6fd-f010, da18b6fd-f031, fixed by @FreeAndNil)</description>


+  per-session, so a service and an interactive process have never coordinated through it (audit
+  da18b6fd-f031, fixed by @FreeAndNil)</description>


+  therefore grows past `MaxFileSize` for as long as the rename keeps failing (audit da18b6fd-f036,
+  fixed by @FreeAndNil)</description>


+    public override void ActivateOptions() => FileAtActivation = CurrentAppender?.File;
+
+    public override Stream? AcquireLock() => Stream.Null;
+
+    public override void ReleaseLock()
+    { }
+
+    public override void OpenFile(string filename, bool append, Encoding encoding)
+    { }
+
+    public override void CloseFile()
+    { }
+
+    public override void OnClose()
+    { }


+    public override Stream? AcquireLock() => CanAcquire ? Stream.Null : null;
+
+    public override void ReleaseLock() => ReleaseCount++;
+
+    public override void OpenFile(string filename, bool append, Encoding encoding)
+    { }
+
+    public override void CloseFile()
+    { }
+
+    public override void ActivateOptions()
+    { }
+
+    public override void OnClose()
+    { }


+      LogLog.Error(_declaringType,
+        $"Rolling {_pendingRename.From} failed, so it is kept and appended to. Only that rename is "
+        + "retried, once per MaxFileSize of growth, so the backups are left alone.");


fluffynuts · 2026-09-18T19:51:49Z

    <NUnitAnalyzersPackageVersion>4.3.0</NUnitAnalyzersPackageVersion>
    <NUnitPackageVersion>4.2.2</NUnitPackageVersion>
    <NUnit3TestAdapterPackageVersion>4.6.0</NUnit3TestAdapterPackageVersion>
+    <PeanutButterUtilsPackageVersion>2.0.63</PeanutButterUtilsPackageVersion>


minor: is quite an old version; it's doing what's needed in this repo, but the release criteria for PB is new features and bugfixes; things generally aren't deprecated (much) and never removed without a major bump. No need to update, just noting.

Thanks for the hint. I will update.

FreeAndNil changed the title ~~keep the log when a roll cannot rename it #319~~ keep the log when a roll cannot rename it Sep 11, 2026

FreeAndNil added 2 commits September 14, 2026 22:10

drop the pre-Vista event log size branch

2f3d444

Windows 7 SP1 is the floor for the net462 build this file compiles into, so the version test could not fail and the 32766 constant behind it was dead. The surviving constant keeps its measured value and loses the superseded lore.

FreeAndNil changed the title ~~keep the log when a roll cannot rename it~~ keep the log when a roll cannot rename it, and name the file lock after the resolved path Sep 14, 2026

FreeAndNil changed the title ~~keep the log when a roll cannot rename it, and name the file lock after the resolved path~~ file locking: keep the log on a failed roll, fix the mutex name Sep 14, 2026

FreeAndNil marked this pull request as ready for review September 14, 2026 20:41

FreeAndNil requested review from fluffynuts and gdziadkiewicz September 14, 2026 20:41

FreeAndNil added this to the 3.5.0 milestone Sep 14, 2026

fluffynuts reviewed Sep 15, 2026

View reviewed changes

Comment thread src/log4net.Tests/Appender/FileAppenderMutexNameTest.cs Outdated

Comment thread src/log4net/Appender/FileAppender.cs

fluffynuts requested changes Sep 15, 2026

View reviewed changes

Comment thread src/log4net/Appender/FileAppender.cs Outdated

Comment thread src/log4net/Appender/FileAppender.cs Outdated

Comment thread src/log4net/Appender/FileAppender.cs Outdated

Comment thread src/log4net/Appender/RollingFileAppender.cs Outdated

FreeAndNil added 3 commits September 17, 2026 22:19

Merge remote-tracking branch 'origin/master' into Feature/319-file-lo…

6dc58e5

…cking

FreeAndNil requested a review from fluffynuts September 17, 2026 20:33

gdziadkiewicz requested a balanced review from Copilot September 17, 2026 21:44

Copilot started reviewing on behalf of gdziadkiewicz September 17, 2026 21:45 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

fluffynuts reviewed Sep 18, 2026

View reviewed changes

fluffynuts approved these changes Sep 18, 2026

View reviewed changes

Navigation Menu

Uh oh!

file locking: keep the log on a failed roll, fix the mutex name - #319

file locking: keep the log on a failed roll, fix the mutex name#319
FreeAndNil wants to merge 6 commits into
masterfrom
Feature/319-file-locking

FreeAndNil commented Sep 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

gdziadkiewicz Sep 17, 2026

Uh oh!

gdziadkiewicz Sep 17, 2026

Uh oh!

FreeAndNil Sep 18, 2026

Uh oh!

gdziadkiewicz Sep 17, 2026

Uh oh!

fluffynuts Sep 18, 2026

Uh oh!

FreeAndNil Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		stream throws. Nothing was lost by this, because the appender reopens the file on the next
		event (audit da18b6fd-f032, fixed by @FreeAndNil)</description>

		that used to throw and exclusion against an older version is nowhere affected (audit
		da18b6fd-f010, da18b6fd-f031, fixed by @FreeAndNil)</description>

		per-session, so a service and an interactive process have never coordinated through it (audit
		da18b6fd-f031, fixed by @FreeAndNil)</description>

		therefore grows past `MaxFileSize` for as long as the rename keeps failing (audit da18b6fd-f036,
		fixed by @FreeAndNil)</description>

Uh oh!

Conversation

FreeAndNil commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

gdziadkiewicz Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

gdziadkiewicz Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

FreeAndNil Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

gdziadkiewicz Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

fluffynuts Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

FreeAndNil Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FreeAndNil commented Sep 11, 2026 •

edited

Loading