feat(examples): the flagship leak in its native habitat — a WPF repro pair, witnessed live on Windows (A2) - #310
Merged
Conversation
…witnessed on Windows The console pair is the shape stripped to its bones; this is the same bug in its native habitat. A DocumentWindow subscribes to the process-lifetime settings hub in its constructor, an unsubscribe exists behind Cleanup(keepAlive), and the Closed handler passes true — so every closed window, with its whole visual tree, stays in the hub's delegate list. The fix variant moves the release into OnClosed, the method WPF itself calls at the end of a window's life. What is platform-bound is only what has to be. ANALYSIS is not: the subscription binds through System.ComponentModel and the release is recognised by teardown name, so gate A runs owen check on the pair on BOTH legs and gets the same verdict without the WindowsDesktop reference pack (verified locally with and without it). The projects compile anywhere (EnableWindowsTargeting), so XAML and code-behind defects surface at desk time rather than on the Windows runner. RUNNING the sample is the part that needs Windows, and that is exactly what the new Windows-only steps do: hold the app live, attach the witness through its public CLI, and require the JSON artifact to name the path AppSettings -> PropertyChanged -> _invocationList -> handler -> DocumentWindow. The ok side is held to the claim the fix actually makes — the hub retains nothing, asserted as the absence of a static-event root — not to "the heap is empty". WPF's own focus and input statics may legitimately hold the last closed window; calling that our leak would be the overreach Owen refuses everywhere else. Holding a sample for a witness needed a second mechanism. A CI runner's stdin is not a console, so the merged ReadLine hold falls straight through and the witness attaches to nothing. Both pairs now share a Hold helper: ReadLine stays the default (scripts/flagship-demo.sh keeps its FIFO, unchanged and re-verified), and setting OWEN_FLAGSHIP_STOP switches to a stop-file release. Either way the hold is bounded by OWEN_FLAGSHIP_HOLD_SECONDS (default 300), so a forgotten sample cannot outlive its job. Verified locally: both WPF projects build on Linux; owen check gives 1/OWN001 on bad and 0 on ok for all four variants; the merged demo script still passes both variants; the new Windows step's control flow (pid parsed from the hold line, stop-file release, witness attach, JSON assertions) was rehearsed end-to-end against the console sample, and both embedded validators were tested to accept the real artifact and reject the opposite one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
…t parked
The first Windows CI round was green and still wrong, which is the more
interesting kind of failure. The bad side reported exactly what it should
(200/200 durably retained, [static-event], 100% through one reference), but
the FIXED sample reported 200 windows retained via [gc-handle] at 41 hops —
after correctly reporting "0 still subscribed". Our release had run; the
windows were still pinned.
The cause was the sample, not WPF and not the witness. `Window.Close()`
finishes through the dispatcher, and the hold parked the UI thread in a
Thread.Sleep loop. That froze WPF mid-teardown, so the witness — faithfully —
photographed framework book-keeping and called it retention. A runtime
witness is only as honest as the moment you take the picture.
Both WPF samples now:
* measure only after the dispatcher has gone idle (the count and the GC
happen in a SystemIdle continuation, not immediately after the loop), and
* hold with the message loop STILL RUNNING — a DispatcherTimer polling the
stop file instead of a blocking sleep.
The console pair keeps its blocking hold: it has no dispatcher, so there is
nothing to starve.
The ok-side CI check gets the same honesty treatment. It asserts only what
the fix claims — no `static-event` root, i.e. the hub retains nothing — and
PRINTS whatever else still holds the type rather than asserting it. A green
check there means "your subscription is gone", never "the heap is empty";
promoting a framework root to a leak would be the overreach Owen refuses in
static analysis, and it should not sneak in through a demo.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
…t that passed anyway With the dispatcher-pump fix in, windows-latest reports what the fix actually achieves: the bad sample is RETAINED via [static-event] with all 200 windows on one reference, and the FIXED sample is ABSENT — every window collected, not one root of any kind. The [gc-handle] retention seen in the previous round was the measurement artifact, not a WPF fact. So the ok-side check is raised to the same user-level contract the console pair already carries: witness exit 0, verdict ABSENT or OBSERVED_ONLY, zero durable roots. Which of the two verdicts appears stays unpinned — that is a GC timing detail, not a public contract — but "something durable holds it" is now a failure instead of a footnote. The previous assertion (no static-event root) was true, narrow, and green while the sample was misbehaving. That is the failure mode worth naming: an assertion scoped tightly enough to survive anything tests nothing. Both directions of the new validator were checked locally — it accepts the real ABSENT artifact and rejects a RETAINED/gc-handle one. README records the correction rather than quietly dropping it: the trap that produced the false reading, why the samples now hold with the message loop running, and that a witness is only as honest as the moment the picture is taken. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds bad and corrected WPF flagship samples demonstrating event-subscription retention, introduces shared hold controls for console and WPF runs, documents the scenarios, and extends CI with static-analysis and Windows runtime witness checks. ChangesWPF retention flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant WPFApp
participant Dispatcher
participant DocumentWindow
participant AppSettings
participant RetentionPath
CI->>WPFApp: start held bad or ok sample
WPFApp->>Dispatcher: schedule open/close cycles
Dispatcher->>DocumentWindow: create, show, and close windows
DocumentWindow->>AppSettings: subscribe or unsubscribe PropertyChanged
CI->>RetentionPath: attach witness to held process
RetentionPath-->>CI: return retention verdict and paths
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62bb43928e
ℹ️ 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".
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 `@examples/flagship/README.md`:
- Around line 59-66: Update the fenced code blocks in the README section around
the dotnet run example and the retention-path diagram to specify language
identifiers: use console for the command/output block and text for the diagram
block. Preserve all existing content inside the fences.
🪄 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: d9f0c849-cab5-425f-a95c-7d5d4ac3b5a2
📒 Files selected for processing (20)
.github/workflows/ci.ymlexamples/flagship/README.mdexamples/flagship/console/bad/DocumentApp.csexamples/flagship/console/bad/Hold.csexamples/flagship/console/ok/DocumentApp.csexamples/flagship/console/ok/Hold.csexamples/flagship/wpf/bad/App.xamlexamples/flagship/wpf/bad/App.xaml.csexamples/flagship/wpf/bad/AppSettings.csexamples/flagship/wpf/bad/BadDocumentWindows.csprojexamples/flagship/wpf/bad/DocumentWindow.xamlexamples/flagship/wpf/bad/DocumentWindow.xaml.csexamples/flagship/wpf/bad/Hold.csexamples/flagship/wpf/ok/App.xamlexamples/flagship/wpf/ok/App.xaml.csexamples/flagship/wpf/ok/AppSettings.csexamples/flagship/wpf/ok/DocumentWindow.xamlexamples/flagship/wpf/ok/DocumentWindow.xaml.csexamples/flagship/wpf/ok/Hold.csexamples/flagship/wpf/ok/OkDocumentWindows.csproj
… just the documented one
Two P2 review findings, both correct, and both the same defect wearing
different clothes: the hold PROMISED a bounded lifetime and a stdin release,
and delivered each only on one path.
* The console helper parsed OWEN_FLAGSHIP_HOLD_SECONDS and then blocked in
Console.ReadLine() forever when no stop file was set — the default path
scripts/flagship-demo.sh uses. The value was computed and never consulted,
while the file header claimed "both paths are bounded by a deadline".
* The WPF helper never read stdin at all, so a reader following the
documented "send a line to exit" would have waited out the full 300s.
Rather than documenting the gaps, the promise is now true. All four samples
share one release contract: a line on stdin, the stop file, or the deadline —
and the deadline applies to every path. Stdin is read on a BACKGROUND thread,
which is what makes the deadline enforceable at all and, in the WPF samples,
keeps the dispatcher pumping (a blocking read there would recreate the frozen-
teardown artifact this branch just finished fixing).
A null read is deliberately NOT a release. With stdin closed or redirected
from nothing — every CI runner — Console.ReadLine() returns null immediately,
and treating that as "the user pressed Enter" would end the hold before a
witness could attach.
Verified locally: stdin at EOF with a 3s deadline holds for 3s and exits
(neither instantly nor forever); the stop file releases in ~1s with stdin at
EOF; the merged demo script still releases through its FIFO promptly (5s
end-to-end) and both variants pass; all four samples keep their owen verdicts;
full suite and ruff green.
Also gives every fenced block in the flagship README a language (MD040,
CodeRabbit), including the pre-existing console ones, so the file is
consistent rather than half-fixed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
PhysShell
pushed a commit
that referenced
this pull request
Jul 26, 2026
The deadline was `DateTime.UtcNow + seconds` — wall-clock arithmetic. A
backwards system-clock adjustment extends it, so the documented guarantee
("a forgotten sample cannot outlive its job") held only while nobody touched
the clock. A bound a clock adjustment can extend is not a bound.
All four samples now measure it with a Stopwatch started at Announce(), and
ShouldRelease() drops its deadline parameter — the callers were each carrying
their own copy of the arithmetic, including the WPF drivers. The stdin-line
and stop-file paths are untouched.
Raised on PR #310 as a non-blocking tail and deliberately deferred out of it
rather than restarting a green run; landing it first here so the claim in the
README is true before the packaging arc quotes it.
Verified locally: stdin at EOF with a 3s deadline exits at 3s; the stop file
still releases in ~1s against a 60s deadline; the demo script still releases
through its FIFO with both variants green; all four samples keep their owen
verdicts (bad → 1, ok → 0).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
14 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 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.
Что и зачем
Второе flagship-репро арки A2: тот же сюжет #278, что и консольная пара, но в родной среде —
DocumentWindowподписывается на статический хаб настроек в конструкторе, отписка существует заCleanup(keepAlive), а обработчикClosedпередаётtrue, так что каждое закрытое окно вместе со всем визуальным деревом остаётся в списке делегатов. Фикс переносит-=вOnClosed— метод, который WPF вызывает сам. Срез самодостаточный: A4 (упаковка) сюда сознательно не добавляется.Платформенно связано только то, что обязано быть. Анализ — нет: подписка связывается через
System.ComponentModel, релиз распознаётся по имени teardown-метода, поэтому вердикт одинаков без WindowsDesktop-ссылок (проверено локально с ref-пакетом и без), а проекты компилируются где угодно (EnableWindowsTargeting) — дефекты XAML и code-behind всплывают за столом, а не на раннере. Windows нужен ровно для одного: запустить образец и подключить к нему свидетеля.Критерии приёмки, которые проверяет CI
Какой именно из двух вердиктов выпадет на ok-стороне, не закрепляется — это деталь тайминга GC, а не публичный контракт; на
windows-latestокна собираются полностью, и читаетсяABSENT.Ловушка замера, стоившая раунда CI
Первый Windows-прогон был зелёным и при этом неверным — самый интересный вид провала. Bad-сторона отчиталась правильно, а исправленный образец показал 200 окон, удержанных через
[gc-handle]на 41 хопе, уже после честного «0 still subscribed». Причина — не WPF и не свидетель, а сам образец:Close()доигрывается через диспетчер, а удержание парковало UI-поток вThread.Sleep. Приложение замерло посреди разрушения окон, и свидетель добросовестно сфотографировал служебные структуры фреймворка. Оба WPF-образца теперь считают только после простоя диспетчера и удерживаются с работающим насосом сообщений (DispatcherTimer); консольная пара сохраняет блокирующее удержание — там диспетчера нет.Второй урок записан в README вместе с первым: изначальная ассерция («нет
static-event-корня») была истинной, узкой и зелёной при сломанном образце. Ассерция, суженная настолько, чтобы пережить что угодно, не проверяет ничего — поэтому ok-сторона поднята до полного контракта выше.Тип изменения
Как проверено
python tests/run_tests.py— весь набор зелёныйruff check .scripts/flagship-demo.sh(оба варианта) после изменения механизма удержанияowen checkдаёт 1/OWN001 наbadи 0 наokдля всех четырёх вариантов (console + wpf), с WindowsDesktop-ссылками и без62bb439строгий контракт подтверждён независимым третьим Windows-прогоном:bad→ RETAINED / 200 durably retained / static-event,ok→ ABSENT / roots seen: noneСвязанные issue
Refs #278 (сюжет репро), #250/#253/#254 (Owen Alpha: это последний инженерный срез арки A2; владельческий гейт #252 остаётся единственным блокером публикации), #270 (runtime witnesses). Упаковка A4 — отдельная ветка после мержа.
Чеклист
examples/flagship/README.md: WPF-пара, контракт ok-стороны, ловушка с заблокированным диспетчером, контракт удержания)feat:,fix:,docs:…)Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation