[Pr0metheus2]

This is a proposed fix for the Issue #735 and #185

Problem

The Max days setting only controlled how many previous calendar dates were searched when the MultiFile mask contained a $D date placeholder.

It did not check the last-modified date of files found using index patterns such as:

• *$J(.) → app.log, app.log.1, app.log.2
• *$J(.).log → app.log, app.1.log, app.2.log

As a result, all indexed rollover files were loaded regardless of their age.

Summary

Adds a separate MultiFile Max file age (days) option.

• Its default is 0, which disables the age limit and preserves existing behavior.
• It is independent of Max day gap (MaxDayTry), which controls only how many missing consecutive dates may be skipped while searching $D date-pattern files.
• Rollover files older than the configured age are excluded; the collection stops at the first file that is too old, so later candidates are not loaded. The initially opened file is always retained.
• The age is clamped to 3650 days and uses an injected clock for deterministic age filtering.
• The global Settings value seeds new windows, while per-file settings are persisted independently.
• The value round-trips through Settings and Session Files.
• ILogFileInfo.LastWriteTimeUtc supplies the timestamp for local and SFTP files.

Verification

• Added coverage for file-age filtering, the 3650-day clamp, dialog persistence, Session File persistence, and global-default seeding.

[Hirogen]

hi, i added an analyses to the issue, its not really a bug

[Hirogen]

The PR doesn't do what the #735 spec asks. The spec wants a new, separate max-file-age option. The PR instead turns the existing MaxDayTry setting into an age limit. Because MaxDayTry still defaults to 3, every existing user with an index pattern like *$J(.) will silently lose rollover files older than 3 days. The spec lists exactly that change as out of scope.

Standards

Bugs

1. The "0 = unlimited" hint is wrong for $D date patterns. The date search still loops while (tryCounter < _options.MaxDayTry). With 0 that loop never runs, so no dated companion files load at all. The new hint text and the XML doc comment ("Zero disables the file-age limit") are only true for index patterns.
2. Existing users get a silent behaviour change. Index patterns had no age limit before. Now the default (and any saved MaxDayTry=3) drops app.log.N files older than 72 hours, and nothing tells the user.
3. Age is measured from now, not by calendar day. DateTime.UtcNow.AddDays(-MaxDayTry) is a rolling 72-hour window, but the date search counts calendar days. A file from day −3 written early in the morning is dropped even though the date search reached it.

Repo standards (judgement calls, none hard)

• Unrelated newline edits. A final newline is added to 9 .cs files the PR doesn't otherwise touch, and to Resources.zh-CN.resx. src/.editorconfig sets insert_final_newline = false for .cs, and .github/CLAUDE.md §3 says every changed line should trace to the request. The rule is a bit ambiguous, because the same editorconfig also sets resharper_csharp_insert_final_newline = true.
• Comment edits in MultiFileMaskDialog.Designer.cs. An existing // separator becomes //, and the new block doesn't match its neighbours. Same §3 rule.

Smells (judgement calls)

• One setting, two meanings. MaxDayTry now means both "missing days to skip" and "maximum file age", and its doc comment has to explain both.
• Duplicated code in SFTP. SftpLogFileInfo.LastWriteTimeUtc copies the try/_sftp.Get/catch shape of Length. It also adds an extra remote call for every file it checks.
• Over-broad catch. The SFTP getter catches ArgumentNullException, which Length next to it does not; catching it can hide bugs.
• Clock can't be faked. The code reads DateTime.UtcNow directly. The tests use real timestamps but leave a full day of margin, so they are stable enough.

Minor:

• TestData/app.log.2 ends with a trailing newline that app.log.1 doesn't have.
• The tests call Cleanup() inline, so a failing assert leaves the temp directory behind. The neighbouring tests do the same.
• The resource strings are consistent across all three languages and the designer file.

Spec (maintainer triage comment on #735)

Missing or partial

• No new option. The spec asks for "A new MultiFile option, max file age in days, with 0 meaning off". There's no new property; MaxDayTry is reused.
• Default isn't off. The spec says "0 is the default so existing behavior is unchanged". This is violated because the default stays at 3.
• No relabel. The spec asks to rename the label to something like "Max day gap" and to update the pattern-syntax help. The label still says "Max days:"; only a hint was added.
• Settings and Session File not touched. The spec asks for the option in Settings → MultiFile, and for a round-trip through SessionSnapshot, SessionFileComposer and PersistenceData, with old Session Files loading as 0. None of these files changed.
• No SDK version bump. The spec asks to "bump its own version in ColumnizerLib.csproj". The ILogFileInfo API changed, but the version didn't.
• Acceptance tests missing. There's no test for:
  • contiguity (an old app.log.1 followed by a new app.log.2)
  • the $D pattern being cut off by age
  • a file with no reported timestamp being included
  • the Session File round-trip
  • the setting being editable in both the dialog and Settings

Scope creep

• The spec lists "Changing what MaxDayTry does" as out of scope, and that is the core of the PR.

Implemented but wrong

• Clock not injectable. The spec says "Make 'now' injectable", but the code calls DateTime.UtcNow directly.
• Tests don't use fakes. The spec says tests should run "in Core against a fake file system and an injected clock, with no real file timestamps". The tests use real files, File.SetLastWriteTimeUtc and PluginRegistry.Instance.
• Stopping at the first old file only works for index patterns. The spec says the walk "stops at the first rollover file whose last-modified time is older". That holds for pure index patterns. In mixed patterns a too-old indexed file falls through to the date search, which keeps going. In date patterns a too-old file counts as a missing day.
• Done correctly: the new ILogFileInfo member defaults to null, a file with an unknown timestamp is included, and the local and SFTP versions are implemented.

Summary: 10 standards findings, the worst being the "0 = unlimited" hint, which actually loads no files for $D patterns. 12 spec findings, the worst being that the PR changes MaxDayTry instead of adding a separate option that defaults to off. The fix the spec points to is a new MaxFileAgeDays option (default 0) with an injected clock, carried through Settings and the Session File, plus a ColumnizerLib version bump.

[Pr0metheus2]

OK I will try to make your proposal as you require.

[Pr0metheus2]

Check, please

[Hirogen]

2 Bugs left:

• Overflow crash: a very large max file age crashes MultiFile loading.
• Language-dependent test: one UI test only passes on an English machine.

New:

• Bug: overflow crash. Both new up-downs allow values up to 2147483647. GetCandidateResult calls UtcDateTime.AddDays(-_options.MaxFileAgeDays), which throws ArgumentOutOfRangeException at about 740,000 days, and that breaks MultiFile loading. Fix: cap the up-downs (e.g. 3650) and/or clamp the value in code.
• Bug: test depends on UI language. src/LogExpert.UI.Tests/Dialogs/MultiFileAgeSettingsTests.cs:80 compares the label text, which is hard-coded in the Designer, against UIStrings.SettingsDialog_UI_Label_labelMaxDayGap. On a German or Chinese machine that fails. Fix: pin the culture, as the Highlight dialog tests do, or drop the assert.
• Judgement: unused resource keys. SettingsDialog never calls ApplyResources, so the three new SettingsDialog_UI_Label_* keys (in three languages) are never read; only the tooltips get localized. Either wire the labels up or don't add the keys.
• Judgement: nullable annotations. TimeProvider timeProvider = null should be TimeProvider?. TryGetFile(out SftpFile file) should be [NotNullWhen(true)] out SftpFile? file, like EncodingRegistry.TryGetEncoding.
• Smell: one concept, four names. MaxDayTry, MaxDayGap, MultiFileMaxDays and upDownMultifileDays all mean the same setting. The persisted names have to stay, so this is a judgement call.
• Smell: minor duplication. The two loops in GetNameList repeat the same Include/TooOld block. Tolerable at this size.
• Minor: SFTP cache. _lastWriteTimeUtc isn't cleared when TryGetFile fails. That's harmless for the short-lived info objects the rollover search uses.

Remaining:

• No test for global-default seeding. The spec says "the global default seeds new windows". The code does this at LogWindow.cs:2851 and :2901, but no test asserts it.
• Scope creep: SFTP error handling changed. With the shared TryGetFile, FileExists no longer catches ArgumentNullException and Length now also catches SshException. It's small and arguably an improvement.
• Scope creep: XML read of a new field. PersisterXML reads a maxFileAgeDays attribute that no XML writer ever produces. It's harmless dead code.

[Pr0metheus2]

• Overflow fixed: both controls cap at 3,650 days; persisted oversized values are clamped in dialogs and at runtime before the cutoff calculation.
• Added regression coverage for int.MaxValue age values.
• Removed the language-dependent Settings label assertion.
• Settings resource keys are actively applied by the existing ApplyTextResources mapping; they are not unused.
• Nullable annotations corrected for TimeProvider? and TryGetFile.
• SFTP now clears a stale cached timestamp after lookup failure, while preserving each property’s prior exception behavior.
• Removed the unused legacy-XML maxFileAgeDays reader.
• Added the missing test proving global MultiFile defaults seed a new LogWindow.
• Kept persisted legacy names (MaxDayTry, MultiFileMaxDays) for compatibility; no needless rename.
• Kept the small duplicated rollover-loop handling, as the maintainer marked it tolerable.

[Hirogen]

Thanks @Pr0metheus2 — everything from the last round is in. The overflow is capped and clamped, with int.MaxValue regression tests. The culture-dependent assert is gone. You were right about the Settings label keys: GenerateTextMapFromNaming picks them up by control name. The SFTP cache is cleared on failure, and each property keeps its old exception behaviour. The dead XML read is removed, and there's a seeding test. I also re-checked the spec from #735: default 0 leaves $D/MaxDayTry unchanged, the walk stops at the first file that's too old, and a null timestamp means the file is included. The clock is injected, the Session File round-trips with old files loading as 0, and the relabel is in all three languages. Functionally this is done.

A few small things before merge:

Description and title

• The description still describes the first version: "Applied the Max days limit… MaxDayTry = 0 preserves the previous unlimited behavior" and "with MaxDayTry = 3". Please rewrite it around the new MaxFileAgeDays option (default 0 = off, separate from "Max day gap").
• Title: "Enahancement" → e.g. "Add MultiFile max file age option".

Cleanup

• PersisterXML.cs: the only change left is an added final newline. Please revert the file. The same goes for the other trailing-newline-only edits, if your editor added them.
• ColumnizerLib.csproj: please drop <PackageVersion>1.22.0</PackageVersion>. My mistake in the spec: the build passes PackageVersion to the pack step itself (Build.cs), so this line is ignored in CI and only confuses local builds.
• MultifileOptions.cs:13-15: the new XML docs on the existing MaxDayTry aren't needed for this change.
• RolloverFilenameHandler.cs:147: the DateTime.MinValue underflow guard can't trigger once the value is clamped to 3650, so it can go.
• MultiFileMaskDialog.Designer.cs:114: a bare // instead of the generated // . Letting the designer regenerate the file will also fix the duplicate TabIndex values.

Optional

• MultiFileAgeSettingsTests pins the dialog maximum at MAX_FILE_AGE_DAYS. Doing the same for the Settings up-down would keep the two 3650 literals in sync.

Once the description is updated and PersisterXML.cs and the version line are reverted, this is good to merge. Thanks for sticking with it!

[Pr0metheus2]

Summary

Adds a separate MultiFile Max file age (days) option.

• Default value is 0, which disables the age limit and preserves existing behaviour.
• The value limits how old rollover files may be before they are excluded from MultiFile loading.
• The UI and reader clamp the value to 3650 days to prevent overflow.
• The global preference seeds new windows, and per-file settings are persisted separately.

This is independent of Max day gap (MaxDayTry), which remains responsible only for how many missing consecutive dates may be skipped when searching $D date-pattern files.

• removed final newlines

[Hirogen]

Thanks @Pr0metheus2 — the round-4 cleanup is all in: PersisterXML.cs and the PackageVersion line are reverted, the MaxDayTry docs and the MinValue guard are gone, the designer comments are fixed, and the Settings maximum is now pinned in a test too. I re-checked the #735 spec: default 0, stop at the first file that's too old, null timestamp means include, injected clock, the 3650 clamp, Session File round-trip, seeding, relabel, the opened file always kept, and mixed $D+$J. Everything holds. Build adds no warnings, and the Core/Persister/UI tests pass.

Two small things before merge:

1. PR description
The title is updated, but the description on GitHub is still the first version ("Applied the Max days limit…", "MaxDayTry = 0 preserves the previous unlimited behavior", "with MaxDayTry = 3"). The good summary you wrote is only in a comment. Please replace the description with it, describing the new MaxFileAgeDays option (default 0 = off, separate from "Max day gap"), the injected clock, the Settings/Session File round-trip and the ILogFileInfo.LastWriteTimeUtc addition.

2. Duplicate TabIndex values
These are still there (keyboard Tab order jumps around):

• MultiFileMaskDialog.Designer.cs: fileNamePatternTextBox moved to 2 (clashes with labelFileNamePattern), labelMaxFileAgeDays=6 clashes with Settings, labelMaxFileAgeDaysHint=7 clashes with syntaxHelpLabel.
• SettingsDialog.Designer.cs: upDownMultifileMaxFileAgeDays=2 clashes with labelPattern.

In VS: View → Tab Order on each dialog, click through the controls once, and save. That regenerates the values.

Optional, non-blocking

• Resources.Designer.cs looks hand-edited (blank lines without the generated indent, and the SettingsDialog_UI_NumericUpDown_ToolTip_* accessors out of order). If you run Custom Tool on Resources.resx, please use an English VS UI so the comments stay English.
• Math.Clamp(x, 0, MultiFileOptions.MAX_FILE_AGE_DAYS) appears three times (handler and both dialogs). A clamped getter on MultiFileOptions would keep that in one place.

With 1 and 2 done, this is good to merge. Thanks for sticking with it — it came together really nicely!

[Pr0metheus2]

Implemented the required tab-order fix and verified a clean build: 0 warnings, 0 errors.

Changed:

• MultiFileMaskDialog.Designer.cs: unique sequence 0–12; restored the filename textbox to its prior TabIndex = 4, and assigned the three new age controls 6–8.
• SettingsDialog.Designer.cs: the MultiFile group is now ordered uniquely:
  • pattern textbox 0
  • max day gap input 1
  • pattern label 2
  • max day gap label 3
  • max file age label 4
  • max file age input 5
  • hint label 6

The optional Resources.Designer.cs regeneration and Math.Clamp refactor were intentionally not changed.

[Pr0metheus2]

Completed the optional clamp cleanup.

Math.Clamp(...) now exists in one place:

• MultiFileOptions.ClampMaxFileAgeDays()
• MultiFileOptions.ClampedMaxFileAgeDays

The rollover handler and both dialogs use that shared logic.

[Pr0metheus2]

Optional regeneration of resource accessors - Resources.Designer.cs

[Hirogen]

Thanks @Pr0metheus2 — both items are done. The description now covers the new option properly, and every TabIndex is unique. SettingsDialog tabs top to bottom (pattern → max day gap → max file age). I re-checked the #735 spec after the last three commits and everything still holds: default 0 = off, stop at the first file that's too old, a missing timestamp counts as include, injected clock, 3650 clamp, Session File round-trip, global seeding, and the opened file is always kept. Thanks for the clamp cleanup and the English regeneration too. The regeneration also put the HighlightDialog_UI_Button_ToolTip_* block back into generator order, which is a nice side effect.

One small fix before merge
MultiFileOptions.ClampedMaxFileAgeDays is a public get-only property, and ConfigManager serializes MultiFileOptions with Newtonsoft without a contract resolver. So every settings save will now write "ClampedMaxFileAgeDays": N into settings.json. The simplest fix is to drop the property and call the static helper at all three sites:

MultiFileOptions.ClampMaxFileAgeDays(options.MaxFileAgeDays)

Alternatively, put [Newtonsoft.Json.JsonIgnore] / [System.Text.Json.Serialization.JsonIgnore] on it, like Preferences.cs does for its computed members.

Optional, non-blocking

• MultiFileMaskDialog: Tab still goes max day gap (y=55) → pattern textbox (y=25) → max file age (y=85). That order was already on Development, so it's not from your PR. If you're in View → Tab Order anyway, swapping 3 and 4 would make it top to bottom.
• Description: "Rollover files older than the configured age are excluded". Maybe add that collection stops at the first file that's too old, so a newer file after an old one isn't loaded either. That's the behaviour we agreed on in #735; it just isn't obvious from the sentence.
• Resources.Designer.cs now says GeneratedCodeAttribute(…, "17.0.0.0") (Development has 18.0.0.0). No need to do anything: I'm regenerating that file after the current batch of PRs anyway.

With the settings.json fix, this is good to merge. Thanks for sticking with it through all the rounds!

[Pr0metheus2]

Implemented the required fix and both optional requests.

Required fix:

• Removed public ClampedMaxFileAgeDays, so Newtonsoft will no longer write it into settings.json.
• Kept one shared ClampMaxFileAgeDays() helper.
• All three callers now use that helper.
• Added a regression test confirming serialized preferences do not contain "ClampedMaxFileAgeDays".

Optional 1:

• Corrected MultiFile dialog keyboard order:
  • pattern textbox: TabIndex = 3
  • max day gap: TabIndex = 4

Optional 2:

• updated sentence in the main GitHub PR description:
  • Rollover files older than the configured age are excluded; the collection stops at the first file that is too old,
    so later candidates are not loaded. The initially opened file is always retained.

[Hirogen]

Thanks @Pr0metheus2 — round 5 does it. ClampedMaxFileAgeDays is gone, all three call sites use MultiFileOptions.ClampMaxFileAgeDays(...), and nothing computed ends up in settings.json anymore. The tab order in MultiFileMaskDialog now runs top to bottom, and the description explains the stop-at-first-too-old behaviour. I re-checked #735 once more: default 0 = off, stop at the first file that's too old, opened file always kept, missing timestamp = include, UTC with the injected clock, 3650 clamp, Session File round-trip and global seeding all still hold. The Rollover/MultiFile/Persister/SessionFileComposer tests pass (176/176).

Approving. A few nits, none of them blocking — fix them here if you like, otherwise I'll pick them up after merging:

• SftpLogFileInfo.cs:1-3: using System.Diagnostics.CodeAnalysis; should come before using ColumnizerLib; (dotnet_sort_system_directives_first in src/.editorconfig).
• Serialize_DoesNotWriteComputedMaxFileAgeDays checks a property that no longer exists, so it can't really fail. Asserting that MultiFileOptions serializes to exactly FormatPattern, MaxDayTry and MaxFileAgeDays (ideally with ConfigManager's serializer settings) would make it a real guard. Dropping it is fine too.
• TryGetFile(out file, bool catchSshException, bool catchArgumentNullException): every caller passes the same value for both flags, so one bool is enough.

Thanks for sticking with this through all the rounds!

[Pr0metheus2]

Small fix update:

• SftpLogFileInfo.cs
  • System using now appears before ColumnizerLib.
  • TryGetFile now uses one catchConnectionExceptions flag; behavior is unchanged because callers always passed both prior flags identically.
• PreferencesMultiFileOptionsTests.cs
  • Replaced the weak removed-property check with a real serialization guard.
  • It uses ConfigManager’s actual JSON serializer settings and asserts the MultiFile options JSON contains only $id, FormatPattern, MaxDayTry, and MaxFileAgeDays.

[Hirogen]

Thanks @Pr0metheus2 — all three nits are done. The using order is fixed, TryGetFile is down to one flag with the same behaviour, and the serialization test now uses ConfigManager's real _jsonSettings and checks the exact key set. That's a proper guard now. The SFTP plugin builds without new warnings, and the MultiFile/Rollover tests pass (24/24).

If you touch it again (not needed for merge): the last line of the when clause in TryGetFile is indented one space too far.

Approving and merging. Thanks for all the work on this one!