Repository navigation
Fix log buffering race by removing the rule selector cache - #7803
Merged
evgenyfedorov2 merged 6 commits intoOct 5, 2026
Merged
Conversation
…Cache LogBufferingFilterRuleSelector is a singleton shared by all global and per-request log buffers. Its cache stored pooled List<T> instances that InvalidateCache() returned to the pool (clearing them) while concurrent Select() calls could still be enumerating them. Under load this threw "Collection was modified; enumeration operation may not execute", NullReferenceException, or silently produced wrong buffering decisions. Cache immutable arrays instead, so InvalidateCache() only needs to clear the dictionary and readers can safely finish enumerating evicted values. Fixes #7383 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the timing-based Parallel.For test with a single-threaded test that invalidates the cache while Select() iterates over the cached rules, and assert explicitly that no exception is thrown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1623526&view=codecoverage-tab |
The cache was cleared at the end of every request, so it rarely produced hits, while it allocated on every Select() call, contended under concurrent load, and ignored the logger category in its key. Select() is now a static, single pass over the category's rules array: no shared state to race on, nothing to invalidate, and no allocations. InvalidateCache(), the selector's DI registration and constructor parameters, and the Dispose methods that only invalidated the cache are removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
evgenyfedorov2
changed the title
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The shared mutable state is fully removed, callers are consistently updated, and targeted regression coverage verifies category isolation.
Review effort: Balanced
Findings: None
What changed in this PR
Removes the shared rule-selector cache to eliminate log-buffering races and incorrect cross-category rule reuse.
Changes:
- Replaces cached rule selection with stateless array iteration.
- Removes obsolete selector injection, invalidation, and disposal paths.
- Adds regression coverage for category-specific rules.
| File | Description |
|---|---|
test/Libraries/Microsoft.Extensions.Telemetry.Tests/Buffering/LogBufferingFilterRuleTests.cs |
Updates selector calls and adds regression coverage. |
src/Libraries/Microsoft.Extensions.Telemetry/Buffering/LogBufferingFilterRuleSelector.cs |
Makes rule selection static and cache-free. |
src/Libraries/Microsoft.Extensions.Telemetry/Buffering/GlobalLogBufferManager.cs |
Removes selector dependency. |
src/Libraries/Microsoft.Extensions.Telemetry/Buffering/GlobalBufferLoggingBuilderExtensions.cs |
Removes obsolete DI registration. |
src/Libraries/Microsoft.Extensions.Telemetry/Buffering/GlobalBuffer.cs |
Uses stateless selection and removes invalidation. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/PerRequestLogBufferManager.cs |
Removes selector propagation. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/IncomingRequestLogBufferHolder.cs |
Removes cache-only disposal behavior. |
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/IncomingRequestLogBuffer.cs |
Uses static selection and drops cache-only disposal. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1623814&view=codecoverage-tab |
dariusclay
approved these changes
Oct 5, 2026
This was referenced Oct 5, 2026
Closed
5 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
LogBufferingFilterRuleSelectoris a singleton shared by every global and per-request log buffer. Under concurrent load, per-request log buffering throwsInvalidOperationException: Collection was modified; enumeration operation may not executefromSelect(), which makes the feature unusable in production.Fixes: #7383
Root cause
The selector cached rule candidates per
(LogLevel, EventId)in pooledList<LogBufferingFilterRule>instances.InvalidateCache()returned those lists to the pool, which clears them, while concurrentSelect()calls could still be iterating them.InvalidateCache()runs every time a request ends: the scopedIncomingRequestLogBufferHolderdisposes each per-categoryIncomingRequestLogBuffer, and each of them invalidated the shared cache. Measured withTestServer, the cache was cleared once per request per category, so it rarely helped:Even a cache hit allocated a closure (56 B per log call), and the cache key ignored the logger category, so one category could get candidates computed from another category's rules.
Fix
Remove the cache.
Select()is now a static, single pass over the category's rules (theLogBufferingFilterRule[]each buffer already computes), with the same matching order and result as before. With no shared state there is nothing to race on, nothing to invalidate, and no per-call allocation.InvalidateCache(), the selector's DI registration and constructor parameters, and theDisposemethods that only existed to invalidate the cache are removed.Benchmarks (BenchmarkDotNet MediumRun, .NET 10, Apple M3 Max), per log call:
Alternatives that keep a cache were measured as well. With 8 threads sharing one selector, an immutable-array cache and a pooled-list cache behind a
ReaderWriterLockSlimcost 80-280 ns per call from contention on cache clears, versus 1-8 ns without a cache. The only scenario where a cache won was a warm cache with 10 rules (7.4 ns vs 15.5 ns); with request-end invalidation it lost again (29.3 ns vs 14.6 ns). Taking an array instead ofIList<T>matters: theIList<T>version took 51 ns in the 10-rule request scenario.Testing
SelectsRightRule_WhenCategoriesHaveDifferentRules: the same scenario returned the other category's rule with the cached implementation.Microsoft.Extensions.Telemetry.Tests(341 tests) andMicrosoft.AspNetCore.Diagnostics.Middleware.Tests(188 tests) pass on net9.0 and net10.0.Microsoft Reviewers: Open in CodeFlow