src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (2)

88-91: Reason string can misrepresent actual policy; include both and use effective max count

Currently logs "MaxArchiveFiles" even when both MaxArchiveFiles and MaxArchiveDays are configured, and it ignores the effective maxArchiveFiles value (after startup adjustments). Prefer composing the reason from the active/effective settings.

-                    var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.MaxArchiveDays > 0) ? $"MaxArchiveDays={_fileTarget.MaxArchiveDays}" : $"MaxArchiveFiles={_fileTarget.MaxArchiveFiles}";
-                    if (initialFileOpen && _fileTarget.DeleteOldFileOnStartup)
-                        archiveCleanupReason = "DeleteOldFileOnStartup=true";
+                    var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.MaxArchiveDays > 0)
+                        ? $"MaxArchiveDays={_fileTarget.MaxArchiveDays}"
+                        : $"MaxArchiveFiles={maxArchiveFiles}"; // reflect effective count
+                    if (_fileTarget.MaxArchiveFiles >= 0 && _fileTarget.MaxArchiveDays > 0)
+                        archiveCleanupReason = $"MaxArchiveFiles={maxArchiveFiles}, MaxArchiveDays={_fileTarget.MaxArchiveDays}";
+                    if (initialFileOpen && _fileTarget.DeleteOldFileOnStartup)
+                        archiveCleanupReason = "DeleteOldFileOnStartup=true";

---

332-373: Echo reason on failures for better diagnostics

Include archiveCleanupReason in the exception paths as well.

-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file as directory not found: '{1}'", _fileTarget, filepath);
+                    InternalLogger.Debug(ex, "{0}: Failed to delete old file as directory not found (Reason: {1}): '{2}'", _fileTarget, archiveCleanupReason, filepath);
...
-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file as file not found: '{1}'", _fileTarget, filepath);
+                    InternalLogger.Debug(ex, "{0}: Failed to delete old file as file not found (Reason: {1}): '{2}'", _fileTarget, archiveCleanupReason, filepath);
...
-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file, maybe file is locked: '{1}'", _fileTarget, filepath);
+                    InternalLogger.Debug(ex, "{0}: Failed to delete old file, maybe file is locked (Reason: {1}): '{2}'", _fileTarget, archiveCleanupReason, filepath);
...
-                    InternalLogger.Warn(ex, "{0}: Failed to delete old archive file: '{1}'.", _fileTarget, filepath);
+                    InternalLogger.Warn(ex, "{0}: Failed to delete old archive file (Reason: {1}): '{2}'.", _fileTarget, archiveCleanupReason, filepath);

src/NLog/Targets/FileArchiveHandlers/ZeroFileArchiveHandler.cs (1)

81-83: Reason label here is about retention, but action is “reset/truncate active file”

Optional: make the reason reflect the action when not on startup, e.g., “Reset Active File”, for clearer logs.

-                    var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.ArchiveOldFileOnStartup) ? "ArchiveOldFileOnStartup=true" : $"MaxArchiveFiles={_fileTarget.MaxArchiveFiles}";
+                    var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.ArchiveOldFileOnStartup)
+                        ? "ArchiveOldFileOnStartup=true"
+                        : "Reset Active File";

src/NLog/Targets/FileArchiveHandlers/RollingArchiveFileHandler.cs (1)

121-121: LGTM; consider a shared constant for the reason token

To avoid typos across files, extract "DeleteOldFileOnStartup=true" into a private const or a static helper.

Example:

private const string ReasonDeleteOldOnStartup = "DeleteOldFileOnStartup=true";

src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs (1)

228-239: Unify terminology: BirthTime vs CreationTime; deduplicate the reason string

Comments reference CreationTime; the reason says “BirthTime”. Use one term consistently and avoid duplicating the literal.

-                if (!DeleteOldArchiveFile(newFilePath, "Reset File BirthTime"))
+                if (!DeleteOldArchiveFile(newFilePath, "Reset File CreationTime"))
...
-            DeleteOldArchiveFile(newFilePath, "Reset File BirthTime"); // Attempt to delete file to reset File-Creation-Time
+            DeleteOldArchiveFile(newFilePath, "Reset File CreationTime"); // Attempt to delete file to reset File-Creation-Time

Optionally:

// near class top
private const string ReasonResetCreationTime = "Reset File CreationTime";
// and use ReasonResetCreationTime in both places

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (4)

81-88: Guard against negative wildcard index when wildCardContainsSeqNo=true.
If fileWildcard lacks '', IndexOf returns -1; passing this into the seq-no scan can crash when wildCardContainsSeqNo=true. Recommend clamping usage to only when a '' is present, and clarifying the variable name.

Apply:

-                int fileWildcardStartIndex = fileWildcard.IndexOf('*');
-                int fileWildcardEndIndex = fileWildcard.Length - fileWildcardStartIndex;
+                int fileWildcardStartIndex = fileWildcard.IndexOf('*');
+                int fileWildcardEndIndex = (fileWildcardStartIndex >= 0) ? (fileWildcard.Length - fileWildcardStartIndex) : -1;
                 var maxArchiveFiles = _fileTarget.MaxArchiveFiles;
@@
-                foreach (var cleanupFileInfo in FileInfoDateTime.CleanupFiles(fileInfos, maxArchiveFiles, _fileTarget.MaxArchiveDays, fileWildcardStartIndex, fileWildcardEndIndex, excludeFileName, wildCardContainsSeqNo))
+                var containsSeqNo = wildCardContainsSeqNo && fileWildcardStartIndex >= 0;
+                foreach (var cleanupFileInfo in FileInfoDateTime.CleanupFiles(fileInfos, maxArchiveFiles, _fileTarget.MaxArchiveDays, fileWildcardStartIndex, fileWildcardEndIndex, excludeFileName, containsSeqNo))

Optional: rename fileWildcardEndIndex to fileWildcardSuffixLength for clarity.

Also applies to: 94-95

---

89-92: Reason string can mislead when both retention limits are set.
When both MaxArchiveFiles and MaxArchiveDays are configured, deletions may be age-driven but the reason logs “MaxArchiveFiles=…”. Prefer a neutral, explicit reason that includes both settings.

-                var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.MaxArchiveDays > 0) ? $"MaxArchiveDays={_fileTarget.MaxArchiveDays}" : $"MaxArchiveFiles={_fileTarget.MaxArchiveFiles}";
+                var archiveCleanupReason = $"Retention(MaxArchiveFiles={_fileTarget.MaxArchiveFiles}, MaxArchiveDays={_fileTarget.MaxArchiveDays})";
                 if (initialFileOpen && _fileTarget.DeleteOldFileOnStartup)
                     archiveCleanupReason = "DeleteOldFileOnStartup=true";

---

93-98: Return true only if deletion actually succeeded.
Currently sets oldFilesDeleted=true before attempting deletion, so transient failures still report success.

-                bool oldFilesDeleted = false;
+                bool oldFilesDeleted = false;
                 foreach (var cleanupFileInfo in FileInfoDateTime.CleanupFiles(fileInfos, maxArchiveFiles, _fileTarget.MaxArchiveDays, fileWildcardStartIndex, fileWildcardEndIndex, excludeFileName, wildCardContainsSeqNo))
                 {
-                    oldFilesDeleted = true;
-                    DeleteOldArchiveFile(cleanupFileInfo.FullName, archiveCleanupReason);
+                    oldFilesDeleted |= DeleteOldArchiveFile(cleanupFileInfo.FullName, archiveCleanupReason);
                 }
 
                 return oldFilesDeleted;

Also applies to: 100-100

---

332-373: Tweak log messages; include attempt count and reason on failures.
Improves diagnosability while preserving behavior.

-                    InternalLogger.Info("{0}: {1} deleting old archive file: '{2}'.", _fileTarget, archiveCleanupReason, filepath);
+                    InternalLogger.Info("{0}: Deleting old archive file due to {1}: '{2}'.", _fileTarget, archiveCleanupReason, filepath);
@@
-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file as directory not found: '{1}'", _fileTarget, filepath);
+                    InternalLogger.Debug(ex, "{0}: Failed to delete old file due to {1} (directory not found): '{2}'", _fileTarget, archiveCleanupReason, filepath);
@@
-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file as file not found: '{1}'", _fileTarget, filepath);
+                    InternalLogger.Debug(ex, "{0}: Failed to delete old file due to {1} (file not found): '{2}'", _fileTarget, archiveCleanupReason, filepath);
@@
-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file, maybe file is locked: '{1}'", _fileTarget, filepath);
+                    InternalLogger.Debug(ex, "{0}: Attempt {1}/3 failed to delete old file (maybe locked) due to {2}: '{3}'", _fileTarget, i, archiveCleanupReason, filepath);
@@
-                    InternalLogger.Warn(ex, "{0}: Failed to delete old archive file: '{1}'.", _fileTarget, filepath);
+                    InternalLogger.Warn(ex, "{0}: Failed to delete old archive file due to {1}: '{2}'.", _fileTarget, archiveCleanupReason, filepath);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (6)

74-77: Fix misleading debug message when directory is missing

The log implies “no files matching wildcard” but this branch is hit because the directory doesn’t exist.

-    InternalLogger.Debug("{0}: Archive Cleanup found no files matching wildcard {1} in directory: {2}", _fileTarget, fileWildcard, fileDirectory);
+    InternalLogger.Debug("{0}: Archive Cleanup skipped; directory does not exist. Wildcard={1}, Directory={2}", _fileTarget, fileWildcard, fileDirectory);

---

92-95: Log effective cleanup driver

Reason currently logs configured MaxArchiveFiles, not the effective value after adjustments. Consider logging the effective value to aid diagnostics.

-var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.MaxArchiveDays > 0) ? $"MaxArchiveDays={_fileTarget.MaxArchiveDays}" : $"MaxArchiveFiles={_fileTarget.MaxArchiveFiles}";
+var archiveCleanupReason = (_fileTarget.MaxArchiveFiles < 0 && _fileTarget.MaxArchiveDays > 0)
+    ? $"MaxArchiveDays={_fileTarget.MaxArchiveDays}"
+    : $"MaxArchiveFiles={maxArchiveFiles}";

---

341-345: Polish Info log phrasing for readability

Minor wording tweak reads better in logs.

-InternalLogger.Info("{0}: {1} deleting old archive file: '{2}'.", _fileTarget, archiveCleanupReason, filepath);
+InternalLogger.Info("{0}: Deleting old archive file ({1}): '{2}'.", _fileTarget, archiveCleanupReason, filepath);

---

356-363: Elevate final IO failure to Warn

On the last retry, consider logging Warn (with reason) to surface persistent lock issues.

-                catch (IOException ex)
+                catch (IOException ex)
                 {
-                    InternalLogger.Debug(ex, "{0}: Failed to delete old file, maybe file is locked: '{1}'", _fileTarget, filepath);
+                    var isFinalAttempt = i >= 3;
+                    if (isFinalAttempt)
+                        InternalLogger.Warn(ex, "{0}: Final attempt failed to delete old file ({1}): '{2}'", _fileTarget, archiveCleanupReason, filepath);
+                    else
+                        InternalLogger.Debug(ex, "{0}: Failed to delete old file, maybe file is locked: '{1}'", _fileTarget, filepath);
                     if (!File.Exists(filepath))
                         return true;
-                    if (i >= 3 && ex.MustBeRethrown(_fileTarget))
+                    if (isFinalAttempt && ex.MustBeRethrown(_fileTarget))
                         throw;
                 }

---

373-375: Optional: add small jitter to retries

Helps reduce thundering-herd on shared paths.

-Thread.Sleep(i * 10);
+Thread.Sleep(i * 10 + ThreadLocalRandom.Next(0, 10));

Additional code (once in the file):

private static class ThreadLocalRandom
{
    private static readonly ThreadLocal<Random> _rng = new ThreadLocal<Random>(() => new Random(unchecked(Environment.TickCount * 31 + Thread.CurrentThread.ManagedThreadId)));
    public static int Next(int min, int max) => _rng.Value!.Next(min, max);
}

---

347-355: Standardize DeleteOldArchiveFile return values
BaseFileArchiveHandler.DeleteOldArchiveFile returns true on DirectoryNotFound/FileNotFound, whereas ConcurrentFileTarget.DeleteOldArchiveFile returns false in those cases. Align both to use a consistent “true means file no longer exists after attempt” semantic across the repository.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.