src/NLog/Targets/FileAppenders/DiscardAllFileAppender.cs (3)

40-48: Add XML documentation for better maintainability.

The properties are well-implemented and align with the "discard all" behavior. Consider adding XML documentation to explain the rationale behind returning specific values (e.g., why FileSize is always 0 and NextArchiveTime is DateTime.MaxValue).

+    /// <summary>
+    /// Represents a file appender that discards all write operations.
+    /// </summary>
     internal sealed class DiscardAllFileAppender : IFileAppender
     {
+        /// <summary>
+        /// Gets the file path (used only for logging purposes as no actual file operations occur).
+        /// </summary>
         public string FilePath { get; }

+        /// <summary>
+        /// Gets the time when this appender was created.
+        /// </summary>
         public DateTime OpenStreamTime { get; }

+        /// <summary>
+        /// Gets the last modified time (always returns creation time as no actual modifications occur).
+        /// </summary>
         public DateTime FileLastModified => OpenStreamTime;

+        /// <summary>
+        /// Gets the next archive time (always returns MaxValue to prevent archiving).
+        /// </summary>
         public DateTime NextArchiveTime => DateTime.MaxValue;

+        /// <summary>
+        /// Gets the file size (always returns 0 as no actual writes occur).
+        /// </summary>
         public long FileSize => 0;

---

56-73: Add XML documentation for interface methods.

The implementation correctly discards all operations as intended. Consider adding XML documentation to explain the no-op behavior and its implications.

+        /// <summary>
+        /// Discards the write operation without performing any I/O.
+        /// </summary>
         public void Write(byte[] buffer, int offset, int count)
         {
             // SONAR: Nothing to write
         }

+        /// <summary>
+        /// No-op as there is no underlying stream to flush.
+        /// </summary>
         public void Flush()
         {
             // SONAR: Nothing to flush
         }

+        /// <summary>
+        /// No-op as there are no resources to dispose.
+        /// </summary>
         public void Dispose()
         {
             // SONAR: Nothing to close
         }

+        /// <summary>
+        /// Always returns true as this appender doesn't interact with the file system.
+        /// </summary>
         public bool VerifyFileExists() => true;

+        /// <summary>
+        /// Returns the file path for logging purposes.
+        /// </summary>
         public override string ToString() => FilePath;

---

38-74: Well-implemented Null Object pattern.

This implementation serves as an excellent example of the Null Object pattern, providing a no-op implementation of IFileAppender that can be used to disable file logging without changing the logging configuration. This approach is more elegant than using conditional checks throughout the codebase.

src/NLog/Targets/FileAppenders/IFileAppender.cs (4)

43-48: Add XML documentation comments to properties for clarity.

The properties FilePath, FileSize, OpenStreamTime, FileLastModified, and NextArchiveTime lack XML documentation comments. Adding summaries to these properties will improve code readability and help other developers understand their purpose.

---

46-48: Consider using DateTimeOffset instead of DateTime for time properties.

Using DateTimeOffset for OpenStreamTime, FileLastModified, and NextArchiveTime can provide more accurate time representation, especially when dealing with multiple time zones. This change enhances the reliability of time-related data.

---

58-60: Add XML documentation comments to Flush and VerifyFileExists methods.

Providing XML documentation for the Flush and VerifyFileExists methods will enhance maintainability and assist developers in understanding their functionality.

---

60-60: Rename VerifyFileExists to FileExists for clarity.

The method name FileExists more clearly indicates that it returns a boolean value representing the existence of the file. This renaming improves code readability.

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

51-51: Ensure parameters are validated for null references.

Consider adding input validation or specifying parameter nullability to prevent potential ArgumentNullException issues with parameters like newFilePath and firstLogEvent.

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

129-134: Enhance exception logging for better troubleshooting.

While catching exceptions in RollToInitialSequenceNumber, consider logging the value of newFilePath and any other relevant variables. This additional context can aid in debugging issues related to file archiving.

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

104-149: Standardize exception handling and retry logic in ArchiveOldFileWithRetry.

The method treats IOException and general Exception differently with respect to retries and sleeps. For consistency and maintainability, consider applying similar retry mechanisms to both exception types.

Optionally, implement a more robust retry mechanism utilizing a retry policy or helper method to handle retries uniformly.

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

43-50: Add XML documentation for the class and constructor

The BaseFileArchiveHandler class and its constructor lack XML documentation comments. Providing XML documentation will enhance code readability and maintainability, and assist other developers in understanding the purpose and usage of this class.

---

91-96: Catch more specific exception types

Catching the general Exception class may obscure unexpected exceptions and make debugging harder. Consider catching more specific exceptions like IOException or UnauthorizedAccessException to handle known error scenarios more precisely.

---

119-133: Implement IComparable<FileInfoDateTime> instead of IComparer<FileInfoDateTime>

Currently, FileInfoDateTime implements IComparer<FileInfoDateTime>, which requires passing instances to the Compare method. Implementing IComparable<FileInfoDateTime> directly on the struct can simplify sorting and make the comparison logic more intuitive.

Apply this diff to implement IComparable<FileInfoDateTime>:

-struct FileInfoDateTime : IComparer<FileInfoDateTime>
+struct FileInfoDateTime : IComparable<FileInfoDateTime>

-public int Compare(FileInfoDateTime x, FileInfoDateTime y)
+public int CompareTo(FileInfoDateTime other)
 {
-    if (x.ArchiveSequenceNumber.HasValue && y.ArchiveSequenceNumber.HasValue)
+    if (this.ArchiveSequenceNumber.HasValue && other.ArchiveSequenceNumber.HasValue)
     {
-        return x.ArchiveSequenceNumber.Value.CompareTo(y.ArchiveSequenceNumber.Value);
+        return this.ArchiveSequenceNumber.Value.CompareTo(other.ArchiveSequenceNumber.Value);
     }
-    else if (x.FileCreatedTimeUtc == y.FileCreatedTimeUtc)
+    else if (this.FileCreatedTimeUtc == other.FileCreatedTimeUtc)
     {
-        return StringComparer.OrdinalIgnoreCase.Compare(x.FileInfo.Name, y.FileInfo.Name);
+        return StringComparer.OrdinalIgnoreCase.Compare(this.FileInfo.Name, other.FileInfo.Name);
     }
     else
     {
-        return x.FileCreatedTimeUtc.CompareTo(y.FileCreatedTimeUtc);
+        return this.FileCreatedTimeUtc.CompareTo(other.FileCreatedTimeUtc);
     }
 }

And update the sort call in line 192:

-fileInfoDates.Sort((x, y) => x.Compare(x, y));
+fileInfoDates.Sort();

---

243-259: Simplify sequence number parsing using int.TryParse

The current implementation manually parses the sequence number character by character. Simplify and improve robustness by using int.TryParse on the substring containing the sequence digits.

Apply this diff to simplify the parsing logic:

-private static bool TryParseStartSequenceNumber(string archiveFileName, int seqStartIndex, out int archiveSequenceNo)
-{
-    int? parsedSequenceNo = null;
-
-    int startIndex = seqStartIndex;
-    for (int i = startIndex; i < archiveFileName.Length; ++i)
-    {
-        char chr = archiveFileName[i];
-        if (!char.IsNumber(chr))
-            break;
-
-        parsedSequenceNo = parsedSequenceNo > 0 ? parsedSequenceNo * 10 : 0;
-        parsedSequenceNo += (chr - '0');
-    }
-    archiveSequenceNo = parsedSequenceNo ?? 0;
-    return parsedSequenceNo.HasValue;
-}
+private static bool TryParseStartSequenceNumber(string archiveFileName, int seqStartIndex, out int archiveSequenceNo)
+{
+    archiveSequenceNo = 0;
+    int endIndex = seqStartIndex;
+    while (endIndex < archiveFileName.Length && char.IsNumber(archiveFileName[endIndex]))
+    {
+        endIndex++;
+    }
+    if (endIndex > seqStartIndex)
+    {
+        string sequenceString = archiveFileName.Substring(seqStartIndex, endIndex - seqStartIndex);
+        return int.TryParse(sequenceString, out archiveSequenceNo);
+    }
+    return false;
+}

---

348-361: Ensure proper disposal of file handles

In FixWindowsFileSystemTunneling, the code creates a file using File.Create(newFilePath).Dispose();. If an exception occurs before Dispose is called, it could lead to resource leaks. Consider using a using statement to ensure the file handle is properly disposed even if an exception is thrown.

Apply this diff to use a using statement:

-File.Create(newFilePath).Dispose();
+using (var fs = File.Create(newFilePath))
+{
+    // File created; no need to write anything.
+}

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

38-45: Add XML documentation for the class and method

The DisabledFileArchiveHandler class and its ArchiveBeforeOpenFile method lack XML documentation comments. Adding documentation will improve code clarity and assist developers in understanding that this handler effectively disables file archiving.

---

42-45: Consider marking Default as static readonly DisabledFileArchiveHandler

Since Default is assigned an instance of DisabledFileArchiveHandler, it might be clearer to specify its exact type rather than the interface. This can prevent accidental reassignment and enhance type safety.

Apply this diff:

-public static readonly IFileArchiveHandler Default = new DisabledFileArchiveHandler();
+public static readonly DisabledFileArchiveHandler Default = new DisabledFileArchiveHandler();

tests/NLog.UnitTests/Internal/FileInfoHelperTests.cs (2)

39-47: Add test cases for edge conditions

While the DetectFilePathKind method tests various paths, consider adding edge cases such as paths with special characters, very long paths, or paths with mixed separators to ensure the method handles all possible inputs correctly.

---

60-76: Use platform-specific test attributes instead of runtime checks

In DetectFilePathKindWindowsPath, the test skips execution on non-Windows platforms using a runtime check. Instead, use platform-specific test attributes to conditionally include or exclude the test, which makes the test intentions clearer.

Apply this diff to use the PlatformSpecific attribute:

+ [PlatformSpecific(TestPlatforms.Windows)]
 [Theory]
 [InlineData(@"d:\test.log", false)]
 // ... other test cases ...
 public void DetectFilePathKindWindowsPath(string path, bool expected)
 {
-    if (System.IO.Path.DirectorySeparatorChar != '\\')
-        return; //no backward-slash on linux
+    // Test code remains the same
 }

Don't forget to include the necessary using directive:

+using System.Runtime.InteropServices;

If PlatformSpecific is not available, consider using Skip parameter in [Theory]:

 [Theory]
+ [InlineData(@"d:\test.log", false, Skip = "Windows-specific test")]

src/NLog/Targets/FileAppenders/MinimalFileLockingAppender.cs (2)

63-80: Consider caching FileInfo to improve performance.

The NextArchiveTime property creates a new FileInfo instance on each access when recalculation is needed. Consider caching the FileInfo instance when _nextArchiveTime is updated to reduce filesystem operations.

 public DateTime NextArchiveTime
 {
     get
     {
         if (_nextArchiveTime < NLog.Time.TimeSource.Current.Time.AddMinutes(1) || _lastFileBirthTimeUtc == DateTime.MinValue)
         {
-            var fileInfo = new FileInfo(_filePath);
+            if (_cachedFileInfo == null || _lastFileInfoUpdate < DateTime.UtcNow.AddSeconds(-1))
+            {
+                _cachedFileInfo = new FileInfo(_filePath);
+                _lastFileInfoUpdate = DateTime.UtcNow;
+            }
+            var fileInfo = _cachedFileInfo;
             var fileBirthTimeUtc = (fileInfo.Exists && fileInfo.Length != 0) ? (FileInfoHelper.LookupValidFileCreationTimeUtc(fileInfo) ?? DateTime.MinValue) : DateTime.MinValue;

---

84-92: Consider caching FileSize for better performance.

Similar to the NextArchiveTime property, FileSize creates a new FileInfo instance on each access. Consider caching this value and updating it after successful writes.

+private long _cachedFileSize;
+private DateTime _lastFileSizeUpdate;

 public long FileSize
 {
     get
     {
-        var fileInfo = new FileInfo(_filePath);
-        var fileSize = fileInfo.Exists ? fileInfo.Length : 0;
-        return fileSize;
+        if (_lastFileSizeUpdate < DateTime.UtcNow.AddSeconds(-1))
+        {
+            var fileInfo = new FileInfo(_filePath);
+            _cachedFileSize = fileInfo.Exists ? fileInfo.Length : 0;
+            _lastFileSizeUpdate = DateTime.UtcNow;
+        }
+        return _cachedFileSize;
     }
 }

src/NLog/Internal/StringBuilderExt.cs (1)

373-376: Consider using string.AsSpan() for more efficient iteration.

The change to foreach loop improves readability. However, for better performance, consider using string.AsSpan() which avoids string allocation and provides more efficient iteration:

-            int i = 0;
-            foreach (var chr in other)
-            {
-                if (builder[i++] != chr)
+            var span = other.AsSpan();
+            for (int i = 0; i < span.Length; i++)
+            {
+                if (builder[i] != span[i])
                    return false;
            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

tests/NLog.UnitTests/Internal/FileInfoHelperTests.cs (3)

41-57: Consider adding more test cases for comprehensive coverage.

While the current test cases cover many scenarios, consider adding tests for:

• Paths with multiple consecutive slashes (e.g., "path//to//file")
• Paths containing environment variables (e.g., "%TEMP%/file.log")
• Paths with Unicode characters

+        [InlineData(@"path//to//file", true)]
+        [InlineData(@"%TEMP%/file.log", true)]
+        [InlineData(@"路径/文件.log", true)]

---

72-73: Consider using platform-specific test attributes instead of runtime checks.

Rather than skipping the test at runtime, consider using xUnit's [PlatformSpecific] attribute or Skip condition to make the test execution behavior more explicit.

-        public void DetectFilePathKindWindowsPath(string path, bool expected)
-        {
-            if (System.IO.Path.DirectorySeparatorChar != '\\')
-                return; //no backward-slash on linux
+        [PlatformTheory(Platform.Windows)]
+        public void DetectFilePathKindWindowsPath(string path, bool expected)
+        {

---

61-69: Add test cases for additional Windows-specific path formats.

Consider adding test cases for:

• Extended-length paths (\\?\)
• Reserved names (CON, PRN, etc.)
• Paths with dots (dir\.\file)

+        [InlineData(@"\\?\C:\test.log", false)]
+        [InlineData(@"C:\CON", false)]
+        [InlineData(@".\test.log", true)]
+        [InlineData(@"dir\.\file", true)]

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

42-45: Enhance class documentation to clarify the "zero" strategy.

The current documentation is minimal. Consider adding more details about:

• What "truncate the active on archive" means in practice
• When this handler should be used vs other handlers
• The implications of never performing rolling

 /// <summary>
-/// Truncate the active on archive, and never perform any rolling
+/// File archive handler that implements a "zero" strategy: truncates the active file when archiving
+/// is triggered, without performing any file rolling operations.
+/// </summary>
+/// <remarks>
+/// This handler is suitable when you want to:
+/// - Simply clear the active log file without maintaining archive copies
+/// - Minimize disk operations during archiving
+/// - Avoid the complexity of file rolling operations
 /// </summary>

---

71-72: Update comment to accurately reflect the implementation.

The comment "Delete with wildcard (also files from yesterday)" is misleading as there's no time-based filtering in the implementation.

-                // Delete with wildcard (also files from yesterday)
+                // Delete with wildcard when filename contains digits

---

68-73: Consider optimizing the digit check.

The current implementation uses LINQ's Any to check for digits in the filename. For better performance, consider using a direct loop or IndexOfAny with a cached array of digit characters.

-                    var filePathWithoutExtension = Path.GetFileNameWithoutExtension(newFilePath);
-                    if (filePathWithoutExtension.Any(chr => char.IsDigit(chr)))
+                    // Cache the digit characters array
+                    private static readonly char[] s_digits = "0123456789".ToCharArray();
+                    
+                    var filePathWithoutExtension = Path.GetFileNameWithoutExtension(newFilePath);
+                    if (filePathWithoutExtension.IndexOfAny(s_digits) >= 0)

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

41-44: Enhance class documentation with more details.

While the current documentation describes the basic purpose, it would be helpful to add:

• Examples of sequence number rolling
• Explanation of how it integrates with BaseFileArchiveHandler
• Description of the IFileArchiveHandler contract implementation

---

51-51: Add XML documentation for the public method.

Add XML documentation to describe:

• Parameters and their purpose
• Return value meaning
• Scenarios when this method is called

---

55-67: Simplify complex conditional logic.

The nested conditions for file deletion and archiving could be simplified for better readability.

Consider refactoring like this:

-            if (_fileTarget.MaxArchiveFiles >= 0 || _fileTarget.MaxArchiveDays > 0 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
-            {
-                bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen);
-
-                if (_fileTarget.MaxArchiveFiles == 0 || _fileTarget.MaxArchiveFiles == 1 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
-                {
-                    if (deletedOldFiles)
-                    {
-                        FixWindowsFileSystemTunneling(newFilePath);
-                    }
-                    return 0;
-                }
-            }
+            bool shouldDeleteOldFiles = _fileTarget.MaxArchiveFiles >= 0 
+                || _fileTarget.MaxArchiveDays > 0 
+                || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup);
+            
+            if (!shouldDeleteOldFiles)
+                return newSequenceNumber;
+            
+            bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen);
+            
+            bool shouldStopArchiving = _fileTarget.MaxArchiveFiles == 0 
+                || _fileTarget.MaxArchiveFiles == 1 
+                || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup);
+                
+            if (shouldStopArchiving)
+            {
+                if (deletedOldFiles)
+                {
+                    FixWindowsFileSystemTunneling(newFilePath);
+                }
+                return 0;
+            }

---

113-116: Extract complex wildcard pattern logic into a separate method.

The wildcard pattern construction logic is complex and would be more maintainable as a separate method.

Consider extracting to a method like this:

+        private (string archivePathWildCard, int startIndex, int endIndex) GetWildcardPattern(string newFilePath, string filename)
+        {
+            var archivePathWildCard = _fileTarget.BuildFullFilePath(newFilePath, int.MaxValue, DateTime.MinValue)
+                .Replace(int.MaxValue.ToString(), "*");
+            var archiveFileName = Path.GetFileName(archivePathWildCard);
+            var startIndex = archiveFileName.IndexOf('*');
+            var endIndex = startIndex >= 0 ? archiveFileName.Length - startIndex : filename.Length;
+            return (archivePathWildCard, startIndex, endIndex);
+        }

---

130-137: Consider using more specific exception handling.

The current catch block handles all exceptions the same way. Consider catching specific exceptions like IOException or UnauthorizedAccessException separately.

Example:

-            catch (Exception ex)
+            catch (IOException ex)
+            {
+                InternalLogger.Warn(ex, "{0}: IO error while resolving archive sequence number for file: {1}", _fileTarget, newFilePath);
+                if (ex.MustBeRethrown(_fileTarget))
+                    throw;
+                return newSequenceNumber;
+            }
+            catch (UnauthorizedAccessException ex)
+            {
+                InternalLogger.Warn(ex, "{0}: Access denied while resolving archive sequence number for file: {1}", _fileTarget, newFilePath);
+                if (ex.MustBeRethrown(_fileTarget))
+                    throw;
+                return newSequenceNumber;
+            }
+            catch (Exception ex)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/NLog/Internal/FileInfoHelper.cs (1)

64-65: Optimize string allocation in path validation.

The TrimStart call creates a new string instance even when there's nothing to trim. Consider checking if trimming is needed first.

-            if (filepath?.Length > 0)
-                filepath = filepath.TrimStart(ArrayHelper.Empty<char>());
+            if (string.IsNullOrEmpty(filepath))
+                return false;
+
+            int start = 0;
+            while (start < filepath.Length && char.IsWhiteSpace(filepath[start]))
+                start++;
+
+            if (start == filepath.Length)
+                return false;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

51-78: Enhance method documentation and readability.

The method handles complex archiving logic but could benefit from the following improvements:

1. Add XML documentation for parameters and return value
2. Break down complex conditions into named variables
3. Add debug logging for better troubleshooting

Consider applying this diff:

+    /// <summary>
+    /// Performs archiving operations before opening a new file
+    /// </summary>
+    /// <param name="newFilePath">Path to the new file being created</param>
+    /// <param name="firstLogEvent">First log event to be written</param>
+    /// <param name="previousFileLastModified">Last modified date of the previous file</param>
+    /// <param name="newSequenceNumber">New sequence number for the file</param>
+    /// <returns>The final sequence number to use</returns>
     public virtual int ArchiveBeforeOpenFile(string newFilePath, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)
     {
         bool initialFileOpen = newSequenceNumber == 0;
+        InternalLogger.Debug("{0}: Archive operation starting for {1} (Initial: {2}, Sequence: {3})", _fileTarget, newFilePath, initialFileOpen, newSequenceNumber);
 
-        if (_fileTarget.MaxArchiveFiles >= 0 || _fileTarget.MaxArchiveDays > 0 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
+        bool shouldManageOldFiles = _fileTarget.MaxArchiveFiles >= 0 || 
+                                   _fileTarget.MaxArchiveDays > 0 || 
+                                   (initialFileOpen && _fileTarget.DeleteOldFileOnStartup);
+        if (shouldManageOldFiles)
         {
             bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen);
 
-            if (_fileTarget.MaxArchiveFiles == 0 || _fileTarget.MaxArchiveFiles == 1 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
+            bool shouldSkipArchiving = _fileTarget.MaxArchiveFiles == 0 || 
+                                     _fileTarget.MaxArchiveFiles == 1 || 
+                                     (initialFileOpen && _fileTarget.DeleteOldFileOnStartup);
+            if (shouldSkipArchiving)
             {
                 if (deletedOldFiles)
                 {
                     FixWindowsFileSystemTunneling(newFilePath);
                 }
+                InternalLogger.Debug("{0}: Skipping archive for {1}", _fileTarget, newFilePath);
                 return 0;
             }
         }
 
         if (initialFileOpen)
         {
-            if (_fileTarget.ArchiveOldFileOnStartup || _fileTarget.ArchiveAboveSize != 0 || _fileTarget.ArchiveEvery != FileArchivePeriod.None)
+            bool shouldArchiveOnStartup = _fileTarget.ArchiveOldFileOnStartup || 
+                                        _fileTarget.ArchiveAboveSize != 0 || 
+                                        _fileTarget.ArchiveEvery != FileArchivePeriod.None;
+            if (shouldArchiveOnStartup)
             {
+                InternalLogger.Debug("{0}: Rolling to initial sequence number for {1}", _fileTarget, newFilePath);
                 return RollToInitialSequenceNumber(newFilePath);
             }
         }
 
+        InternalLogger.Debug("{0}: Using provided sequence number {1} for {2}", _fileTarget, newSequenceNumber, newFilePath);
         return newSequenceNumber;
     }

---

80-139: Refactor method for better maintainability.

The method is quite long and handles complex file path manipulation. Consider breaking it down into smaller, focused methods.

Consider extracting the following helper methods:

1. File path manipulation:

+    private (string filename, string extension, string wildcard) GetFileComponents(string filePath)
+    {
+        var filename = Path.GetFileNameWithoutExtension(filePath);
+        var fileext = Path.GetExtension(filePath);
+        var fileWildCard = filename + "*" + fileext;
+        return (filename, fileext, fileWildCard);
+    }

2. Archive file search:

+    private FileInfo[] GetArchiveFiles(string directory, string wildcard)
+    {
+        var directoryInfo = new DirectoryInfo(directory);
+        return !directoryInfo.Exists ? Array.Empty<FileInfo>() : directoryInfo.GetFiles(wildcard);
+    }

3. Sequence number calculation:

+    private int CalculateNextSequenceNumber(FileInfo[] files, string archivePathPattern)
+    {
+        var archiveFileName = Path.GetFileName(archivePathPattern);
+        var fileWildcardStartIndex = archiveFileName.IndexOf('*');
+        var fileWildcardEndIndex = fileWildcardStartIndex >= 0 ? 
+            archiveFileName.Length - fileWildcardStartIndex : 
+            Path.GetFileNameWithoutExtension(archivePathPattern).Length;
+
+        var maxSequenceNo = GetMaxArchiveSequenceNo(files, fileWildcardStartIndex, fileWildcardEndIndex) ?? 0;
+        return _fileTarget.ArchiveOldFileOnStartup ? maxSequenceNo + 1 : maxSequenceNo;
+    }

This would make the main method more readable:

     private int RollToInitialSequenceNumber(string newFilePath)
     {
         try
         {
             var filedir = Path.GetDirectoryName(newFilePath);
             if (string.IsNullOrEmpty(filedir))
                 return 0;
 
-            var directoryInfo = new DirectoryInfo(filedir);
-            if (!directoryInfo.Exists)
-                return 0;
-
-            var filename = Path.GetFileNameWithoutExtension(newFilePath);
-            var fileext = Path.GetExtension(newFilePath);
-            var fileWildCard = filename + "*" + fileext;
-            var fileInfos = directoryInfo.GetFiles(fileWildCard);
+            var (_, _, fileWildCard) = GetFileComponents(newFilePath);
+            var fileInfos = GetArchiveFiles(filedir, fileWildCard);
             InternalLogger.Debug("{0}: Archive Sequence Rolling found {1} files matching wildcard {2} in directory: {3}", 
                 _fileTarget, fileInfos.Length, fileWildCard, filedir);
 
             if (fileInfos.Length == 0)
                 return 0;
 
             if (_fileTarget.DeleteOldFileOnStartup)
             {
-                // Delete all files in the directory with matching filename-wildcard
-                foreach (var fileInfo in fileInfos)
-                {
-                    DeleteOldArchiveFile(fileInfo.FullName);
-                }
+                Array.ForEach(fileInfos, file => DeleteOldArchiveFile(file.FullName));
                 return 0;
             }
 
             var archivePathWildCard = _fileTarget.BuildFullFilePath(newFilePath, int.MaxValue, DateTime.MinValue)
                 .Replace(int.MaxValue.ToString(), "*");
-            var archiveFileName = Path.GetFileName(archivePathWildCard);
-            var fileWildcardStartIndex = archiveFileName.IndexOf('*');
-            var fileWildcardEndIndex = fileWildcardStartIndex >= 0 ? 
-                archiveFileName.Length - fileWildcardStartIndex : 
-                filename.Length;
-
-            newSequenceNumber = GetMaxArchiveSequenceNo(fileInfos, fileWildcardStartIndex, fileWildcardEndIndex) ?? 0;
-            if (_fileTarget.ArchiveOldFileOnStartup)
-            {
-                newSequenceNumber += 1;
-            }
+            return CalculateNextSequenceNumber(fileInfos, archivePathWildCard);
-            return newSequenceNumber;
         }
         catch (Exception ex)
         {
             InternalLogger.Warn(ex, "{0}: Failed to resolve initial archive sequence number for file: {1}", 
                 _fileTarget, newFilePath);
             if (ex.MustBeRethrown(_fileTarget))
                 throw;
 
-            return newSequenceNumber;
+            return 0;
         }
     }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

42-47: Enhance concurrency-related documentation in remarks.

The remarks note that archiving can fail if files are locked by other applications or multi-process logging. Consider expanding these remarks to include guidance around concurrency scenarios, so users are fully aware of potential race conditions and recommended mitigation strategies.

---

72-93: Consider additional concurrency checks and logs when deleting old files.

When multiple processes run the same archiving logic simultaneously, file deletion or enumeration of wildcard patterns might cause race conditions. Adding more in-depth logs or concurrency checks could help diagnose issues in multi-process environments.

---

104-149: Refine the retry strategy to a more robust exponential backoff.

Using Thread.Sleep(i * 10) is a simple approach but may be insufficient in heavily contended environments. Consider using an exponential or randomized backoff strategy, especially if file locks last longer than the small increments offered here.

Below is an optional snippet that illustrates a short exponential backoff approach:

 for (int i = 1; i <= 3; ++i)
 {
     try
     {
         ...
     }
     catch (IOException ex)
     {
         InternalLogger.Debug(ex, "{0}: Failed to archive file: '{1}'", _fileTarget, newFilePath);
         if (!File.Exists(newFilePath))
             return oldFilesDeleted;
-        if (i >= 3 && LogManager.ThrowExceptions)
+        if (i >= 3 && LogManager.ThrowExceptions)
         {
             throw;
         }
     }
     catch (Exception ex)
     {
         ...
     }
-    System.Threading.Thread.Sleep(i * 10);
+    // Exponential backoff with a max of 500ms, for example
+    var backoffMillis = (int)Math.Min(Math.Pow(2, i - 1) * 50, 500);
+    System.Threading.Thread.Sleep(backoffMillis);
 }

---

151-178: Clarify the logic for timestamp adjustments.

Line 155 adjusts the last write time if previousFileLastModified is greater than the file’s last write time or if the file’s last write time is at or beyond firstLogEvent.TimeStamp. Consider adding explanatory comments so maintainers understand this time reassignment, especially if the file’s last write time is in the future.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

41-44: Enhance class documentation for better maintainability.

The current XML documentation is minimal. Consider adding:

• <remarks> section explaining the rolling archive strategy
• Documentation about thread-safety guarantees
• Examples of sequence number generation
• Relationship with BaseFileArchiveHandler

---

51-52: Add XML documentation for public method.

Add XML documentation for the public method including:

• Purpose and behavior
• Parameter descriptions using <param>
• Return value description using <returns>
• Exceptions that might be thrown using <exception>

---

51-78: Consider breaking down complex method into smaller focused methods.

The method handles multiple responsibilities:

1. Deletion of old files
2. Initial file opening logic
3. Sequence number management

Consider extracting these into separate private methods for better maintainability and testability:

• HandleOldFilesDeletion
• HandleInitialFileOpen

Also, consider adding parameter validation:

 public virtual int ArchiveBeforeOpenFile(string newFilePath, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)
 {
+    if (string.IsNullOrEmpty(newFilePath))
+        throw new ArgumentNullException(nameof(newFilePath));
+    if (firstLogEvent is null)
+        throw new ArgumentNullException(nameof(firstLogEvent));
+    if (newSequenceNumber < 0)
+        throw new ArgumentOutOfRangeException(nameof(newSequenceNumber), "Sequence number cannot be negative");
+
     bool initialFileOpen = newSequenceNumber == 0;

---

94-100: Add comments explaining the wildcard pattern logic.

The wildcard pattern construction and file matching logic is complex. Consider adding comments to explain:

• Why * is added between filename and extension
• What types of files this pattern will match
• Examples of matched filenames

---

133-137: Consider logging more details about the exception.

When logging the warning, include more context about what operation failed:

-    InternalLogger.Warn(ex, "{0}: Failed to resolve initial archive sequence number for file: {1}", _fileTarget, newFilePath);
+    InternalLogger.Warn(ex, "{0}: Failed to resolve initial archive sequence number for file: {1}. Directory: {2}, Pattern: {3}", 
+        _fileTarget, newFilePath, filedir, fileWildCard);

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

50-53: Consider a null-check for fileTarget.

Although the constructor uses a base class property, a missing safeguard could lead to a NullReferenceException if fileTarget is unexpectedly null.

 public LegacyArchiveFileNameHandler(FileTarget fileTarget)
     : base(fileTarget)
 {
+    if (fileTarget == null)
+        throw new ArgumentNullException(nameof(fileTarget));
 }

---

104-149: Improve retry resilience.

The retry loop for archiving a locked file is limited to 3 attempts with minimal backoff. Prolonged file locks could persist beyond this duration. Consider increasing attempts or using exponential backoff for robustness.

---

238-277: Sequence number logic is clear but prone to race conditions.

ResolveNextArchiveSequenceNo scans existing archive files for the highest number, then increments. If other processes create or archive a file concurrently, the final name might collide.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

116-119: ⚠️ Potential issue

Secure the file path manipulation against path traversal.

The file path manipulation using Replace and IndexOf could be vulnerable to path traversal if the input contains malicious patterns.

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

51-80: Consider extracting complex conditions into well-named methods.

The method handles multiple archiving scenarios with complex conditional logic. Consider extracting conditions into descriptive methods to improve readability.

Example refactor:

     public virtual int ArchiveBeforeOpenFile(string newFileName, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)
     {
         bool initialFileOpen = newSequenceNumber == 0;
-        if (_fileTarget.MaxArchiveFiles >= 0 || _fileTarget.MaxArchiveDays > 0 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
+        if (ShouldDeleteOldFiles(initialFileOpen))
         {
             var newFilePath = FileTarget.CleanFullFilePath(newFileName);
             bool deletedOldFiles = DeleteOldFilesBeforeArchive(newFilePath, initialFileOpen);
 
-            if (_fileTarget.MaxArchiveFiles == 0 || _fileTarget.MaxArchiveFiles == 1 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
+            if (ShouldSkipArchiving(initialFileOpen))
             {
                 if (deletedOldFiles)
                 {
                     FixWindowsFileSystemTunneling(newFilePath);
                 }
                 return 0;
             }
         }
 
         if (initialFileOpen)
         {
-            if (_fileTarget.ArchiveOldFileOnStartup || _fileTarget.ArchiveAboveSize != 0 || _fileTarget.ArchiveEvery != FileArchivePeriod.None)
+            if (ShouldArchiveOnStartup())
             {
                 var newFilePath = FileTarget.CleanFullFilePath(newFileName);
                 return RollToInitialSequenceNumber(newFilePath);
             }
         }
 
         return newSequenceNumber;
     }
+
+    private bool ShouldDeleteOldFiles(bool initialFileOpen)
+    {
+        return _fileTarget.MaxArchiveFiles >= 0 || 
+               _fileTarget.MaxArchiveDays > 0 || 
+               (initialFileOpen && _fileTarget.DeleteOldFileOnStartup);
+    }
+
+    private bool ShouldSkipArchiving(bool initialFileOpen)
+    {
+        return _fileTarget.MaxArchiveFiles == 0 || 
+               _fileTarget.MaxArchiveFiles == 1 || 
+               (initialFileOpen && _fileTarget.DeleteOldFileOnStartup);
+    }
+
+    private bool ShouldArchiveOnStartup()
+    {
+        return _fileTarget.ArchiveOldFileOnStartup || 
+               _fileTarget.ArchiveAboveSize != 0 || 
+               _fileTarget.ArchiveEvery != FileArchivePeriod.None;
+    }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

48-54: Consider potential concurrency pitfalls when using legacy file archiving.
This class is marked "legacy" and relies on simple file-moving logic that can fail if multiple processes access the same file simultaneously. As stated in the comments, you may experience stability issues when the file is locked by other processes.

---

74-99: Refactor the boolean expression for improved readability.
The variable wildCardStrictSeqNo contains a lengthy conditional check for both the archive suffix format and layout text. Splitting the logic into smaller methods or more descriptive condition checks can improve maintainability and reduce complexity.

- bool wildCardStrictSeqNo = _fileTarget.ArchiveSuffixFormat.IndexOf("{0", StringComparison.Ordinal) >= 0 && ...
+ bool isStrictSeqFormat = _fileTarget.ArchiveSuffixFormat.IndexOf("{0", StringComparison.Ordinal) >= 0
+     && _fileTarget.ArchiveSuffixFormat.IndexOf("{1", StringComparison.Ordinal) < 0;
+ bool isNotDateLayout = !( ... );
+ bool wildCardStrictSeqNo = isStrictSeqFormat && isNotDateLayout;

---

106-151: Revisit the retry approach with Thread.Sleep for multi-process scenarios.
Relying on Thread.Sleep(i * 10) may not be ideal in high-concurrency environments, as it introduces non-deterministic delays for file archiving. A more robust approach might involve backoff strategies or system-provided synchronization primitives to coordinate file access between processes.

---

201-201: Address the TODO comment about handling double footers.
Leaving the footer from the original file and appending any new footer logic can lead to duplicated content at the end of the archive. This could cause confusion during log analysis.

Do you want help drafting a solution for properly handling or removing any potential footer duplicates?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

232-268: ⚠️ Potential issue

Guard against race conditions in sequence number increments
When multiple processes write to the same directory, parallel archiving can result in overlapping sequence numbers. As previously noted, a more robust synchronization mechanism is advisable.

Do you want me to provide a potential approach using an atomic counter or cross-process lock for sequence number increments?

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

36-49: Consider sealing the class if not intended for subclassing
This class is marked internal and extends RollingArchiveFileHandler. If there is no requirement to extend it further, marking it as sealed can prevent unintended inheritance and optimize runtime calls.

-internal class LegacyArchiveFileNameHandler : RollingArchiveFileHandler, IFileArchiveHandler
+internal sealed class LegacyArchiveFileNameHandler : RollingArchiveFileHandler, IFileArchiveHandler

---

74-105: Fragile wildcard logic
The wildcard logic at lines 79-84 relies on string manipulation, searching for placeholders ("int.MaxValue") to create a pattern. Consider refactoring this into a dedicated helper method to reduce complexity and potential off-by-one or naming collision issues.

---

193-193: Address TODO regarding double footer
Line 193 references a TODO about handling double footers. This could lead to data corruption or unexpected file content. Consider adding a check for existing footers to avoid appending them twice.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

38-46: Add XML documentation for better API clarity.

The implementation is clean and follows good patterns (singleton, null object). Consider adding XML documentation to describe:

• The purpose of this disabled handler
• The meaning of the return value
• Parameter descriptions for ArchiveBeforeOpenFile

Example documentation:

+    /// <summary>
+    /// Provides a disabled implementation of <see cref="IFileArchiveHandler"/> that performs no archiving.
+    /// </summary>
     internal sealed class DisabledFileArchiveHandler : IFileArchiveHandler
     {
+        /// <summary>
+        /// Gets the singleton instance of the disabled file archive handler.
+        /// </summary>
         public static readonly IFileArchiveHandler Default = new DisabledFileArchiveHandler();

+        /// <summary>
+        /// Always returns 0 to indicate no archiving should be performed.
+        /// </summary>
+        /// <param name="newFileName">The name of the file being created.</param>
+        /// <param name="firstLogEvent">The first log event to be written.</param>
+        /// <param name="previousFileLastModified">The last write time of the previous file, if any.</param>
+        /// <param name="newSequenceNumber">The sequence number for the new file.</param>
+        /// <returns>Always returns 0 to indicate no archive action is needed.</returns>
         public int ArchiveBeforeOpenFile(string newFileName, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

38-38: Well-designed class declaration.

Good use of internal sealed modifiers. The class follows the Single Responsibility Principle by focusing solely on representing a disabled state for file archiving.

Consider documenting the purpose of this handler in XML comments, particularly explaining when to use this implementation versus other archive handlers.

---

42-45: Implementation is correct but could benefit from more documentation.

The implementation correctly represents a disabled state by always returning 0. Consider adding XML documentation to explain:

• The meaning of the return value (0)
• The purpose of each parameter
• When this implementation should be used

Add XML documentation:

+    /// <summary>
+    /// Implements the archive handler interface with disabled functionality.
+    /// </summary>
+    /// <param name="newFileName">The name of the file being created.</param>
+    /// <param name="firstLogEvent">The first log event to be written.</param>
+    /// <param name="previousFileLastModified">The last modified time of the previous file, if any.</param>
+    /// <param name="newSequenceNumber">The sequence number for the new file.</param>
+    /// <returns>Always returns 0 to indicate no archive action was taken.</returns>
     public int ArchiveBeforeOpenFile(string newFileName, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)

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

55-72: Add null check for newFileName parameter.

While the method handles null/whitespace check for archiveFileName, it should also validate the newFileName parameter before using it in FileTarget.CleanFullFilePath.

 public override int ArchiveBeforeOpenFile(string newFileName, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)
 {
+    if (string.IsNullOrEmpty(newFileName))
+        throw new ArgumentNullException(nameof(newFileName));
+
     var archiveFileName = _fileTarget.ArchiveFileName?.Render(firstLogEvent);
     if (StringHelpers.IsNullOrWhiteSpace(archiveFileName))

---

74-104: Extract wildcard pattern matching logic into a separate method.

The complex wildcard pattern matching logic in ArchiveBeforeOpenFile makes the method harder to understand and maintain.

Consider extracting the wildcard logic into a separate method:

+private bool ShouldUseWildCardStrictSequenceNumber(string archiveFileName)
+{
+    return _fileTarget.ArchiveSuffixFormat.IndexOf("{0", StringComparison.Ordinal) >= 0 && 
+           _fileTarget.ArchiveSuffixFormat.IndexOf("{1", StringComparison.Ordinal) < 0 &&
+           !(_fileTarget.ArchiveFileName is SimpleLayout simpleLayout && 
+             (simpleLayout.OriginalText.IndexOf("${date", StringComparison.OrdinalIgnoreCase) >= 0 || 
+              simpleLayout.OriginalText.IndexOf("${shortdate", StringComparison.OrdinalIgnoreCase) >= 0));
+}

 private bool ArchiveBeforeOpenFile(string archiveFileName, string newFilePath, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, bool initialFileOpen)
 {
     bool oldFilesDeleted = false;
     if (_fileTarget.MaxArchiveFiles >= 0 || _fileTarget.MaxArchiveDays > 0 || (initialFileOpen && _fileTarget.DeleteOldFileOnStartup))
     {
-        bool wildCardStrictSeqNo = _fileTarget.ArchiveSuffixFormat.IndexOf("{0", StringComparison.Ordinal) >= 0 && _fileTarget.ArchiveSuffixFormat.IndexOf("{1", StringComparison.Ordinal) < 0 &&
-            !(_fileTarget.ArchiveFileName is SimpleLayout simpleLayout && (simpleLayout.OriginalText.IndexOf("${date", StringComparison.OrdinalIgnoreCase) >= 0 || simpleLayout.OriginalText.IndexOf("${shortdate", StringComparison.OrdinalIgnoreCase) >= 0));
+        bool wildCardStrictSeqNo = ShouldUseWildCardStrictSequenceNumber(archiveFileName);

---

191-230: Address TODO comment and optimize buffer handling.

Several improvements needed:

1. The TODO comment about double footer needs to be addressed
2. Consider using a larger buffer size for better performance
3. Use using declarations (C# 8.0+) for better resource cleanup

Would you like me to help implement the double footer handling logic?

-    private void ArchiveFileAppendExisting(string newFilePath, string archiveFilePath)
+    private void ArchiveFileAppendExisting(string newFilePath, string archiveFilePath, int bufferSize = 81920)
     {
-        // TODO Handle double footer
         InternalLogger.Info("{0}: Already exists, append to {1}", _fileTarget, archiveFilePath);
 
         var fileShare = FileShare.Read | FileShare.Delete;
-        using (FileStream newFileStream = File.Open(newFilePath, FileMode.Open, FileAccess.ReadWrite, fileShare))
-        using (FileStream archiveFileStream = File.Open(archiveFilePath, FileMode.Append))
+        using var newFileStream = File.Open(newFilePath, FileMode.Open, FileAccess.ReadWrite, fileShare);
+        using var archiveFileStream = File.Open(archiveFilePath, FileMode.Append);
         {
             // ... rest of the method
-            byte[] buffer = new byte[4096];
+            byte[] buffer = new byte[bufferSize];

---

1-270: Consider implementing the Command pattern for archive operations.

The current implementation mixes different responsibilities (file operations, sequence number management, path handling). Consider refactoring to use the Command pattern, separating these concerns into individual command objects.

This would:

1. Make the code more testable
2. Allow for better error handling
3. Enable easier implementation of new archive strategies
4. Facilitate rollback of failed operations

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

38-40: Well-implemented Null Object pattern!

The implementation follows best practices:

• Sealed class prevents inheritance
• Static readonly field ensures thread-safe singleton initialization
• Internal scope appropriately encapsulates the implementation

This is a good example of the Null Object pattern, providing a no-op implementation that simplifies client code by eliminating null checks.

---

42-45: Add XML documentation for better IDE support.

While the inline comment is helpful, adding XML documentation would improve IDE tooltips and make the API more discoverable.

Consider adding:

+    /// <summary>
+    /// Disabled implementation that never performs archiving.
+    /// </summary>
+    /// <param name="newFileName">The name of the file being created.</param>
+    /// <param name="firstLogEvent">The first log event to be written.</param>
+    /// <param name="previousFileLastModified">The last write time of the previous file, if any.</param>
+    /// <param name="newSequenceNumber">The sequence number for the new file.</param>
+    /// <returns>Always returns 0 to indicate no archiving was performed.</returns>
     public int ArchiveBeforeOpenFile(string newFileName, LogEventInfo firstLogEvent, DateTime? previousFileLastModified, int newSequenceNumber)

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

55-72: Avoid method name duplication.
There is a public method and a private overload both named ArchiveBeforeOpenFile. While valid, it can cause confusion. A distinct name for the private method could improve clarity.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro