Skip to content

Conversation

@snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Aug 30, 2025

Resolves #5976

@snakefoot snakefoot added bug Bug report / Bug fix file-target file-archiving Issues with archiving with the file target labels Aug 30, 2025
@coderabbitai
Copy link

coderabbitai bot commented Aug 30, 2025

Walkthrough

Refines 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

Cohort / File(s) Summary
Archive wildcard selection logic
src/NLog/Targets/FileArchiveHandlers/BaseFileArchiveHandler.cs
Adds hasDigit tracking in GetDeleteOldFileNameWildcard. Only selects longest non-letter run if it contains a digit; updates run/termination and reset behavior. Removes previous post-processing trim. No signature changes.
Unit tests update
tests/NLog.UnitTests/Targets/FileTargetTests.cs
Changes test base filename from Log{0}.txt to Log-file{0}.txt and updates expected archive names; test logic unchanged otherwise.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Assessment against linked issues

Objective Addressed Explanation
Ensure maxArchiveFiles enforcement for filenames with multiple dots/extensions (e.g., nlog-all.ecs.json) by fixing wildcard/matching logic so archives are correctly recognized and old files deleted [#5976]

Poem

I nibble through names in the archive glade,
Hunting digits where wild runs are made.
If numbers hide, I'll leap along past—
Only runs with figures become my cast.
Clean logs sleep safe, my work is fast. 🐇✨


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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.

📥 Commits

Reviewing files that changed from the base of the PR and between adf3f33 and 13f4b46.

📒 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 digits

Tracking 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 run

Updating 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 rule

The post-loop comparison correctly preserves only digit-containing segments.

tests/NLog.UnitTests/Targets/FileTargetTests.cs (1)

3250-3250: Test update aligns with new wildcard selection

Switching 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.

@snakefoot snakefoot force-pushed the filetarget_cleanup_wildcard branch from 13f4b46 to c9f98ef Compare August 30, 2025 12:42
@snakefoot snakefoot force-pushed the filetarget_cleanup_wildcard branch 2 times, most recently from e711eae to ef8131d Compare August 30, 2025 12:46
@sonarqubecloud
Copy link

@snakefoot
Copy link
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Aug 30, 2025

✅ Actions performed

Full review triggered.

@snakefoot
Copy link
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Aug 30, 2025

✅ Actions performed

Full review triggered.

@snakefoot snakefoot merged commit b39294f into NLog:dev Aug 30, 2025
6 checks passed
Copy link

@coderabbitai coderabbitai bot left a 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.

📥 Commits

Reviewing files that changed from the base of the PR and between adf3f33 and ef8131d.

📒 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.

This was referenced Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug report / Bug fix file-archiving Issues with archiving with the file target file-target size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NLog v6 MaxArchiveFiles not functioning as expected

1 participant