FileTarget - Improve archive cleanup when using ArchiveSuffixFormat with datetime #6027
Conversation
WalkthroughAdds a boolean flag parseArchiveSequenceNo to archive-file filtering and propagates it through CleanupFiles and ExcludeFileName; refactors archive-related unit tests for configurable generation and date-based naming; and updates several archival and rolling-related log message texts. Changes
Sequence Diagram(s)sequenceDiagram
participant Cleanup as CleanupFiles(...)
participant Exclude as ExcludeFileName(...)
participant Parser as ArchiveSequenceParser
Cleanup->>Exclude: call(archiveFileName, fileWildcardStartIndex, fileWildcardEndIndex, parseArchiveSequenceNo, excludeFileName)
alt parseArchiveSequenceNo == true
Exclude->>Parser: parse sequence number from filename
Parser-->>Exclude: sequenceNo / none
Exclude-->>Cleanup: decide include/exclude using name + sequenceNo
else parseArchiveSequenceNo == false
Exclude-->>Cleanup: decide include/exclude using name only
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (2)
3641-3649: Nit: align timestamps with generated filenames.time is computed via a separate counter (i--) and can mismatch the date embedded in filePath. Consider deriving time from the actual filePath date or advancing a single DateTime used for both name and timestamps to keep intent obvious.
3676-3681: Fix size assertion: using Min makes the condition unsatisfiable for differing lengths.You compute min length, then assert each file.Length <= min, which only passes if all files share the minimum size. Use Max (or InRange) instead.
- var expectedFileSize = currentFiles.Min(f => f.Length); - foreach (var file in currentFiles) - Assert.True(file.Length > 0 && file.Length <= expectedFileSize, $"{file.Length} > {expectedFileSize}"); + var maxFileSize = currentFiles.Max(f => f.Length); + foreach (var file in currentFiles) + Assert.True(file.Length > 0 && file.Length <= maxFileSize, $"{file.Length} > {maxFileSize}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(4 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
src/NLog/Config/XmlLoggingConfiguration.cs (10)
XmlLoggingConfiguration(52-754)XmlLoggingConfiguration(61-64)XmlLoggingConfiguration(70-72)XmlLoggingConfiguration(79-84)XmlLoggingConfiguration(90-93)XmlLoggingConfiguration(100-103)XmlLoggingConfiguration(111-116)XmlLoggingConfiguration(161-166)XmlLoggingConfiguration(172-175)XmlLoggingConfiguration(182-185)
🔇 Additional comments (7)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (4)
169-171: Good: sequence-scan now ignores weekday-named archives.Passing parseArchiveSequenceNo = true when scanning max sequence ensures files like “Mon/Tue” are excluded from sequence detection.
192-195: Single-file cleanup guard is correct.The early-return path now respects ExcludeFileName with the new flag, preventing accidental deletion.
204-212: Cleanup respects lettered suffixes when not parsing sequences.Conditioning ExcludeFileName on parseArchiveSequenceNo avoids filtering out weekday-based archives during cleanup.
228-243: Clarify/verify wildcard assumptions for letter-scan.Letter-scan only runs when there’s a single wildcard (fileWildcardEndIndex > 0). If GetDeleteOldFileNameWildcard yields two wildcards, the scan is skipped by design. Please confirm the wildcard produced for “suffix-only” patterns (e.g., archiveFileName=".log" + archiveSuffixFormat="{1:ddd}") is a single-asterisk form so the intended letter-exclusion participates in sequence parsing.
tests/NLog.UnitTests/Targets/FileTargetTests.cs (3)
3608-3610: OK: calls updated for new helper signature.Both theory cases pass generateFilesCount, matching the refactored helper.
3612-3616: Nice coverage for weekday format “ddd”.Exercise 7 distinct weekdays with archiving-on-startup.
3625-3625: Helper signature refactor looks fine.Parameter order is clear; generates deterministic sets for different formats.
0919f3f to
652d8ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (5)
3612-3616: Stabilize "ddd" test against culture differences.
“ddd” is culture-sensitive. Pin culture within the test and (optionally) also cover the name-based cleanup path.Apply:
[Fact] public void MaxArchiveFilesWithWeekday() { - TestMaxArchiveFilesWithDate(7, 7, 7, "ddd", true); + var prev = System.Threading.Thread.CurrentThread.CurrentCulture; + var prevUi = System.Threading.Thread.CurrentThread.CurrentUICulture; + try + { + System.Threading.Thread.CurrentThread.CurrentCulture = System.Globalization.CultureInfo.InvariantCulture; + System.Threading.Thread.CurrentThread.CurrentUICulture = System.Globalization.CultureInfo.InvariantCulture; + TestMaxArchiveFilesWithDate(7, 7, 7, "ddd", true); // time-based + TestMaxArchiveFilesWithDate(7, 7, 7, "ddd", false); // name-based + } + finally + { + System.Threading.Thread.CurrentThread.CurrentCulture = prev; + System.Threading.Thread.CurrentThread.CurrentUICulture = prevUi; + } }
3618-3626: Update XML-doc to include new parameter.
Add a param doc for generateFilesCount to keep the helper’s contract clear./// <summary> /// /// </summary> -/// <param name="maxArchiveFilesConfig">max count of archived files</param> +/// <param name="generateFilesCount">number of mock archive files to pre-create</param> +/// <param name="maxArchiveFilesConfig">max count of archived files</param> /// <param name="expectedArchiveFiles">expected count of archived files</param> /// <param name="dateFormat">date format</param> /// <param name="changeCreationAndWriteTime">change file creation/last write date</param> private static void TestMaxArchiveFilesWithDate(int generateFilesCount, int maxArchiveFilesConfig, int expectedArchiveFiles, string dateFormat, bool changeCreationAndWriteTime)
3640-3643: Align file timestamps with generated filenames (off‑by‑one).
First file path is “now-1 day” (due to Skip(1)), but time starts at “now”. Make them match to avoid skew in time-based cleanup runs.- int i = 0; - foreach (string filePath in ArchiveFileNamesGenerator(archivePath, "{0:" + dateFormat + "}" + fileExt).Skip(1).Take(generateFilesCount)) + int dayOffset = -1; + foreach (string filePath in ArchiveFileNamesGenerator(archivePath, "{0:" + dateFormat + "}" + fileExt).Skip(1).Take(generateFilesCount)) { - var time = now.AddDays(i--); + var time = now.AddDays(dayOffset--);
3659-3666: Prefer OS-safe path construction in XML config.
Small nit: avoid hard-coded “/” and build archive file base consistently.- fileName='" + tempDir + @"/app" + fileExt + @"' + fileName='" + System.IO.Path.Combine(tempDir, "app" + fileExt).Replace("\\", "/") + @"' ... - archiveFileName='" + Path.Combine(archivePath, fileExt) + @"' + archiveFileName='" + Path.Combine(archivePath, fileExt).Replace("\\", "/") + @"'Note: Replace backslashes for XML readability on Windows; behavior unchanged.
3676-3681: Assertion could be less brittle.
Requiring every file length to equal the minimum can fail if one archive gets a tiny header/BOM difference. Consider asserting non-empty and within a small variance, or drop the size check entirely and just validate count.- var expectedFileSize = currentFiles.Min(f => f.Length); - foreach (var file in currentFiles) - Assert.True(file.Length > 0 && file.Length <= expectedFileSize, $"{file.Length} > {expectedFileSize}"); + Assert.All(currentFiles, f => Assert.True(f.Length > 0));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(9 hunks)src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs(2 hunks)src/NLog/Targets/FileTarget.cs(2 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/NLog/Targets/FileTarget.cs (1)
src/NLog/Common/InternalLogger.cs (1)
InternalLogger(51-511)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
src/NLog/Common/InternalLogger.cs (1)
InternalLogger(51-511)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
src/NLog/Config/XmlLoggingConfiguration.cs (10)
XmlLoggingConfiguration(52-754)XmlLoggingConfiguration(61-64)XmlLoggingConfiguration(70-72)XmlLoggingConfiguration(79-84)XmlLoggingConfiguration(90-93)XmlLoggingConfiguration(100-103)XmlLoggingConfiguration(111-116)XmlLoggingConfiguration(161-166)XmlLoggingConfiguration(172-175)XmlLoggingConfiguration(182-185)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (10)
src/NLog/Targets/FileTarget.cs (2)
823-823: LGTM: Improved log message clarity.The addition of "rolling" clarifies that archiving is triggered by dynamic filesize threshold checks.
833-833: LGTM: Improved log message clarity.Consistent with line 823, the "rolling" terminology better describes the time-based archive trigger.
src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs (2)
175-175: LGTM: Improved log message clarity.The "Archive" prefix and present-tense verb provide better context about the operation being performed.
204-204: LGTM: Improved log message clarity.Present-tense verb and explicit "file:" label make the message more descriptive and grammatically consistent.
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (5)
75-75: LGTM: Improved log message formatting.Adding quotes around the wildcard pattern improves readability and makes it easier to identify the search pattern in logs.
Also applies to: 80-80
98-98: LGTM: Correct parameter propagation.The
parseArchiveSequenceNoflag is properly propagated toCleanupFiles, enabling different archive naming strategies (e.g., weekday-based vs. sequence-based).
169-169: LGTM: Correct hardcoded value.Passing
trueforparseArchiveSequenceNois correct here since scanning for the maximum sequence number inherently requires parsing sequence numbers from filenames.
256-256: LGTM: Improved log message clarity.The log message updates improve consistency and readability across archive operations without changing behavior.
Also applies to: 357-357, 382-382, 407-407
188-188: Manual verification needed for weekday archive test and ExcludeFileName logic.I found that a test exists (
MaxArchiveFilesWithWeekday) for weekday-format archives using the "ddd" format. However, I cannot fully verify the correctness of the implementation without understanding the completeExcludeFileNamelogic in BaseFileArchiveHandler (lines 228-243).Key concerns:
RollingArchiveFileHandler automatically sets
parseArchiveSequenceNobased on whether digits exist in the active log filename. For "app.log" with weekday format archives, it setsparseArchiveSequenceNo=true. The impact on how weekday-named files ("Mon.log", "Tue.log") are handled during cleanup needs clarification.Test validation: Please confirm:
- The
MaxArchiveFilesWithWeekdaytest passes consistently- Archive files with weekday names are correctly preserved (not excluded during cleanup)
- The test actually exercises the cleanup logic for all 7 weekday scenarios
ExcludeFileName implementation: Examine lines 228-243 to verify files with letters (like weekday names) are handled correctly when
parseArchiveSequenceNo=true.tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
3608-3610: Calls updated to new helper signature — looks good.
652d8ff to
44c514e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (2)
3625-3626: Minor: align file timestamp with generated filename for clarity.You skip(1) to avoid “today”, but set time = now.AddDays(i--) starting from 0. Consider deriving “time” from the same date used in ArchiveFileNamesGenerator to avoid confusion.
Also applies to: 3641-3643
3677-3681: Fix brittle size assertion (uses Min as an upper bound).As written, all files must have exactly the minimum size. Relax to assert non-empty (and optionally check narrow range if needed).
- var currentFiles = archiveDir.GetFiles(); - Assert.Equal(expectedArchiveFiles, currentFiles.Length); - var expectedFileSize = currentFiles.Min(f => f.Length); - foreach (var file in currentFiles) - Assert.True(file.Length > 0 && file.Length <= expectedFileSize, $"{file.Length} > {expectedFileSize}"); + var currentFiles = archiveDir.GetFiles(); + Assert.Equal(expectedArchiveFiles, currentFiles.Length); + foreach (var file in currentFiles) + Assert.True(file.Length > 0); + // Optional: tighten if you want near-uniform sizes + // var minSize = currentFiles.Min(f => f.Length); + // var maxSize = currentFiles.Max(f => f.Length); + // Assert.InRange(maxSize, minSize, minSize + 256);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(9 hunks)src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs(2 hunks)src/NLog/Targets/FileTarget.cs(2 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NLog/Targets/FileArchiveHandlers/LegacyArchiveFileNameHandler.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/NLog/Targets/FileTarget.cs (1)
src/NLog/Common/InternalLogger.cs (1)
InternalLogger(51-511)
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
src/NLog/Config/XmlLoggingConfiguration.cs (10)
XmlLoggingConfiguration(52-754)XmlLoggingConfiguration(61-64)XmlLoggingConfiguration(70-72)XmlLoggingConfiguration(79-84)XmlLoggingConfiguration(90-93)XmlLoggingConfiguration(100-103)XmlLoggingConfiguration(111-116)XmlLoggingConfiguration(161-166)XmlLoggingConfiguration(172-175)XmlLoggingConfiguration(182-185)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
src/NLog/Common/InternalLogger.cs (1)
InternalLogger(51-511)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (10)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (5)
75-81: Clearer internal logging for archive cleanup.Wording and quoting improvements help diagnostics; no behavioral change.
169-177: Sequence scan correctly skips filenames with letters in wildcard segment.Passing parseArchiveSequenceNo=true here is appropriate for max-seq scanning.
256-258: More informative “old file” log message.Extra context (age, dates) is helpful; behavior unchanged.
357-361: Archive cleanup logging tweaks.Consistent phrasing and quoting; good for troubleshooting.
Also applies to: 383-387, 407-408
190-213: Code already correctly gates letter-based exclusion and sets parseArchiveSequenceNo at all call sites; no issues found.The verification shows:
CleanupFiles (lines 190-213) already uses the
parseArchiveSequenceNoparameter correctly at lines 192 and 204.Call sites properly derive the flag from archive format:
- LegacyArchiveFileNameHandler checks
ArchiveSuffixFormat.IndexOf("{0", ...)to setwildCardStrictSeqNo(true = sequence-only)- RollingArchiveFileHandler derives it from filename check (no digits = sequence-based)
- Both pass the appropriate value to
DeleteOldFilesBeforeArchiveScanFileNamesForMaxSequenceNo (line 159, hardcoded true) is a separate method used only for finding max sequence numbers in sequence-based rolling scenarios, not for cleanup. The hardcoded true is intentional for that specific purpose.
Likely an incorrect or invalid review comment.
src/NLog/Targets/FileTarget.cs (2)
823-825: Debug message wording only.“rolling filesize” clarifies trigger; no functional impact.
833-835: Debug message wording only.“rolling filetime” clarifies trigger; no functional impact.
tests/NLog.UnitTests/Targets/FileTargetTests.cs (3)
3608-3610: Refactor to parameterize generation count.Makes the test reusable and faster to tweak.
3612-3616: New weekday-based test (ddd).Covers the new scenario; good addition.
3652-3652: Config uses ArchiveSuffixFormat with {1:...} (date-only) and Day; matches weekday/date suffix use.Looks correct for both yyyy* and ddd.
Also applies to: 3660-3666
|
The archive-cleanup-logic was very strict about excluding old files matching wildcard when suffix part contained letters, now only extra strict when using
archiveSuffixFormat="_{0}".Adding support for: