Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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

198-210: Critical: maxArchiveFiles == 0 logic still contradicts NLog specification.

The condition at line 205 (maxArchiveFiles == 0) yields the file for deletion, but according to NLog specification, MaxArchiveFiles=0 means "infinite archives" (do not delete based on count). The current logic is inverted:

1. Incorrect semantic: When users set MaxArchiveFiles=0, they expect infinite archives, but the code deletes the file
2. Age constraint bypass: The code yields without checking MaxArchiveDays (line 207), so even files newer than the configured age threshold are deleted

This issue was identified in the previous review and remains unaddressed.

Apply this fix to respect both the specification and age constraints:

-                else if (maxArchiveFiles == 0)
+                else if (maxArchiveFiles == 0 && _fileTarget.MaxArchiveDays > 0)
                 {
-                    yield return fileInfos[0];
-                    yield break;
+                    // Single file: check age constraint before yielding
+                    var fileCreatedTimeUtc = FileInfoHelper.LookupValidFileCreationTimeUtc(fileInfos[0]);
+                    var fileInfoDate = new FileInfoDateTime(fileInfos[0], fileCreatedTimeUtc);
+                    if (ShouldDeleteFile(fileInfoDate, 1, maxArchiveFiles, _fileTarget.MaxArchiveDays))
+                    {
+                        yield return fileInfos[0];
+                    }
+                    yield break;
                 }

Note: This fix handles the maxArchiveFiles == 0 case when MaxArchiveDays > 0. You should also verify the overall logic: if maxArchiveFiles == 0 is intended as a sentinel for "delete all" (via DeleteOldFileOnStartup), the relationship between user-configured MaxArchiveFiles values and the resolved maxArchiveFiles needs clearer separation to avoid conflating "infinite archives" with "delete all".

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

60-61: Refactor for clarity: variable name and logic flow are confusing.

The variable deletedOldFiles is first assigned File.Exists(newFilePath) (line 60), then updated with an AND operation (line 61). This is logically correct but confusing because:

1. The name "deletedOldFiles" suggests it tracks whether files were deleted, but it initially tracks whether the target file exists
2. The AND operation DeleteOldFilesBeforeArchive(...) && deletedOldFiles is backwards for readability

Consider this clearer alternative:

-                bool deletedOldFiles = File.Exists(newFilePath);
-                deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen, parseArchiveSequenceNo) && deletedOldFiles;
+                bool targetFileExists = File.Exists(newFilePath);
+                bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen, parseArchiveSequenceNo);
-                if (deletedOldFiles && initialFileOpen)
+                if (deletedOldFiles && targetFileExists && initialFileOpen)
                 {
                     FixWindowsFileSystemTunneling(newFilePath);
                 }

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

114-122: Simplify ResolveMaxArchiveFiles for readability.

The conditional logic at lines 117-118 is difficult to parse:

1. Line 117: The condition (!_fileTarget.ArchiveOldFileOnStartup || _fileTarget.DeleteOldFileOnStartup) requires careful boolean logic analysis
2. Line 118: The ternary _fileTarget.DeleteOldFileOnStartup ? 0 : maxArchiveFiles may assign maxArchiveFiles to itself (no-op) when DeleteOldFileOnStartup is false

Consider restructuring for clarity:

 private int ResolveMaxArchiveFiles(bool initialFileOpen)
 {
     var maxArchiveFiles = _fileTarget.MaxArchiveFiles;
-    if (initialFileOpen && (!_fileTarget.ArchiveOldFileOnStartup || _fileTarget.DeleteOldFileOnStartup))
-        maxArchiveFiles = _fileTarget.DeleteOldFileOnStartup ? 0 : maxArchiveFiles;
+    if (initialFileOpen && _fileTarget.DeleteOldFileOnStartup)
+    {
+        maxArchiveFiles = 0; // Delete all files on startup
+    }
+    else if (initialFileOpen && !_fileTarget.ArchiveOldFileOnStartup)
+    {
+        // Keep maxArchiveFiles as-is, don't decrement
+    }
     else if (maxArchiveFiles > 0)
+    {
         maxArchiveFiles -= 1;
+    }
     return maxArchiveFiles;
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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

60-60: LGTM - Correctly handles edge case for weekday-format archive cleanup.

The new initialFileExists check and subsequent FixWindowsFileSystemTunneling call correctly address scenarios where files with weekday patterns (e.g., "app_Mon.log") might be both the current file and an old archive requiring cleanup. Since FixWindowsFileSystemTunneling internally checks !File.Exists(newFilePath) before acting, this is safe even when the file wasn't actually deleted.

Optional micro-optimization: If profiling shows this matters, you could check File.Exists(newFilePath) again before calling FixWindowsFileSystemTunneling to avoid the method call overhead when the file still exists after cleanup:

 if (initialFileExists && deletedOldFiles)
 {
-    FixWindowsFileSystemTunneling(newFilePath);
+    if (!File.Exists(newFilePath))
+    {
+        FixWindowsFileSystemTunneling(newFilePath);
+    }
 }

However, since FixWindowsFileSystemTunneling already performs this check and the method is lightweight, the current implementation prioritizes code simplicity.

Also applies to: 72-75

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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

198-210: Single-file fast‑path still bypasses MaxArchiveDays; guard or drop the special‑case.

With maxArchiveFiles == 0 the file is yielded unconditionally, ignoring MaxArchiveDays. If the intent is that age should still gate deletion, either guard by days or let the general path handle it. This mirrors prior feedback on this block.

Option A (keep fast‑path but respect days):

-else if (maxArchiveFiles == 0)
+else if (maxArchiveFiles == 0)
 {
-    yield return fileInfos[0];
-    yield break;
+    if (maxArchiveDays <= 0)
+    {
+        yield return fileInfos[0];
+    }
+    else
+    {
+        var createdUtc = FileInfoHelper.LookupValidFileCreationTimeUtc(fileInfos[0]);
+        var candidate = new FileInfoDateTime(fileInfos[0], createdUtc);
+        // Disable count-based deletion here so age decides
+        if (ShouldDeleteFile(candidate, 1, -1, maxArchiveDays))
+            yield return fileInfos[0];
+    }
+    yield break;
 }

Option B (simpler, rely on common logic and avoid surprises):

-else if (maxArchiveFiles == 0)
-{
-    yield return fileInfos[0];
-    yield break;
-}
+// No single-file shortcut; let common path apply count/age rules uniformly

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

84-87: Add missing fast-path for “unlimited archives and no age policy”.

If MaxArchiveFiles < 0 and MaxArchiveDays ≤ 0 there’s nothing to delete; skip enumeration to reduce I/O and log noise.

 int maxArchiveFiles = ResolveMaxArchiveFiles(initialFileOpen);
-if (maxArchiveFiles >= fileInfos.Length && _fileTarget.MaxArchiveDays <= 0)
+// Nothing to delete when count already within limit and no age policy
+if (maxArchiveFiles >= fileInfos.Length && _fileTarget.MaxArchiveDays <= 0)
     return false;
+// Also nothing to delete when unlimited archives and no age policy
+if (maxArchiveFiles < 0 && _fileTarget.MaxArchiveDays <= 0)
+    return false;

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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

72-75: Avoid duplicate FixWindowsFileSystemTunneling call paths.

You now call FixWindowsFileSystemTunneling both inside the early-return branch (Lines 65–68) and here. Consider consolidating into a single post-cleanup condition to reduce duplication, e.g., invoke once when deletedOldFiles is true and initialFileOpen is true; the method itself safely no-ops if the file still exists.

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

114-122: Clarify ResolveMaxArchiveFiles semantics.

Helper improves readability. Add a brief comment summarizing the adjustments for initialFileOpen, ArchiveOldFileOnStartup, DeleteOldFileOnStartup, and the “-1/0/>0” cases to prevent regressions and future confusion.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro