fix: bound the log file and stop the Volume Control tab flooding it - #1722
Open
laurentiu021 wants to merge 1 commit into
Open
fix: bound the log file and stop the Volume Control tab flooding it#1722laurentiu021 wants to merge 1 commit into
laurentiu021 wants to merge 1 commit into
Conversation
Closes #1645. The sink passed no fileSizeLimitBytes and no rollOnFileSizeLimit, so it took Serilog's defaults — 1 GB per file with rolling OFF — and the only bound on the folder was the 14-FILE count. Measured against the real sink rather than reasoning from the docs: 4000 lines of ~4 KB produced ONE 15 MB file with no roll. With the limit and rollOnFileSizeLimit it produces two files, the largest exactly 10 MB. That matters because the documented support path is "attach the log" (SUPPORT.md and the bug-report template), so an unattachable file breaks the evidence trail exactly when it is needed — and Debug is a real volume tier here, 290 Log.Debug call sites, some in loops. 10 MB x 14 files is a predictable ~140 MB worst case that also respects the low-end laptop this app targets. The issue placed the fix on a plain WriteTo.File call; it is actually inside the custom UserPathScrubbingSink's inner logger, which is where it went. MaxLogFileBytes and RetainedFileCount are now named constants, so Init and the sink cannot drift apart and the tests assert the same values the app uses. Also stopped the flood at source. AudioMixerService.GetPeak logged every COMException at Debug, and it is driven by the Volume Control tab's 50 ms peak-meter timer — one bad audio session wrote ~20 identical lines a second for as long as the tab stayed open, which both fills the file and pushes out everything worth reading. Now logged once per session-handle generation, mirroring the existing _routingProbed one-shot pattern, and reset in ReleaseGroups so a genuinely new failure after a device change is still reported. Tests: three added to the existing sink suite, which already drives the real private sink type by reflection into a temp file. Two pin the intent (small enough to attach to an issue at GitHub's 25 MB ceiling; whole folder bounded) and one drives the sink hard and asserts no produced file exceeds the limit — asserting the constants alone would have passed even with the arguments still missing, which was the actual defect. Not covered by a test: the GetPeak guard needs real COM audio sessions failing, which a unit test cannot produce. Stated rather than implied. All four projects rebuild with 0 warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1645.
The problem, measured
The sink passed no
fileSizeLimitBytesand norollOnFileSizeLimit, so it took Serilog's defaults — 1 GB per file with rolling off — and the only bound on the folder was the 14-file count.I drove the real sink rather than reasoning from the docs. 4000 lines of ~4 KB each:
That matters because the documented support path is "attach the log" (
SUPPORT.md, the bug-report template) — an unattachable file breaks the evidence trail exactly when it's needed. And Debug is a real volume tier here: 290Log.Debugcall sites, some in loops.The fix
fileSizeLimitBytes: MaxLogFileBytes+rollOnFileSizeLimit: true, keeping the 14-file retention. 10 MB × 14 is a predictable ~140 MB worst case that also respects the low-end laptop this app targets.One correction to the issue: it placed the fix on a plain
WriteTo.Filecall, but it's actually inside the customUserPathScrubbingSink's inner logger — that's where it went.MaxLogFileBytesandRetainedFileCountare now named constants soInitand the sink can't drift apart, and the tests assert the same values the app uses.The flood, stopped at source
AudioMixerService.GetPeaklogged everyCOMExceptionat Debug, and it's driven by the Volume Control tab's 50 ms peak-meter timer — one bad audio session wrote ~20 identical lines a second for as long as the tab stayed open. That both fills the file and pushes out everything worth reading.Now logged once per session-handle generation, mirroring the existing
_routingProbedone-shot pattern, and reset inReleaseGroupsso a genuinely new failure after a device change is still reported.Tests
Three added to the existing sink suite, which already drives the real private sink type by reflection into a temp directory:
TheSizeLimit_IsSmallEnoughToAttachToABugReport— pins the intent against GitHub's 25 MB attachment ceiling, not just the number.TheWholeLogFolder_IsBounded— per-file ceiling × retained count, the predictable worst case the file-count-alone version never gave.TheSink_KeepsEveryFileUnderTheLimit— drives the sink hard and asserts no produced file exceeds the limit. Asserting the constants alone would have passed even with the arguments still missing, which was the actual defect. UsesRollingInterval.Infiniteso only the size limit can create a second file — with a daily interval a date change could, and the test would pass for the wrong reason.Not covered by a test: the
GetPeakguard needs real COM audio sessions failing, which a unit test can't produce. Saying so rather than implying coverage.All four projects rebuild
--no-incrementalwith 0 errors, 0 warnings.