oracle: timer leak (а) + golden runtime-correlation fixture (б) - #21
Merged
Conversation
The in-scope half of слой 2: validate the architecture pass on a real, faithful graph of the LeakyOracle app (8 internal types + the framework types they touch) instead of synthetic fixtures, and pin the graph.json contract the Own.NET-side Roslyn extractor must emit for the oracle. - oracle/fixtures/graph.json — faithful arch-graph of the oracle (ownAudit/arch-graph/v1). - oracle/fixtures/rules.json — an MVVM layering profile (Views -> ViewModels -> Services; reverse forbidden), showing the rules engine is configurable per codebase, not STS-hardcoded. - test_oracle_arch.py: clean faithful graph -> 0 findings (arch/ doesn't cry wolf on well-layered MVVM; the oracle's smells are lifetime/heap, not architecture). One planted MVVM inversion (WatchlistViewModel -> MainWindow) -> exactly ARCH-MVVM-VM-VIEW + ARCH-CYCLE-TYPE + ARCH-CYCLE-NS. Also guards the graph against drift (every internal node must point at a real file under oracle/). Wired into CI (normal + -O). All suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
… P2, CodeRabbit) Codex: the documented extractor contract (docs/arch-graph.md) counts param/return types and generic args as dependencies, so a faithful extractor emits System.String edges for EventHandler<string>/Tick(string)/QuoteRow(string)/string[] — which the golden omitted. Add System.String (an external node) + its edges from the six types that reference it. String earns its place: string duplication is one of the oracle's own smells. Reframe to match reality: hand-authoring can't be byte-exact on BCL leaves, so the INTERNAL type graph is the contract the extractor must reproduce; external nodes list the meaningful framework types, and pure-infrastructure leaves (Int32, Console, GC, attributes, arrays) are elided — a faithful extractor emits a superset. The test already pins only the internal subgraph, not external completeness. CodeRabbit: the _comment's stale reference to a non-existent graph.degraded.json is removed by the same reframe (the regression variant is in-memory in the test). Still 0 findings on the clean graph; test 4/4 normal + -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
(а) A second intentional, heap-provable lifetime leak in the oracle: TickerViewModel starts a recurring System.Threading.Timer in its ctor and never disposes it, so it stays rooted by the runtime's TimerQueue (own-check OWN-TIMER; coverage matrix "Timers"). FixedTickerViewModel disposes it — the control case. LeakScenario now proves BOTH leaks (subscription + timer) and BOTH fixes through the same WeakReference harness: 50/50 leaky survive, 0/50 fixed collected, stable across runs. Builds clean on net8.0, zero warnings. (б) Runtime correlation golden fixture — phase 5 on real, oracle-shaped data: - findings.json: the static suspects own-check would emit (OWN001 subscription on WatchlistViewModel, OWN-TIMER on TickerViewModel); resource is a description, so correlate keys on the source-file stem. - runtime.json: heap evidence faithful to the leak proof — 50 of each leaky VM retained (expected 0), plus the 250k QuoteRow they transitively retain. - test_oracle_runtime.py: asserts the three-way split — both leaks CONFIRMED high (each naming the leaked CLR type + the static rule it corroborates), nothing static-only, QuoteRow surfaces RUNTIME-ONLY (a static blind spot), the high gate fails on the two confirmed leaks. Drift guard ties every retained type to a real oracle source file. Both fixtures pin the contract the слой-2 collectors (ClrMD heap walk) must emit; when they land in Own.NET, these same passes run on extracted data. Wired into CI (normal + -O); all suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a timer leak/control pair, expands the leak scenario to cover subscription and timer cases, adds runtime correlation fixtures and tests, and wires the new runtime test into CI. ChangesOracle timer leak correlation
Sequence Diagram(s)sequenceDiagram
participant TestScript as test_oracle_runtime.py
participant RuntimeCorrelate as runtime.correlate
participant Gate as C.gate
TestScript->>RuntimeCorrelate: correlate findings.json and runtime.json
RuntimeCorrelate-->>TestScript: confirmed, static_only, runtime_only
TestScript->>Gate: gate(res, "high")
Gate-->>TestScript: blocked items for high confidence
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 9a87eb60a4
ℹ️ 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".
| /// The `_timer` field is deliberate — without it the public Timer wrapper would be finalized and stop | ||
| /// the timer; holding it keeps the timer alive (and the leak real), exactly as leaky code does. | ||
| /// </summary> | ||
| public sealed class TickerViewModel |
There was a problem hiding this comment.
I checked oracle/fixtures/graph.json and oracle/fixtures/test_oracle_arch.py: the golden architecture contract still describes 8 internal types and has no TickerViewModel/FixedTickerViewModel nodes or LeakScenario edges to them. With this new view-model in the oracle, a real Roslyn extraction over oracle/LeakyOracle would now emit those two types, so the pinned graph is no longer faithful and the drift guard keeps passing the stale fixture instead of catching the mismatch.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Correct on both counts — fixed in 777f82b. Adding the timer VMs in (а) left the arch golden at 8 internal types, and the old drift guard ("every node points at a real file") structurally couldn't catch a missing type. Fix: added TickerViewModel/FixedTickerViewModel nodes + their edges (LeakScenario → Ticker VMs, Ticker → System.Threading.Timer, FixedTicker → IDisposable), and replaced the guard with a both-directions check — the graph's internal node set must equal the set of oracle source types (glob of LeakyOracle/**/*.cs), so a new view-model with no node (or a node for a deleted type) now fails the test. Still 0 findings on the clean graph; 4/4 normal + -O.
Generated by Claude Code
…n drift guard (Codex P2)
(а) added TickerViewModel/FixedTickerViewModel to the oracle but the arch golden
graph still described 8 internal types — a faithful extraction would now emit 10,
so the pinned graph was stale, and the old drift guard ("every node points at a
real file") couldn't catch a MISSING type.
Fix both: add the two nodes + their edges (LeakScenario -> Ticker VMs; Ticker VMs
-> System.Threading.Timer; FixedTicker -> IDisposable) and a System.Threading.Timer
external node. Replace the guard with a both-directions check: the graph's internal
node set must EQUAL the set of oracle source types (glob of LeakyOracle/**/*.cs),
so a new view-model without a node — or a node for a deleted type — fails the test.
Still 0 findings on the clean graph; planted-inversion test unchanged; 4/4 normal + -O.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
![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
🧹 Nitpick comments (1)
oracle/fixtures/test_oracle_runtime.py (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the oracle fixture's correlation config.
_correlate()currently pulls the sharedruntimeconfig, so unrelated tuning inruntime/config.jsoncan change this fixture's confirmed/static-only split or downgrade the expectedhighconfidence without any oracle fixture drift. For a golden fixture, pass an explicit local config here (or load one fromoracle/fixtures) so the contract only changes when these fixtures do.Proposed change
+ORACLE_RUNTIME_CONFIG = { + "leak_categories": ["subscription-leak", "idisposable-leak", "region-escape"], + "default_expected": 1, + "min_count": 2, + "high_count": 10, +} + def _correlate(): - findings = json.load(open(FINDINGS, encoding="utf-8"))["findings"] - dump = json.load(open(RUNTIME, encoding="utf-8")) - return C.correlate(findings, dump, C.load_config()), dump + findings = json.load(open(FINDINGS, encoding="utf-8"))["findings"] + dump = json.load(open(RUNTIME, encoding="utf-8")) + return C.correlate(findings, dump, ORACLE_RUNTIME_CONFIG), dump🤖 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 `@oracle/fixtures/test_oracle_runtime.py` around lines 36 - 39, The _correlate() fixture is using the shared C.load_config() runtime configuration, which makes the oracle output sensitive to unrelated config changes. Update _correlate() in test_oracle_runtime.py to load an explicit fixture-local correlation config from oracle/fixtures instead of the shared runtime config, while still passing it into C.correlate(...) alongside findings and dump. Keep the change scoped to the _correlate() helper so the golden oracle contract only changes when the fixture data changes.
🤖 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 `@oracle/LeakyOracle/ViewModels/TickerViewModel.cs`:
- Around line 23-28: The OWN-TIMER fixture is pointing at the wrong location for
TickerViewModel; update the finding in oracle/fixtures/findings.json so it
references the timer allocation inside TickerViewModel instead of the
constructor header. Use the _timer assignment in TickerViewModel() as the anchor
and move the fixture to the line where new Timer(...) is created.
---
Nitpick comments:
In `@oracle/fixtures/test_oracle_runtime.py`:
- Around line 36-39: The _correlate() fixture is using the shared
C.load_config() runtime configuration, which makes the oracle output sensitive
to unrelated config changes. Update _correlate() in test_oracle_runtime.py to
load an explicit fixture-local correlation config from oracle/fixtures instead
of the shared runtime config, while still passing it into C.correlate(...)
alongside findings and dump. Keep the change scoped to the _correlate() helper
so the golden oracle contract only changes when the fixture data changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1d4541a-ef3d-4fe7-8ee6-721566728140
📒 Files selected for processing (9)
.github/workflows/ci.ymloracle/LeakyOracle/LeakScenario.csoracle/LeakyOracle/ViewModels/FixedTickerViewModel.csoracle/LeakyOracle/ViewModels/TickerViewModel.csoracle/README.mdoracle/fixtures/README.mdoracle/fixtures/findings.jsonoracle/fixtures/runtime.jsonoracle/fixtures/test_oracle_runtime.py
…Rabbit) - findings.json: anchor each suspect at its real fix site — OWN-TIMER at the `new Timer(...)` allocation (TickerViewModel.cs:24 -> :27), and (for the same honesty) OWN001 at the `+=` line (WatchlistViewModel.cs:35 -> :32). - test_oracle_runtime.py: pin a local ORACLE_CFG instead of C.load_config(), so this golden depends only on its own fixtures — unrelated tuning in the shared runtime/config.json can no longer shift the confirmed/runtime-only split or downgrade the expected `high`. Mirrors the default; same convention as runtime/tests/test_runtime.py. Correlation keys on the file stem, so the line fixes don't change the split; 6/6 normal + -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
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.
Two complementary in-scope additions that dovetail: (а) gives the oracle a second heap-provable leak, and (б) correlates both leaks through
correlate.py.(а) Timer leak in the oracle
TickerViewModelstarts a recurringSystem.Threading.Timerin its ctor and never disposes it → it stays rooted by the runtime's TimerQueue (own-checkOWN-TIMER; coverage-matrix "Timers").FixedTickerViewModeldisposes it — the control case.LeakScenarionow proves both leaks (subscription + timer) and both fixes through the same WeakReference harness:Stable across repeated runs; builds clean on net8.0, zero warnings.
(б) Runtime correlation golden fixture — phase 5 on real oracle-shaped data
findings.json— the static suspects own-check would emit (OWN001subscription onWatchlistViewModel,OWN-TIMERonTickerViewModel).resourceis a description, so correlation keys on the source-file stem.runtime.json— heap evidence faithful to the leak proof: 50 of each leaky VM retained (expected0), plus the 250 000QuoteRowthey transitively retain.test_oracle_runtime.pyasserts the three-way split:WatchlistViewModel×50 +TickerViewModel×50 — both high, each naming the leaked CLR type + the static rule it corroboratesQuoteRow×250 000 — a static blind spot (rows retained through the leaked VMs)The
highgate fails on the two confirmed leaks. A drift guard ties every retained type to a realoracle/LeakyOracle/ViewModels/*.cs.Scope
Both fixtures pin the contract the слой-2 ClrMD/gcdump collectors (Own.NET) must emit; when those land, these same
arch/+runtime/passes run on extracted data. Wired into CI (normal +-O); all suites green. The .NET oracle build is deliberately not on CI (build-free Python contract) —oracle/tools/leak-scenario.shis the reproducer.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit