Tier the ignored-.Subscribe() leak by source lifetime (self-cycle → silent)#40
Merged
Conversation
… result)
The bridge half of the WalletWasabi precision win (118 raw -> the real leaks).
The ignored-`.Subscribe()` detector was a flat OWN001 ("always a leak"), but a
self-rooted `this.WhenAnyValue(x => x.SelfProp).Subscribe(...)` is a
GC-collectible self-cycle (the observable, its handler and `this` form one cycle
the GC collects together) -- NOT a leak. Only an EXTERNAL source (EventBus, an
injected model/field) holds the component from a longer-lived root.
Tier the `subscribe` finding by `source`, mirroring the `+=` else branch:
- source == "self" -> dropped in to_module/to_own (silent, no finding)
- source == "injected" -> OWN001 WARNING (unknown lifetime, "may outlive")
- source == "static"/external/unannotated -> OWN001 leak (error, unchanged)
Additive + zero regression: an unannotated `subscribe` (every current fixture)
stays an OWN001 error. The extractor half -- classifying the `.Subscribe()` chain
root and stamping `source` -- is CI-validated next; this engine is what makes the
self-cycles go silent once it does. ownir 77/77, full suite, ruff, mypy --strict.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
The extractor half of the WalletWasabi precision win. The ignored-`.Subscribe()` detector (WPF004) now classifies the chain root and stamps `source`: a self-rooted `this.WhenAnyValue(p => p.SelfProp).….Subscribe(...)` is a GC-collectible self-cycle (`source: "self"`, which the bridge drops), while any other root -- an external receiver, `EventBus.Subscribe`, an injected field, or a NESTED path `p => p.A.B` that roots through `A` -- stays unclassified (`source: null`) and remains a flagged leak. `IsSelfRootedWhenAny` is purely syntactic (no SemanticModel): walk the fluent chain to its leftmost invocation and require `this.WhenAnyValue(p => p.Member)` with a single-hop self-member lambda. Conservative by design -- it silences only the unambiguous self-cycle, never an external-source subscription. Safe on the existing sample: `MessengerViewModel`'s `messenger.Subscribe(OnMessage)` has a field receiver (not WhenAnyValue) -> `source: null` -> unchanged OWN001. The bridge tiering that consumes `source` landed in the previous commit; together they take the WalletWasabi mine's 118 raw findings down to the real (external-source) leaks. CI-validated (no local dotnet). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Roslyn extractor gains ChangesSubscribe Source-Lifetime Tiering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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://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: bd2106bad1
ℹ️ 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".
…ns (codex P1) codex flagged a real recall regression: IsSelfRootedWhenAny only inspected the leftmost invocation, so `this.WhenAnyValue(x => x.Foo).CombineLatest(_bus.Stream) .Subscribe(...)` (or `.SelectMany(_ => _bus.Events)`) stamped source:"self" and got silenced -- yet the downstream external observable roots the subscription externally, so a real leak went quiet. Fix: a chain is self-rooted only if the HEAD is `this.WhenAnyValue(p => p.Member)` AND every downstream operator is self-preserving (args are only funcs/schedulers/ scalars). A combinator (CombineLatest/Merge/SelectMany/WithLatestFrom/Zip/Switch/ ...), an operator with an observable-taking overload (Throttle/Buffer/Sample/ TakeUntil/Window), or any UNKNOWN operator is treated as possibly-external, so the chain stays flagged. Conservative by construction: an unrecognised op never silences a real leak. Trade-off: a few self-cycles using ambiguous-overload ops (Throttle/Buffer) are re-flagged (residual FP), but no external-source leak can be silenced. CI validates the build + the unchanged MessengerViewModel sample. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This was referenced Jun 19, 2026
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
2 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.
Source-lifetime tiering for the ignored-
.Subscribe()leak detector (WPF004) — the precision half of the WalletWasabi find. Two commits, both halves of the pipeline.The problem
The detector flagged any ignored
.Subscribe()as a flat OWN001. But a self-rootedthis.WhenAnyValue(x => x.SelfProp).…Subscribe(...)is a GC-collectible self-cycle (the observable, its handler andthisform one cycle the GC collects together) — not a leak. Only an external source (EventBus, an injected model/field) holds the component from a longer-lived root.Extractor (
bd2106b)IsSelfRootedWhenAny— purely syntactic, no SemanticModel: walk the fluent chain to its leftmost invocation; if it isthis.WhenAnyValue(p => p.Member)with a single-hop self-member lambda, stampsource: "self". Anything else (external receiver,EventBus.Subscribe, an injected field, a nestedp => p.A.B) stays unclassified (source: null) and remains flagged. Conservative by design — only the unambiguous self-cycle is touched.Bridge (
3b6f0e1)Tier the
subscribefinding bysource, mirroring the+=else branch:selfinjectedstatic/ external / unannotatedValidation
this.WhenAnyValue(x => x.OwnProp)self-cycles; every external-source leak survived (EventBus.Subscribe<ExchangeRateChanged>,amountProvider.X,wallet.Balances,WalletRepository.Wallets.Connect(), andtriggerSource.WhenAnyValue— an external receiver correctly not silenced). No external-source leak was silenced (no-FP-first held).ownir 77/77, full suite, ruff, mypy--strict. CI green: the C# leak extractor job built + ran the modified extractor and theMessengerViewModelsample is unchanged (messenger.Subscribeis a field receiver → still OWN001).Honest residual (conservative first cut)
Still flagged on purpose: multi-arg
this.WhenAnyValue(x => x.A, x => x.B)self-watches (also self-cycles → a richer classifier would trim these), and nested-paththis.WhenAnyValue(x => x.Settings.X)(genuinely ambiguous —Settingsmay be an injected model → a real leak). Multi-arg self-watch detection is the clean next refinement.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
.Subscribe()patterns andsubscribefacts: self-rooted cases are silently skipped, injected sources now raise warnings (“may outlive”), and static/external/unknown sources are treated as confirmed leaks.Documentation
resource: "subscribe"documentation to reflect the new source-based tiering behavior.Tests
subscribesource-lifetime tiering.