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

CLAUDE.md

@@ -26,6 +26,8 @@ almost always be doing.
   Omit the type wherever the target is known, including `return new(…);` and `=> new(…);`, where
   the enclosing member's return type supplies it. It cannot be omitted when the target type is an
   interface or abstract class, as in `Func<ISmtpTransport> f = () => new MailKitSmtpTransport();`.
+- `x?.Method() ?? false` rather than `x is not null && x.Method()`, and merge nested guards
+  into one condition.
 - Expression-bodied members whenever the body fits on one line, including constructors
   (`resharper_constructor_or_destructor_body = expression_body`).
 - Braces on `if`/`else` bodies even for a single statement.
@@ -35,8 +37,11 @@ almost always be doing.
 - **Wrap long string literals with a multi-line raw string (`"""`), never with `+`
   concatenation.** This includes attribute arguments; see the `[Obsolete(...)]` message on
   `log4net.Appender.SmtpAppender`. Raw strings have no line-continuation, so each source line
-  break really is a `\n` in the value, but that is fine here: compiler diagnostics render those
-  newlines as spaces, so a wrapped message still reads as one sentence. Raw strings are constant
+  break really is a `\n` in the value. For a compiler diagnostic that is free, since the tooling
+  renders those newlines as spaces. For a message that is **logged**, through `LogLog` or an
+  `ErrorHandler`, the breaks reach the output: `LogLog` prefixes once per message, so the
+  continuation lines carry no `log4net:ERROR`. That is accepted, and the rule holds there too;
+  do not reintroduce `+` concatenation to keep such a message on one line. Raw strings are constant
   expressions, so they are legal in attributes, and the feature is purely syntactic, so it works on
   `net462`/`netstandard2.0` too.
 - Private fields are `_camelCase`. Private fields and helper methods are commonly placed
@@ -149,6 +154,13 @@ almost always be doing.
   the assertion is about control characters, use
   `Contains.Substring(x).Using(StringComparison.Ordinal)`, negated with the `!` operator that
   `Constraint` defines, or assert the whole value with `Is.EqualTo`, which is ordinal.
+- **Order `[TestCase]` attributes shortest to longest by source line**, not by argument length.
+- **If no black-box test can reach a defect, extract the sequence into a small private helper
+  and drive that by reflection.** Do not delete the test and call the defect untestable. The
+  extraction is usually an improvement anyway: `FileAppender.RunWithBestEffortLock` replaced two
+  copies of an acquire/release pair, one of which released a lock it had failed to take.
+- **A test that passes before the fix is worthless.** Revert the production change and watch
+  it fail; if it does not, the test is wrong or the defect is not where you think it is.
 - **Give a `[TestCase]` an explicit `TestName` when an argument holds a control character.**
   Otherwise the whole fixture can become invisible to `dotnet test --filter`, silently: it is
   listed by `--list-tests` and runs in a full pass, but every filter reports "No test matches".
@@ -206,11 +218,15 @@ Every user-visible change gets an entry in `src/changelog/<unreleased version>/`
   `missing attribute: link` otherwise, which is only caught by the Maven site build.
 - Put anything that has no issue number, such as an external finding identifier, in the description
   text rather than inventing an `<issue>` for it.
+- **The description is whitespace-collapsed before the AsciiDoc transform, so block syntax does
+  not survive.** No bullets, no code blocks: `*` ends up mid-sentence as a literal asterisk.
+  Write prose. Bullets are fine in commit messages.
 - Close the description with an attribution in parentheses, crediting both sides: who raised it and
   who did the work, as in `(reported by @viktorgobbi, fixed by @FreeAndNil)`. `implemented by` reads
-  better than `fixed by` for an `added` or `changed` entry, and once a pull request exists the house
-  form appends it: `fixed by @FreeAndNil in https://github.com/apache/logging-log4net/pull/246[#246]`.
-  Take the fixer from the active committers in `STATUS.txt`, whose Apache ids are the GitHub handles
+  better than `fixed by` for an `added` or `changed` entry. **Do not append the pull request link to
+  the attribution**: the `<issue>` element above the description already carries it, and entries
+  that repeat it are the older form. Take the fixer from the active committers in `STATUS.txt`,
+  whose Apache ids are the GitHub handles
   (`freeandnil`, `gdziadkiewicz`, `davydm`), and identify which one from the session's `git config
   user.email`. Ask rather than guess if that does not match a listed committer.
 - `src/changelog/3.3.2/298-fix-interprocesslock-mutex-leak.xml` shows the shape for a change that

src/Directory.Build.props

@@ -34,6 +34,7 @@
     <NUnitAnalyzersPackageVersion>4.3.0</NUnitAnalyzersPackageVersion>
     <NUnitPackageVersion>4.2.2</NUnitPackageVersion>
     <NUnit3TestAdapterPackageVersion>4.6.0</NUnit3TestAdapterPackageVersion>
+    <PeanutButterUtilsPackageVersion>3.0.437</PeanutButterUtilsPackageVersion>
     <QuackersTestLoggerPackageVersion>1.0.25</QuackersTestLoggerPackageVersion>
   </PropertyGroup>
   <ItemGroup>

src/changelog/3.5.0/319-lock-level-underflow.xml

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+  <description format="asciidoc">Stop a lock the appender could not take from
+  disabling it. Writing the footer, closing the writer and opening the file released the file lock
+  even when taking it had failed, and every later attempt then failed too, so the footer and
+  anything still buffered were dropped without a word. Only a lock that was taken is released
+  now (audit da18b6fd-f032, fixed by @FreeAndNil)</description>
+</entry>

src/changelog/3.5.0/319-mutex-name-length.xml

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+  <description format="asciidoc">Keep logging to a deep path on Unix. The locks
+  that serialise writing and rolling are named after the log file, and Unix rejects a name longer
+  than 255 UTF-8 bytes, which a path of 104 characters can already exceed, so the appender failed to
+  start and nothing was written at all. Such a name is hashed now; shorter ones, and every name on
+  Windows, are unchanged (audit da18b6fd-f010, da18b6fd-f031, fixed by @FreeAndNil)</description>
+</entry>

src/changelog/3.5.0/319-mutex-resolved-path.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+  <description format="asciidoc">Let two processes that spell the log path
+  differently share one file lock. The lock was named after the configured path before that path was
+  resolved, so a relative and an absolute spelling of one file took two different locks and excluded
+  nothing. The resolved path names it now. Only that spelling is covered: symbolic links, hard
+  links, 8.3 short names, letter case and UNC versus mapped-drive spellings still count as different
+  files, and on Windows a service and a desktop application never share the lock at
+  all (audit da18b6fd-f031, fixed by @FreeAndNil)</description>
+</entry>

src/changelog/3.5.0/319-rollover-keeps-events.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+  <description format="asciidoc">Keep the log file when a rollover cannot rename it. The failed
+  rename was reported and the file then reopened without appending, which destroyed everything it
+  held; a reader holding the file without `FILE_SHARE_DELETE`, such as a backup or antivirus agent,
+  is enough to cause it. The file is kept and appended to now, at startup as well, and the rename is
+  retried as the file grows, so it can exceed `MaxFileSize` while the rename keeps failing (audit
+  da18b6fd-f036, fixed by @FreeAndNil)</description>
+</entry>

src/log4net.Ext.Mail.Tests/Appender/FakeSmtpTransport.cs

@@ -71,6 +71,9 @@ internal sealed class FakeSmtpTransport : ISmtpTransport
   /// <summary>When set, every call that takes a token waits this long, honouring it.</summary>
   internal int DelayMillisPerCall { get; set; }
 
+  /// <summary>The calls that found the token cancelled, in order.</summary>
+  internal List<string> CancelledCalls { get; } = [];
+
   public int Timeout { get; set; }
 
   public bool IsConnected { get; private set; }
@@ -81,7 +84,7 @@ public void Connect(string host, int port, SecureSocketOptions secureSocketOptio
     CancellationToken cancellationToken)
   {
     Calls.Add(nameof(Connect));
-    Delay(cancellationToken);
+    Delay(nameof(Connect), cancellationToken);
     ConnectedHost = host;
     ConnectedPort = port;
     SecureSocketOptions = secureSocketOptions;
@@ -91,23 +94,23 @@ public void Connect(string host, int port, SecureSocketOptions secureSocketOptio
   public void Authenticate(ICredentials credentials, CancellationToken cancellationToken)
   {
     Calls.Add(nameof(Authenticate));
-    Delay(cancellationToken);
+    Delay(nameof(Authenticate), cancellationToken);
     Credentials = credentials;
     IsAuthenticated = true;
   }
 
   public void Authenticate(SaslMechanism mechanism, CancellationToken cancellationToken)
   {
     Calls.Add(nameof(Authenticate));
-    Delay(cancellationToken);
+    Delay(nameof(Authenticate), cancellationToken);
     SaslMechanism = mechanism;
     IsAuthenticated = true;
   }
 
   public void Send(MimeMessage message, CancellationToken cancellationToken)
   {
     Calls.Add(nameof(Send));
-    Delay(cancellationToken);
+    Delay(nameof(Send), cancellationToken);
     if (SendException is Exception exception)
     {
       throw exception;
@@ -128,13 +131,18 @@ public void Dispose()
     IsDisposed = true;
   }
 
-  private void Delay(CancellationToken cancellationToken)
+  private void Delay(string call, CancellationToken cancellationToken)
   {
     if (DelayMillisPerCall > 0)
     {
       cancellationToken.WaitHandle.WaitOne(DelayMillisPerCall);
     }
 
+    if (cancellationToken.IsCancellationRequested)
+    {
+      CancelledCalls.Add(call);
+    }
+
     cancellationToken.ThrowIfCancellationRequested();
   }
 }

src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs

@@ -21,7 +21,6 @@
 using System.Collections.Generic;
 using System.Net;
 using System.Net.Mail;
-using System.Diagnostics;
 using System.Text;
 using log4net.Core;
 using log4net.Ext.Mail.Appender;
@@ -655,21 +654,19 @@ public void TheTimeoutReachesTheTransport()
   [Test]
   public void TheDeadlineCoversTheWholeSendAndNotOneOperation()
   {
+    // Under the timeout per call, over it in sum.
     const int sendTimeoutMillis = 300;
     const int delayMillisPerCall = 200;
-    const int generousBoundMillis = 5_000;
 
     SmtpAppender appender = CreateAppender();
     appender.SendTimeoutMillis = sendTimeoutMillis;
     _transport.DelayMillisPerCall = delayMillisPerCall;
 
-    Stopwatch stopwatch = Stopwatch.StartNew();
     Append(appender);
-    stopwatch.Stop();
 
     Assert.That(_transport.SentMails, Is.Empty);
     Assert.That(_errorHandler.Message, Does.Contain("Failed to send a logging event."));
-    Assert.That(stopwatch.ElapsedMilliseconds, Is.LessThan(generousBoundMillis));
+    Assert.That(_transport.CancelledCalls, Is.Not.Empty, "the deadline never stopped the send");
   }
 
   /// <summary>Bounded, so an unreachable server cannot grow the queue without limit.</summary>