-
Notifications
You must be signed in to change notification settings - Fork 1.4k
FileTarget - Improve file-wildcard for archive cleanup #5979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughRefines archive filename wildcard selection to consider the longest non-letter segment only if that segment contains at least one digit. Adds a hasDigit flag in GetDeleteOldFileNameWildcard and adjusts run/selection/reset logic. Updates a unit test filename pattern; no public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant FT as FileTarget
participant AH as BaseFileArchiveHandler
participant FS as FileSystem
App->>FT: write log entry
FT->>AH: trigger archive cleanup
AH->>AH: GetDeleteOldFileNameWildcard(filename)\n- scan chars\n- track non-letter runs\n- set hasDigit=true if run has digits\n- select longest run only if hasDigit
AH->>FS: enumerate/delete files matching wildcard
FS-->>AH: matching files list
AH-->>FT: cleanup complete
FT-->>App: continue write
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
317-321: Optionally trim to the first digit within the chosen segment (handles multiple leading separators)Today only one leading non-digit is trimmed. If the segment starts with multiple separators (e.g., "__-001"), consider trimming all leading non-digits so the wildcard anchors exactly at the numeric portion.
Apply this diff inside the same block to advance to the first digit:
- if (lastStart > 0 && lastLength > 1 && !char.IsDigit(filename[lastStart + 1])) - { - lastStart += 1; - lastLength -= 1; - } + if (lastStart > 0 && lastLength > 0) + { + int advance = 0; + while (advance < lastLength && !char.IsDigit(filename[lastStart + advance])) + advance++; + if (advance > 0 && advance < lastLength) + { + lastStart += advance; + lastLength -= advance; + } + }
📜 Review details
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.
📒 Files selected for processing (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(2 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(1 hunks)
🔇 Additional comments (4)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (3)
284-293: Good improvement: only consider non-letter runs that include digitsTracking hasDigit within the active non-letter run and ignoring digit-free segments reduces false-positive wildcard matches during cleanup. The implementation is straightforward and efficient.
296-305: Correct gating when closing a runUpdating lastStart/lastLength only when hasDigit is true prevents anchors like "-" from driving the wildcard. Resetting hasDigit with currentLength is correct.
309-313: Final run check mirrors the new ruleThe post-loop comparison correctly preserves only digit-containing segments.
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
3250-3250: Test update aligns with new wildcard selectionSwitching to "Log-file{0}.txt" ensures the cleanup wildcard anchors on the digit-containing segment, matching the intended behavior. Assertions below still validate both current and archived file cases.
13f4b46 to
c9f98ef
Compare
e711eae to
ef8131d
Compare
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (2)
284-296: Include preceding non-letter chars in the same run (optional).Currently the run starts at the first digit, excluding leading separators like '_' that belong to the same contiguous non-letter run. Broadening the run improves robustness without changing behavior.
Apply:
- bool hasDigit = false; + bool hasDigit = false; for (int i = 0; i < filename.Length; ++i) { if (!char.IsLetter(filename[i])) { - hasDigit = hasDigit || char.IsDigit(filename[i]); - if (hasDigit) - { - if (currentLength == 0) - currentStart = i; - ++currentLength; - } + if (currentLength == 0) + currentStart = i; + ++currentLength; + if (char.IsDigit(filename[i])) + hasDigit = true; } else { if (currentLength != 0) { - if (lastLength <= currentLength) + if (hasDigit && lastLength <= currentLength) { lastStart = currentStart; lastLength = currentLength; } currentLength = 0; } - hasDigit = false; + hasDigit = false; } } - if (lastLength < currentLength) + if (hasDigit && lastLength < currentLength) { lastStart = currentStart; lastLength = currentLength; }Also applies to: 308-316
52-60: Plumb “contains sequence” to sorting (optional).When the wildcard stems from a numeric segment, CleanupFiles can sort by sequence for more deterministic trimming. Consider returning a flag from GetDeleteOldFileNameWildcard and passing it to wildCardContainsSeqNo.
Proposed minimal internal change:
- protected bool DeleteOldFilesBeforeArchive(string filePath, bool initialFileOpen, string? excludeFileName = null) + protected bool DeleteOldFilesBeforeArchive(string filePath, bool initialFileOpen, string? excludeFileName = null) { // Get all files matching the filename, order by timestamp, and when same timestamp then order by filename // - First start with removing the oldest files string fileDirectory = Path.GetDirectoryName(filePath); // Replace all non-letter with '*' replace all '**' with single '*' - string fileWildcard = GetDeleteOldFileNameWildcard(filePath); - return DeleteOldFilesBeforeArchive(fileDirectory, fileWildcard, initialFileOpen, excludeFileName); + bool wildcardHasDigits; + string fileWildcard = GetDeleteOldFileNameWildcard(filePath, out wildcardHasDigits); + return DeleteOldFilesBeforeArchive(fileDirectory, fileWildcard, initialFileOpen, excludeFileName, wildcardHasDigits); }- private static string GetDeleteOldFileNameWildcard(string filepath) + private static string GetDeleteOldFileNameWildcard(string filepath, out bool containsDigitsSegment) { var filename = Path.GetFileNameWithoutExtension(filepath) ?? string.Empty; var fileext = Path.GetExtension(filepath) ?? string.Empty; if (string.IsNullOrEmpty(filename) && string.IsNullOrEmpty(fileext)) - return string.Empty; + { + containsDigitsSegment = false; + return string.Empty; + } ... - if (lastLength > 0) + containsDigitsSegment = lastLength > 0; + if (lastLength > 0) ... return string.Concat(filename, "*", fileext); }Please run the affected unit tests that rely on DeleteOldFilesBeforeArchive ordering to confirm no regressions.
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
3247-3296: Add a regression test for multi-dot filenames with pre-extension numeric suffix (optional).To lock the fix for issue #5976, add an explicit test with "nlog-all.ecs.json" and ArchiveSuffixFormat="_{0:000}".
Suggested addition:
@@ public void FileTarget_SameDirectory_MaxArchiveFiles(int maxArchiveFiles) { @@ } + + [Fact] + public void MaxArchiveFiles_MultiDotFile_PreExtSuffix() + { + var tempDir = ArchiveFileNameHelper.GenerateTempPath(); + var baseName = Path.Combine(tempDir, "nlog-all.ecs.json"); + try + { + var fileTarget = new FileTarget + { + FileName = baseName, + ArchiveAboveSize = 100, + LineEnding = LineEndingMode.LF, + Layout = "${message}", + MaxArchiveFiles = 3, + Encoding = Encoding.ASCII, + ArchiveSuffixFormat = "_{0:000}", + }; + + LogManager.Setup().LoadConfiguration(c => c.ForLogger().WriteTo(fileTarget)); + Generate100BytesLog('a'); // -> _000 + Generate100BytesLog('b'); // -> _001 + Generate100BytesLog('c'); // -> _002 + Generate100BytesLog('d'); // -> _003 (current becomes ddd) + LogManager.Configuration = null; + + var times = 25; + AssertFileContents(Path.Combine(tempDir, "nlog-all.ecs_003.json"), StringRepeat(times, "ddd\n"), Encoding.ASCII); + Assert.True(File.Exists(Path.Combine(tempDir, "nlog-all.ecs_001.json"))); + Assert.True(File.Exists(Path.Combine(tempDir, "nlog-all.ecs_002.json"))); + Assert.False(File.Exists(Path.Combine(tempDir, "nlog-all.ecs_000.json"))); // trimmed + Assert.False(File.Exists(Path.Combine(tempDir, "nlog-all.ecs_004.json"))); // not created + } + finally + { + if (Directory.Exists(tempDir)) + Directory.Delete(tempDir, true); + } + }
📜 Review details
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.
📒 Files selected for processing (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs(2 hunks)tests/NLog.UnitTests/Targets/FileTargetTests.cs(1 hunks)
🔇 Additional comments (2)
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs (1)
284-296: Fix: wildcard now anchors on digit-bearing non-letter runs (addresses multi-dot + pre-extension suffix).This change makes cleanup target the intended archives like nlog-all.ecs_008.json instead of skipping due to letter segments. Looks good.
Also applies to: 308-309
tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)
3250-3251: Test pattern tweak is sensible.Using "Log-file{0}.txt" covers a non-letter segment before the numeric suffix and aligns with the new wildcard logic.



Resolves #5976