audit(runtime): retention paths — the half of the runtime arm HeapCounter leaves undone#280
Open
PhysShell wants to merge 4 commits into
Open
audit(runtime): retention paths — the half of the runtime arm HeapCounter leaves undone#280PhysShell wants to merge 4 commits into
PhysShell wants to merge 4 commits into
Conversation
A regression guard for the release-reachability soundness gap: the subscription is to a STATIC publisher, a matching `-=` does exist in the class — so the extractor's "any matching `-=` in the class releases it" model falls silent — but it never runs. It sits behind a bool parameter every caller passes as `true`, in a method that is not a teardown, and that a whole subsystem never calls at all. before.cs is RED today: it is a real leak and own-check says nothing. That is the point of the case. after.cs must stay silent (the release is unconditional and in Dispose), so the fix cannot trade the false negative for a false positive. The .own reduction shows the core already handles this correctly — it reports OWN001 "not released before return (leaks on at least one path)" and points at the guarded branch. The bug is upstream, in the extractor's release-matching. Reduced from SectorTS GTD.cs:5192/:5259 and proven on the heap with a ClrMD root walk: 66.3% of the heap still reachable from the GC roots after 31 documents, held via [PinnedHandle] -> static KernelProperty -> GBProperty -> PropertyChangedEventHandler -> GTD. Detaching after each document makes the process memory-flat (peak RSS 2.71 GB -> 0.61 GB, byte-identical output), which confirms the diagnosis. Prior art: corpus/wpf/subscription-explicit-delegate-release/notes.md:28-41 already records the model as not flow-sensitive, but scoped it to a rebinding setter and deferred it. This case shows the surface is wider and gives it a real instance. CI: recall floor is unaffected (--min-recall is an absolute floor of caught cases, and this one is not caught yet); after.cs adds no false positive. The cfg/diag parity fixtures are regenerated because the corpus grew; tests/run_tests.py and cargo test are both green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjQnE1FDucd6vBVQbFswiE
…nter leaves undone The README table has promised "Heap analysis (retained, duplicates, retention paths) — ClrMD" since Plan.md §4, and Plan.md:300 says retention paths are "considered ourselves via ClrMD". No tool did. OwnAudit/src/OwnAudit.Runtime/RuntimeArm.cs is an explicit placeholder for exactly this — it already declares `RetentionProof(TypeName, RetainedDelta, GcRootPath, Leaks)` and a TODO to "gcroot the survivors" — and points here: "the runtime arm is CANONICAL in Own.NET/audit/runtime". RetentionPath answers the two questions that decide a leak hunt, and that HeapCounter cannot: census — is any of it RETAINED, or is the heap just full of uncollected garbage? roots — if it is retained, WHO holds it? The first is not pedantry. ClrHeap.EnumerateObjects() walks the heap segments linearly and returns everything allocated, INCLUDING garbage the GC has not collected. A big heap is not evidence of a leak. HeapCounter mitigates that by forcing a GC in the target first; marking from the roots answers it directly and needs no cooperation from the target. If the retained share is low there is no reference to hunt and the next question is about GC timing, not about who holds what. Every hop of a root path names the field it traverses (ClrMD's EnumerateReferencesWithFields), which is what turns "this object is alive" into "this field is holding it" — the sentence a developer can act on. Attaches to a LIVE process (no procdump — which HeapCounter needs and which is not always installed); a dump still works via --dump. Output is the runtime.json contract (OwnAudit/docs/runtime-contract.md), so OwnAudit's runtime/correlate.py consumes it with no adapter — closing the loop its PR #13 designed: a `runtime-only` finding (retention with nothing static to explain it) is the analyzer's blind spot, i.e. a rule request. #278 is the first one. Validated end-to-end against a live net472 x64 target: 70.4% of a 573 MB heap retained, and the root walk names the holding fields. Limits are documented in the README rather than glossed: the path reported is the SHORTEST one and may not be the most explanatory; a [stack] root is not retention; and type matching compares the type, not its spelling (asking for GTDGoody must not match System.Func<...GTDGoody, Boolean> — it did, and confidently reported a path to the wrong object). Not CI-gated, like its three neighbours — CI runs only audit/runtime/ingest.py (--selftest 26/26, still green; tests/run_tests.py exit 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjQnE1FDucd6vBVQbFswiE
📝 WalkthroughWalkthroughAdds a Windows ChangesRuntime retention analysis
Subscription release regression
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant RetentionPath
participant RetentionWalker
participant ClrMD
participant RuntimeJson
Operator->>RetentionPath: Run census, roots, or dominators
RetentionPath->>RetentionWalker: Attach to process or load dump
RetentionWalker->>ClrMD: Enumerate heap, roots, and references
ClrMD-->>RetentionWalker: Managed object graph
RetentionWalker-->>RetentionPath: Census, RetentionReport, or DominatorTree
RetentionPath->>RuntimeJson: Write optional runtime.json
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.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: 9f030ba5cd
ℹ️ 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".
Comment on lines
+119
to
+120
| targets[o.Address] = o.Type.Name; | ||
| if (targets.Count >= maxPaths) break; |
There was a problem hiding this comment.
When there are more matching objects than --max-paths, this caps targets using Heap.EnumerateObjects() before the root walk has proven those objects are reachable. In the common case this tool is meant to handle—some matching instances are uncollected garbage while later instances of the same type are actually rooted—the default --max-paths 3 can fill targets with garbage addresses, the BFS never finds them, and roots reports no live instance even though a retained leak exists. Match the requested type as nodes are reached from roots, or avoid truncating the precomputed target set.
Useful? React with 👍 / 👎.
| foreach (var reference in obj.EnumerateReferencesWithFields()) | ||
| { | ||
| var child = reference.Object; | ||
| if (!child.IsValid || !seen.Add(child.Address)) continue; |
There was a problem hiding this comment.
This address-only seen pruning means the walker can never return more than one path to a given suspect object. If the same retained instance is reachable both from a transient stack root and from a static event, the shortest stack path wins and --max-paths 3 still cannot show the static-event path the README tells users to look for, so the actionable retainer is hidden. Allow additional paths/root kinds for already-seen target objects rather than dropping every duplicate address globally.
Useful? React with 👍 / 👎.
Comment on lines
+286
to
+288
| bool viaDelegate = Path.Any(p => | ||
| p.IndexOf("EventHandler", StringComparison.Ordinal) >= 0 || | ||
| p.IndexOf("MulticastDelegate", StringComparison.Ordinal) >= 0); |
There was a problem hiding this comment.
For static events whose delegate type name does not contain EventHandler—for example Action, Func<T>, or a custom delegate with a different suffix—the path still traverses a delegate object but viaDelegate stays false, so a pinned static-event root is emitted as static-field. The comment says downstream correlation keys on static-event, so these common static-event leaks are mis-bucketed unless delegate-ness is detected from type metadata rather than only these two substrings.
Useful? React with 👍 / 👎.
… object "Who holds this object" is ill-posed for an object reachable from many roots: there are as many answers as there are paths, and the shortest is an arbitrary pick. Reporting one of them was the tool's real weakness — the first version happily pointed at a prototype held by the stack while the leak was eight hops away through a delegate's invocation list. `roots` now samples the retained instances (--sample, default 200), computes every one's shortest path in a SINGLE BFS (breadth-first from the whole root set gives each node its shortest path for free), and reports the paths as a RANKED HISTOGRAM. On the live target this finds the leak on its own: #1 25/50 (50.0%) — via [static-event], 7 hops System.Object[] BrokerDataClasses.Property.KernelProperty BrokerDataClasses.Property.GBProperty (.fGBProperty) System.ComponentModel.PropertyChangedEventHandler (.PropertyChanged) System.Object[] (._invocationList) System.ComponentModel.PropertyChangedEventHandler BrokerDataClasses.GTD (._target) #4 1/50 (2.0%) — via [stack] — noise, and labelled as such Parent pointers are stored as addresses, not labels; type and field names are resolved only for the sampled paths. Carrying a label per node would cost hundreds of MB on a 4M-object heap. The run also exposes the remaining limit, and the README now says so plainly: the histogram partitions by SHORTEST path, so an object held by two references at once is attributed to the nearer one. Here 50% of the GTDs land under the static event and 44% under a static List<Object> — most likely many are held by BOTH, and detaching the event alone would not free them. "Which single reference, if cut, frees this object" is a different, well-posed question with a standard answer this tool does not implement: a dominator tree with retained sizes (Lengauer-Tarjan / Cooper-Harvey-Kennedy — what Eclipse MAT and dotMemory are built on). That is the honest next step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PjQnE1FDucd6vBVQbFswiE
|
@coderabbitai review |
… if cut, frees the memory
`roots` reports what holds the typical instance. It cannot tell you that CUTTING that
reference would free anything, because an object held by two references at once is
attributed to whichever is nearer. That is not an implementation shortcoming — "who holds
it" is ill-posed. The well-posed question is "which single reference, if cut, makes this
object collectable, and how much memory does that free", and dominance answers it: D
dominates X when EVERY path from a root to X goes through D, so D's retained size is what
you get back by dropping it. This is what Eclipse MAT and dotMemory are built on.
It immediately paid for itself on the SectorTS leak. `roots` names the static
PropertyChanged event, and it is not wrong — but `dominators` says:
>>> NO single reference holds this memory — the biggest dominator accounts for only
6.7% (24 MB of 361 MB). The objects are reachable from SEVERAL roots at once, so
cutting any one of them frees nothing.
Which is correct, and is exactly what the real fix turned out to be: the working fix
detaches SEVERAL references at once (UnregisterEventHandlers(false)). A shortest-path
walk would have named one, confidently, and the fix would not have worked.
Algorithm: Cooper-Harvey-Kennedy, "A Simple, Fast Dominance Algorithm" (2001) — the
iterative formulation LLVM used for years. A page of code, converges in a couple of
passes, no balanced forests. The paper is public; this is an implementation of it, not a
copy of anyone's code. (PerfView, MIT, is the closest .NET reference — it computes a
spanning tree with inclusive sizes, which approximates this.)
The graph is CSR, not List<List<int>>: ids are handed out in discovery order and BFS
processes nodes in that same order, so each node's successors land contiguously in one
int[]. ~150 bytes/object; 3.7M objects cost ~600 MB and 48 s attached live.
Correctness is not taken on faith:
* `RetentionPath selftest` (no target, no Windows, no ClrMD) checks the algorithm
against graphs whose dominators are known by hand — a DIAMOND (an object reachable
through both branches is dominated by NEITHER — the exact case a path walk gets
wrong), a CHAIN (retained size accumulates), and a CYCLE (a gate dominates a
reference cycle, which reference counting can never free). 13/13.
* At run time the super-root's retained size must equal the total size of the reachable
graph, or the walk REFUSES to report. A wrong dominator tree does not fail loudly — it
quietly tells you to cut the wrong reference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PjQnE1FDucd6vBVQbFswiE
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
corpus/wpf/unsubscribe-behind-a-flag/notes.md (1)
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Static analysis tools flag this fenced code block for lacking a language identifier (MD040). Consider specifying
textto resolve the warning.💡 Proposed refactor
-``` +```text on the heap : 1 685 951 objects 223 MB REACHABLE from roots : 1 569 072 objects 148 MB >>> 66.3% of the heap is genuinely RETAINED [PinnedHandle] System.Object[] KernelProperty <- AppData.Properties (static) GBProperty PropertyChangedEventHandler System.Object[] <- the delegate's invocation list PropertyChangedEventHandler GTD <- the whole document graph</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@corpus/wpf/unsubscribe-behind-a-flag/notes.mdaround lines 44 - 56, Add the
text language identifier to the fenced code block in the notes content, changing
the opening fence to use text while preserving the diagnostic output unchanged.</details> <!-- cr-comment:v1:1da25edaab57711308bc03f7 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@audit/runtime/README.md:
- Around line 47-50: Update the RetentionPath layout documentation to include
Dominators.cs and describe the dominator-related command or functionality
provided by Program.cs alongside census and roots. Keep the existing file
descriptions unchanged otherwise.In
@audit/runtime/RetentionPath/Dominators.cs:
- Around line 349-360: The Dominators.Top method reports dominating objects, not
uniquely removable references. In audit/runtime/RetentionPath/Dominators.cs
lines 349-360, either compute and expose actionable dominating edges/unique
ingress or explicitly label results as objects whose all incoming paths must be
removed; update audit/runtime/RetentionPath/Program.cs lines 237-260 to avoid
claiming that one reference can be cut unless an edge is proven; revise
audit/runtime/README.md lines 115-127 to document object-retained size and the
corresponding removal semantics.In
@audit/runtime/RetentionPath/Heap.cs:
- Around line 123-133: In audit/runtime/RetentionPath/Heap.cs lines 123-133,
update the roots workflow to determine reachability for all matching objects
before limiting the population to the sample size, preserve the exact
retained-object count, and generate sampled retention paths only from that
reachable population. In audit/runtime/RetentionPath/Program.cs lines 196-200,
serialize the exact retained count returned by the retention report instead of
TotalOnHeap.- Around line 317-334: The ContractKind method incorrectly classifies any
delegate-containing GC-handle path as a static event. Restrict static-event
classification to paths that establish a static event root or equivalent static
ownership evidence, while preserving static-field for non-delegate pinned
handles and gc-handle for ordinary handle-rooted instance-event paths.- Around line 176-183: Update the grouping key construction in the RetentionPath
grouping flow around Unwind and the signature variable to include each hop’s
identifying field information and the resulting root kind, not just hop types.
Ensure distinct fields and root kinds produce separate Retainer entries while
preserving the existing Retainer creation and group lookup behavior.- Around line 49-55: Update the RetentionWalker constructor and Dispose
implementation to guarantee _target.Dispose() runs whenever CLR discovery or
clr.CreateRuntime() fails, and even if _runtime.Dispose() throws or blocks. Use
try/finally around construction and runtime cleanup, preserving normal
initialization and disposal behavior while always releasing the DataTarget.In
@audit/runtime/RetentionPath/Program.cs:
- Around line 266-272: The fallback report in the RetentionPath analysis should
describe memory as distributed across multiple dominator subtrees without
asserting that every object has several root paths or that detaching any single
root frees nothing. Update the output in Program.cs and revise the corresponding
README example conclusion so both consistently use this narrower interpretation.- Around line 138-141: Validate the parsed sample and maxHops options before
calling walker.FindRetainers in the analysis flow: reject non-positive values
for both, including --sample 0, and terminate with the existing argument-error
behavior rather than proceeding with invalid settings.- Around line 172-179: Update the dominant retention headline condition in the
report logic around dominant and ContractKind() so it excludes finalizer roots
as well as stack roots. Preserve the existing threshold and headline output for
other contract kinds, using the established ContractKind() value that identifies
finalizer-queue objects.
Nitpick comments:
In@corpus/wpf/unsubscribe-behind-a-flag/notes.md:
- Around line 44-56: Add the text language identifier to the fenced code block
in the notes content, changing the opening fence to use text while preserving
the diagnostic output unchanged.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `3aaf5255-4ec9-4fb1-a182-8b31cd656f80` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 53bf05abd7a3aa3b0fff4a2eea231687b112b0ea and 35368fbdb5c73f3925857062bb3beaf7671580c2. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `audit/runtime/README.md` * `audit/runtime/RetentionPath/Dominators.cs` * `audit/runtime/RetentionPath/Heap.cs` * `audit/runtime/RetentionPath/Program.cs` * `audit/runtime/RetentionPath/RetentionPath.csproj` * `corpus/wpf/unsubscribe-behind-a-flag/after.cs` * `corpus/wpf/unsubscribe-behind-a-flag/before.cs` * `corpus/wpf/unsubscribe-behind-a-flag/case.own` * `corpus/wpf/unsubscribe-behind-a-flag/expected-diagnostics.txt` * `corpus/wpf/unsubscribe-behind-a-flag/notes.md` * `tests/fixtures/cfg_parity.json` * `tests/fixtures/diag_parity.json` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+47
to
+50
| RetentionPath/ # C# retention paths — Windows/build-required, NOT CI-gated | ||
| RetentionPath.csproj # net472; Microsoft.Diagnostics.Runtime | ||
| Heap.cs # ClrMD: mark from the GC roots; root -> object path with field names | ||
| Program.cs # `census` (is it retained at all?) and `roots` (who holds it?) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the dominator implementation in the layout.
The new layout omits Dominators.cs and still describes Program.cs as supporting only census and roots.
Proposed documentation update
RetentionPath/
RetentionPath.csproj # net472; Microsoft.Diagnostics.Runtime
Heap.cs # ClrMD: mark from the GC roots; root -> object path with field names
- Program.cs # `census` (is it retained at all?) and `roots` (who holds it?)
+ Dominators.cs # dominator tree and retained-size analysis
+ Program.cs # `census`, `roots`, and `dominators` CLI commands📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RetentionPath/ # C# retention paths — Windows/build-required, NOT CI-gated | |
| RetentionPath.csproj # net472; Microsoft.Diagnostics.Runtime | |
| Heap.cs # ClrMD: mark from the GC roots; root -> object path with field names | |
| Program.cs # `census` (is it retained at all?) and `roots` (who holds it?) | |
| RetentionPath/ # C# retention paths — Windows/build-required, NOT CI-gated | |
| RetentionPath.csproj # net472; Microsoft.Diagnostics.Runtime | |
| Heap.cs # ClrMD: mark from the GC roots; root -> object path with field names | |
| Dominators.cs # dominator tree and retained-size analysis | |
| Program.cs # `census`, `roots`, and `dominators` CLI commands |
🤖 Prompt for 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.
In `@audit/runtime/README.md` around lines 47 - 50, Update the RetentionPath
layout documentation to include Dominators.cs and describe the dominator-related
command or functionality provided by Program.cs alongside census and roots. Keep
the existing file descriptions unchanged otherwise.
Comment on lines
+349
to
+360
| /// <summary> | ||
| /// The objects whose removal frees the most memory — i.e. the answer to "what is holding | ||
| /// all of this". The super-root is skipped (it dominates everything by construction, which | ||
| /// is true and useless). | ||
| /// </summary> | ||
| public IReadOnlyList<DominatorHit> Top(int count, long minBytes) | ||
| { | ||
| var hits = new List<DominatorHit>(); | ||
| for (int u = 1; u < _n; u++) | ||
| { | ||
| if (_retained[u] < minBytes) continue; | ||
| hits.Add(new DominatorHit(u, _address[u], _retained[u], _size[u])); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Separate object dominance from edge dominance. The implementation computes dominating objects, but the CLI and documentation promise one removable reference.
audit/runtime/RetentionPath/Dominators.cs#L349-L360: compute dominating edges/unique ingress, or expose results explicitly as objects requiring all incoming paths to be removed.audit/runtime/RetentionPath/Program.cs#L237-L260: stop saying “cut this ONE reference” unless an actionable edge was proven.audit/runtime/README.md#L115-L127: document object-retained size rather than unique-reference removal semantics.
📍 Affects 3 files
audit/runtime/RetentionPath/Dominators.cs#L349-L360(this comment)audit/runtime/RetentionPath/Program.cs#L237-L260audit/runtime/README.md#L115-L127
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Dominators.cs` around lines 349 - 360, The
Dominators.Top method reports dominating objects, not uniquely removable
references. In audit/runtime/RetentionPath/Dominators.cs lines 349-360, either
compute and expose actionable dominating edges/unique ingress or explicitly
label results as objects whose all incoming paths must be removed; update
audit/runtime/RetentionPath/Program.cs lines 237-260 to avoid claiming that one
reference can be cut unless an edge is proven; revise audit/runtime/README.md
lines 115-127 to document object-retained size and the corresponding removal
semantics.
Comment on lines
+49
to
+55
| private RetentionWalker(DataTarget target) | ||
| { | ||
| _target = target; | ||
| var clr = _target.ClrVersions.FirstOrDefault() | ||
| ?? throw new InvalidOperationException( | ||
| "the target contains no CLR — is it a managed process / a full (-ma) dump?"); | ||
| _runtime = clr.CreateRuntime(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline audit/runtime/RetentionPath/Heap.cs --view expanded || true
# Show the relevant section with line numbers
sed -n '1,260p' audit/runtime/RetentionPath/Heap.cs | cat -n
# Find all disposal-related members in the file
rg -n "Dispose|IDisposable|DataTarget|ClrRuntime|ClrVersions|CreateRuntime" audit/runtime/RetentionPath/Heap.csRepository: PhysShell/Own.NET
Length of output: 17220
Guarantee cleanup on constructor and dispose failure paths. RetentionWalker can leave DataTarget open if CLR discovery/runtime creation throws, and _runtime.Dispose() can block _target.Dispose(). Wrap construction and disposal in try/finally so the target is always released, especially for live attachments that suspend the process.
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Heap.cs` around lines 49 - 55, Update the
RetentionWalker constructor and Dispose implementation to guarantee
_target.Dispose() runs whenever CLR discovery or clr.CreateRuntime() fails, and
even if _runtime.Dispose() throws or blocks. Use try/finally around construction
and runtime cleanup, preserving normal initialization and disposal behavior
while always releasing the DataTarget.
Comment on lines
+123
to
+133
| var targets = new Dictionary<ulong, string>(); | ||
| long totalOfType = 0; | ||
| foreach (var o in Heap.EnumerateObjects()) | ||
| { | ||
| if (!o.IsValid || o.Type?.Name == null) continue; | ||
| if (!IsType(o.Type.Name, typeName)) continue; | ||
| totalOfType++; | ||
| if (targets.Count < sample) targets[o.Address] = o.Type.Name; | ||
| } | ||
| if (targets.Count == 0) | ||
| return new RetentionReport(typeName, 0, 0, new List<Retainer>()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Compute retained targets before sampling and serialization. The roots workflow currently samples heap objects before reachability is known and consequently lacks the exact retained count required by runtime.json.
audit/runtime/RetentionPath/Heap.cs#L123-L133: mark matching reachable objects first, record their exact count, and sample paths from that population.audit/runtime/RetentionPath/Program.cs#L196-L200: serialize the exact retained count rather thanTotalOnHeap.
📍 Affects 2 files
audit/runtime/RetentionPath/Heap.cs#L123-L133(this comment)audit/runtime/RetentionPath/Program.cs#L196-L200
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Heap.cs` around lines 123 - 133, In
audit/runtime/RetentionPath/Heap.cs lines 123-133, update the roots workflow to
determine reachability for all matching objects before limiting the population
to the sample size, preserve the exact retained-object count, and generate
sampled retention paths only from that reachable population. In
audit/runtime/RetentionPath/Program.cs lines 196-200, serialize the exact
retained count returned by the retention report instead of TotalOnHeap.
Comment on lines
+176
to
+183
| var hops = Unwind(kv.Key, parent, rootKind, maxHops, out ClrRootKind kind); | ||
| string signature = string.Join(" -> ", hops.Select(h => h.Type)); | ||
|
|
||
| if (!groups.TryGetValue(signature, out var retainer)) | ||
| { | ||
| retainer = new Retainer(hops, kind); | ||
| groups[signature] = retainer; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include fields and root kind in the grouping key.
Grouping only by hop types merges distinct fields and root kinds. The first path then supplies Member, RootKind, and runtime.json classification for every merged instance.
Proposed grouping key
-string signature = string.Join(" -> ", hops.Select(h => h.Type));
+string signature = kind + " -> " + string.Join(
+ " -> ",
+ hops.Select(h => h.Type + "\u001f" + (h.Field ?? "")));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var hops = Unwind(kv.Key, parent, rootKind, maxHops, out ClrRootKind kind); | |
| string signature = string.Join(" -> ", hops.Select(h => h.Type)); | |
| if (!groups.TryGetValue(signature, out var retainer)) | |
| { | |
| retainer = new Retainer(hops, kind); | |
| groups[signature] = retainer; | |
| } | |
| var hops = Unwind(kv.Key, parent, rootKind, maxHops, out ClrRootKind kind); | |
| string signature = kind + " -> " + string.Join( | |
| " -> ", | |
| hops.Select(h => h.Type + "\u001f" + (h.Field ?? ""))); | |
| if (!groups.TryGetValue(signature, out var retainer)) | |
| { | |
| retainer = new Retainer(hops, kind); | |
| groups[signature] = retainer; | |
| } |
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Heap.cs` around lines 176 - 183, Update the
grouping key construction in the RetentionPath grouping flow around Unwind and
the signature variable to include each hop’s identifying field information and
the resulting root kind, not just hop types. Ensure distinct fields and root
kinds produce separate Retainer entries while preserving the existing Retainer
creation and group lookup behavior.
Comment on lines
+317
to
+334
| public string ContractKind() | ||
| { | ||
| bool viaDelegate = Path.Any(h => | ||
| h.Type.IndexOf("EventHandler", StringComparison.Ordinal) >= 0 || | ||
| h.Type.IndexOf("MulticastDelegate", StringComparison.Ordinal) >= 0 || | ||
| (h.Field != null && h.Field.IndexOf("invocationList", StringComparison.OrdinalIgnoreCase) >= 0)); | ||
|
|
||
| switch (RootKind) | ||
| { | ||
| case ClrRootKind.Stack: | ||
| return "stack"; // live in a frame right now — not retention | ||
| case ClrRootKind.FinalizerQueue: | ||
| return "finalizer"; // awaiting finalization — a stall, not a reference leak | ||
| case ClrRootKind.PinnedHandle: | ||
| return viaDelegate ? "static-event" : "static-field"; | ||
| default: | ||
| return viaDelegate ? "static-event" : "gc-handle"; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not infer static-event from delegate presence alone.
The default branch labels every GC-handle path containing a delegate as static-event. Instance-event delegates can appear on ordinary handle-rooted paths, producing false static-leak correlation.
Conservative classification
case ClrRootKind.PinnedHandle:
return viaDelegate ? "static-event" : "static-field";
default:
- return viaDelegate ? "static-event" : "gc-handle";
+ return "gc-handle";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public string ContractKind() | |
| { | |
| bool viaDelegate = Path.Any(h => | |
| h.Type.IndexOf("EventHandler", StringComparison.Ordinal) >= 0 || | |
| h.Type.IndexOf("MulticastDelegate", StringComparison.Ordinal) >= 0 || | |
| (h.Field != null && h.Field.IndexOf("invocationList", StringComparison.OrdinalIgnoreCase) >= 0)); | |
| switch (RootKind) | |
| { | |
| case ClrRootKind.Stack: | |
| return "stack"; // live in a frame right now — not retention | |
| case ClrRootKind.FinalizerQueue: | |
| return "finalizer"; // awaiting finalization — a stall, not a reference leak | |
| case ClrRootKind.PinnedHandle: | |
| return viaDelegate ? "static-event" : "static-field"; | |
| default: | |
| return viaDelegate ? "static-event" : "gc-handle"; | |
| } | |
| public string ContractKind() | |
| { | |
| bool viaDelegate = Path.Any(h => | |
| h.Type.IndexOf("EventHandler", StringComparison.Ordinal) >= 0 || | |
| h.Type.IndexOf("MulticastDelegate", StringComparison.Ordinal) >= 0 || | |
| (h.Field != null && h.Field.IndexOf("invocationList", StringComparison.OrdinalIgnoreCase) >= 0)); | |
| switch (RootKind) | |
| { | |
| case ClrRootKind.Stack: | |
| return "stack"; // live in a frame right now — not retention | |
| case ClrRootKind.FinalizerQueue: | |
| return "finalizer"; // awaiting finalization — a stall, not a reference leak | |
| case ClrRootKind.PinnedHandle: | |
| return viaDelegate ? "static-event" : "static-field"; | |
| default: | |
| return "gc-handle"; | |
| } |
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Heap.cs` around lines 317 - 334, The ContractKind
method incorrectly classifies any delegate-containing GC-handle path as a static
event. Restrict static-event classification to paths that establish a static
event root or equivalent static ownership evidence, while preserving
static-field for non-delegate pinned handles and gc-handle for ordinary
handle-rooted instance-event paths.
Comment on lines
+138
to
+141
| int sample = ArgInt(args, "--sample", 200); | ||
| int maxHops = ArgInt(args, "--max-hops", 40); | ||
|
|
||
| var report = walker.FindRetainers(type, sample, maxHops); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-positive sampling options.
--sample 0 leaves targets empty and reports that no instance exists even when totalOfType is nonzero. Validate both sample and maxHops before analysis.
Proposed validation
int sample = ArgInt(args, "--sample", 200);
int maxHops = ArgInt(args, "--max-hops", 40);
+if (sample <= 0 || maxHops <= 0)
+{
+ Console.Error.WriteLine("--sample and --max-hops must be positive");
+ return 2;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int sample = ArgInt(args, "--sample", 200); | |
| int maxHops = ArgInt(args, "--max-hops", 40); | |
| var report = walker.FindRetainers(type, sample, maxHops); | |
| int sample = ArgInt(args, "--sample", 200); | |
| int maxHops = ArgInt(args, "--max-hops", 40); | |
| if (sample <= 0 || maxHops <= 0) | |
| { | |
| Console.Error.WriteLine("--sample and --max-hops must be positive"); | |
| return 2; | |
| } | |
| var report = walker.FindRetainers(type, sample, maxHops); |
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Program.cs` around lines 138 - 141, Validate the
parsed sample and maxHops options before calling walker.FindRetainers in the
analysis flow: reject non-positive values for both, including --sample 0, and
terminate with the existing argument-error behavior rather than proceeding with
invalid settings.
Comment on lines
+172
to
+179
| var dominant = report.Retainers[0]; | ||
| double dominantShare = 100.0 * dominant.Instances / report.SampledRetained; | ||
| if (dominantShare >= 50 && dominant.ContractKind() != "stack") | ||
| { | ||
| string member = dominant.Member != null ? "." + dominant.Member : ""; | ||
| Console.WriteLine($">>> {dominantShare:N1}% of the retained instances hang off ONE reference: " + | ||
| $"{dominant.Holder}{member} [{dominant.ContractKind()}]"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude finalizer roots from the retention headline.
The documentation and ContractKind() describe finalizer-queue objects as awaiting finalization rather than reference leaks, but this condition excludes only stack. A dominant finalizer path is consequently reported as one actionable retaining reference.
Proposed condition
-if (dominantShare >= 50 && dominant.ContractKind() != "stack")
+string dominantKind = dominant.ContractKind();
+if (dominantShare >= 50 &&
+ dominantKind != "stack" &&
+ dominantKind != "finalizer")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var dominant = report.Retainers[0]; | |
| double dominantShare = 100.0 * dominant.Instances / report.SampledRetained; | |
| if (dominantShare >= 50 && dominant.ContractKind() != "stack") | |
| { | |
| string member = dominant.Member != null ? "." + dominant.Member : ""; | |
| Console.WriteLine($">>> {dominantShare:N1}% of the retained instances hang off ONE reference: " + | |
| $"{dominant.Holder}{member} [{dominant.ContractKind()}]"); | |
| } | |
| var dominant = report.Retainers[0]; | |
| double dominantShare = 100.0 * dominant.Instances / report.SampledRetained; | |
| string dominantKind = dominant.ContractKind(); | |
| if (dominantShare >= 50 && | |
| dominantKind != "stack" && | |
| dominantKind != "finalizer") | |
| { | |
| string member = dominant.Member != null ? "." + dominant.Member : ""; | |
| Console.WriteLine($">>> {dominantShare:N1}% of the retained instances hang off ONE reference: " + | |
| $"{dominant.Holder}{member} [{dominant.ContractKind()}]"); | |
| } |
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Program.cs` around lines 172 - 179, Update the
dominant retention headline condition in the report logic around dominant and
ContractKind() so it excludes finalizer roots as well as stack roots. Preserve
the existing threshold and headline output for other contract kinds, using the
established ContractKind() value that identifies finalizer-queue objects.
Comment on lines
+266
to
+272
| else | ||
| { | ||
| Console.WriteLine($">>> NO single reference holds this memory — the biggest dominator accounts for " + | ||
| $"only {explained:N1}% ({Mb(biggest):N0} MB of {Mb(tree.TotalRetained):N0} MB)."); | ||
| Console.WriteLine(" The objects are reachable from SEVERAL roots at once, so cutting any one of"); | ||
| Console.WriteLine(" them frees nothing. The fix must detach all of them. (A shortest-path walk"); | ||
| Console.WriteLine(" would have named one and been confidently wrong.)"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A small largest dominator does not prove shared multi-root retention.
audit/runtime/RetentionPath/Program.cs#L266-L272: report that memory is distributed across dominator subtrees without claiming each object has several root paths.audit/runtime/README.md#L140-L142: make the example conclusion match that narrower result.
📍 Affects 2 files
audit/runtime/RetentionPath/Program.cs#L266-L272(this comment)audit/runtime/README.md#L140-L142
🤖 Prompt for 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.
In `@audit/runtime/RetentionPath/Program.cs` around lines 266 - 272, The fallback
report in the RetentionPath analysis should describe memory as distributed
across multiple dominator subtrees without asserting that every object has
several root paths or that detaching any single root frees nothing. Update the
output in Program.cs and revise the corresponding README example conclusion so
both consistently use this narrower interpretation.
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
1 participant
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.
Adds
audit/runtime/RetentionPath/— a fourth standalone tool alongsideLeakHarness,DuplicateDetectorandPropertyChangedStorm.Why here, and why now
The README table has promised "Heap analysis (retained, duplicates, retention paths) — ClrMD" since Plan.md §4, and
Plan.md:300says retention paths are "считаем сами по ClrMD". Nothing implemented them.HeapCounter.cscounts instances of named types; it has no root walk.Meanwhile
OwnAudit/src/OwnAudit.Runtime/RuntimeArm.csis an explicit placeholder that already declares the exact shape:…and points back here: "the runtime arm is CANONICAL in Own.NET/audit/runtime. Do NOT reimplement it here." So this is the declared home and the declared API.
What it does
Two verbs, and the first one is the one that matters:
This distinction is not pedantry.
ClrHeap.EnumerateObjects()walks the heap segments linearly and returns everything allocated, including garbage the GC has not collected yet. A big heap is not evidence of a leak.HeapCountermitigates that by forcing a GC in the target first (SematixTrace) — which works when you can drive the target. Marking from the roots answers it directly and needs no cooperation. If the retained share is low, stop: there is no reference to hunt, and the next question is about GC timing, not about who holds what.The field names come from ClrMD's
EnumerateReferencesWithFields. They are what turn "this object is alive" into "this field is holding it" — the sentence a developer can act on.Note
[PinnedHandle]: there is noStaticVarroot kind — on .NET Framework a class's statics live in a pinnedSystem.Object[], so that is where a static-field leak surfaces. A delegate hop further down the path is what makes it a static event rather than a plain static field — the distinctioncorrelate.py'shightier keys on.It closes the loop OwnAudit PR #13 designed
Output is the
runtime.jsoncontract (OwnAudit/docs/runtime-contract.md), soruntime/correlate.pyconsumes it with no adapter. That gives the three-way split its missing input:#278 is the first
runtime-onlyfinding, and it came out of exactly this walk.What it does not do — please read before trusting it
--max-paths 3and read them all. "Which retainer holds most of the N instances" is the obvious next feature and is not built.[stack]root is not retention — the object is live in a frame right now. It is labelled as such so it is not mistaken for a leak; same for[finalizer].--type GTDGoodymust not matchSystem.Func<BrokerDataClasses.GTDGoody, System.Boolean>— a cached lambda whose generic argument mentions it. It did, during development, and confidently reported a 2-hop path to the wrong object. A tool that points at the wrong culprit is worse than no tool.Validation
Built and run end-to-end against a live net472 x64 target (not a synthetic fixture): 70.4 % of a 573 MB heap retained, root paths resolved with field names, exit codes behave (0 = analysed/clean, 1 = retention found, 2 = could not read — a failed read must never read as clean).
CI
Not CI-gated, like its three neighbours — CI runs only
audit/runtime/ingest.py. Both still green:ingest.py --selftest→ 26/26,tests/run_tests.py→ exit 0. No solution file to update; no existing project touched.🤖 Generated with Claude Code
https://claude.ai/code/session_01PjQnE1FDucd6vBVQbFswiE
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests