fix(extractor): exempt EventSource-owned DiagnosticCounter fields from the field-disposable leak (mined Npgsql)#89
Merged
Conversation
…m the field-disposable leak (mined Npgsql) An EventSource's diagnostic counters — EventCounter / PollingCounter / IncrementingEventCounter / IncrementingPollingCounter — are constructed with `this` (the parent EventSource) as their owner argument. That registration hands the counter to the source: the runtime's CounterGroup pins it to the EventSource's lifetime, and an EventSource is a process-lived `static readonly Log` singleton, so a counter is a process-lived diagnostic that is idiomatically NEVER field-disposed (every BCL EventSource — RuntimeEventSource, the HTTP/ASP.NET counters — does the same). Reporting such a field as an undisposed IDisposable was a false positive. Mined: Npgsql's NpgsqlEventSource builds eight counters in OnEventCommand, none field-disposed — eight OWN001 FPs cleared. Scope (narrow, sound): the field is skipped only when the containing type derives from System.Diagnostics.Tracing.EventSource AND the field is assigned `new <DiagnosticCounter subclass>(... this ...)`. A counter is always built in a method body (OnEventCommand) — `this` is unavailable in a field initializer — so the assignment shape (matching the `constructed` set) is the only one that can match. Regression sample EventSourceCountersSample.cs: SampleEventSource's four counters stay SILENT; the Codex control `_scratch` — a plain owned IDisposable in the SAME EventSource — STILL raises the OWN001 disposable-field leak, proving the exemption keys off the DiagnosticCounter type handed to `this`, not the EventSource class. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a false-positive exemption to the Roslyn disposable-field leak detector: ChangesEventSource DiagnosticCounter exemption
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72b6927bc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| // an EventSource's own DiagnosticCounter (registered to `this`) is process-lived | ||
| // and never field-disposed -> not an owned leak (mined: Npgsql NpgsqlEventSource). | ||
| if (eventSourceCounters.Contains(v.Identifier.Text)) |
There was a problem hiding this comment.
When an EventSource has an owned IDisposable field that is also ever assigned a counter, this name-only skip suppresses the whole field instead of just the counter construction. For example, an IDisposable _resource = new MemoryStream(); later assigned _resource = new EventCounter(..., this) would now produce no disposable-field fact at all, hiding the non-counter leak; the exemption should bind the assignment to this field and/or require the declared field to be a DiagnosticCounter-only field before continuing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2197-2202: The FieldName method used in the assignment loop
matches any field regardless of the receiver instance, which incorrectly
populates eventSourceCounters with fields from other-instance assignments like
other._counter. Replace the FieldName(a.Left) call with ThisFieldName(a.Left) to
ensure only current-instance field assignments (_field or this._field) are added
to eventSourceCounters. Apply this same fix to the similar code pattern at lines
2222-2225 that also uses FieldName to prevent incorrectly suppressing fields
that should not be exempted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 786c438d-d266-473f-8d6d-f64fdd18d077
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/EventSourceCountersSample.cs
…nter fields (Codex P2 + CodeRabbit) Two review findings on the over-broad first cut, both the over-suppression class: - Codex P2 — require the field's DECLARED type to derive from DiagnosticCounter before skipping. A field declared as a plain IDisposable that is merely assigned a counter once (`IDisposable _mixed = new MemoryStream(); _mixed = new EventCounter(..., this);`) would otherwise have its OTHER resource (the MemoryStream) silently suppressed by the name-only skip. The declared-type guard binds the exemption to actual counter fields (Npgsql declares them as `IncrementingPollingCounter?` etc.), so the non-counter leak still fires. - CodeRabbit (major) — populate the exemption set with ThisFieldName, not FieldName: the latter also matches `other._counter`, which could exempt this class's same-named field from another instance's assignment. Tightening a suppression to THIS instance's field (`_f` / `this._f`) is strictly safer. Regression: EventSourceCountersSample gains `_mixed` — declared IDisposable, initialized `new MemoryStream()` and later assigned `new EventCounter(..., this)` — which must STILL raise OWN001 on the MemoryStream. The four genuine counter fields stay SILENT; `_scratch` (non-counter) and `_mixed` (non-counter-declared) both warn. CI asserts all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| // Codex control: a NON-counter owned IDisposable field in the same EventSource is NOT exempt. | ||
| // The exemption keys off the DiagnosticCounter type handed to `this`, not the containing class, | ||
| // so this new'd-but-never-disposed resource STILL raises OWN001 ('disposable field'). | ||
| private readonly OwnedScratch _scratch = new OwnedScratch(); |
| // must STILL leak. The exemption requires the DECLARED field type to be a DiagnosticCounter, so | ||
| // the `new MemoryStream()` owned here is not hidden by the later `_mixed = new EventCounter(...)` | ||
| // (a name-only skip would wrongly suppress the whole field). -> OWN001 on the MemoryStream. | ||
| private IDisposable _mixed = new MemoryStream(); |
PhysShell
pushed a commit
that referenced
this pull request
Jun 23, 2026
#90 CTS-alias) for the npgsql re-mine
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
3 participants
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.
What
An
EventSource's diagnostic counters —EventCounter/PollingCounter/IncrementingEventCounter/IncrementingPollingCounter— are constructed withthis(the parentEventSource) as their owner argument:That registration hands the counter to the source: the runtime's
CounterGrouppins the counter to theEventSource's lifetime, and anEventSourceis the canonical process-livedstatic readonly Logsingleton. So a counter is a process-lived diagnostic that is idiomatically never field-disposed — every BCLEventSource(RuntimeEventSource, the HTTP/ASP.NET counters) does exactly the same. Reporting such a field as an undisposedIDisposablewas a false positive.Mined
Npgsql'sNpgsqlEventSourcebuilds eight counters inOnEventCommand, none field-disposed → eight OWN001 FPs cleared. (This is the first of the Npgsql-mined fixes.)How (narrow + sound)
A field is skipped only when both hold:
System.Diagnostics.Tracing.EventSource, andnew <DiagnosticCounter-subclass>(… this …).A counter is always built in a method body (
OnEventCommand) —thisis unavailable in a field initializer — so the assignment shape (the same one theconstructedset already recognizes) is the only one that can match. Type checks are semantic (walk the base chain toDiagnosticCounter/EventSource); the BCL types resolve on the Linux runner.Regression sample (
EventSourceCountersSample.cs)SampleEventSource's four counters (one of each kind) → SILENT._scratch— a plain ownedIDisposablein the sameEventSource— STILL raises the OWN001 disposable-field leak, proving the exemption keys off theDiagnosticCountertype handed tothis, not theEventSourceclass.CI assertions added to the
wpf-extractorjob for both the silenced counters and the firing control. The corpus benchmark guards against any real-C# recall regression (the exemption is purely additive on a shape no corpus sample matches).🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Tests
EventSource-ownedDiagnosticCounterpatterns and validates expected leak findings (including negative controls).Bug Fixes
DiagnosticCounterfields that are owned by a process-livedEventSourceviathis, while preserving detections for other owned disposables and mismatched ownership/type scenarios.