Teach Bloom to publish its health, and add the Doctor's wire format (BL-16719) - #8259
Teach Bloom to publish its health, and add the Doctor's wire format (BL-16719)#8259JohnThomson wants to merge 2 commits into
Conversation
…BL-16719) The first of two PRs. This one is everything that goes INSIDE Bloom, plus the protocol both sides share; the Doctor process itself follows in a second PR stacked on this branch. Split this way for two reasons. The first is review: at ~19,600 lines the combined change defeated every automated reviewer we have - Devin's analysis never completed on it across four attempts on four different commits. The second is that these two halves carry very different risk. The Doctor is a separate process, and if it is wrong a card is wrong. This half runs inside the shipping application, so it is the half that deserves the scrutiny. What Bloom gains: - FreezeDoctorSupport, which publishes a UI-thread heartbeat and what Bloom believes it is doing into a small shared-memory page, and writes a per-run session file with the facts a watcher cannot work out from outside - above all which log file this run is writing to. - ApiActivityTracker, which records in-flight API requests and which lock each one is waiting on, so a deadlock can be read off a report. - Long-operation scopes at the half-dozen places Bloom legitimately stops answering for minutes (BloomPUB, ePUB, RAB, video, upload, download), which raise the freeze threshold rather than letting a legitimate slow job look like a freeze. - DoctorLauncher, which starts the Doctor if the setting is on, and the debug-menu toggle for that setting. Off by default. - FreezeSimulator, which makes Bloom break itself on purpose so the detection and crash paths can be exercised at all. Refuses to arm on the release channels. The heartbeat is the part worth understanding, because the obvious alternative does not work: a WinForms UI thread blocked in a managed wait still dispatches sent messages, so IsHungAppWindow and Process.Responding report a thoroughly wedged Bloom as healthy - measured at nine minutes frozen and reported responsive. A timer-driven heartbeat in shared memory is the only thing that sees it. BloomFreezeDoctor.Protocol is a plain project both sides reference rather than a package, so one wire format has one definition. Its layout is pinned by value in tests on both sides. Nothing here does anything on its own: with no Doctor installed, Bloom writes a heartbeat nobody reads.
|
| Filename | Overview |
|---|---|
| src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs | Adds heartbeat, session persistence, long-operation accounting, crash-dump signaling, and shutdown-state publication with guarded failure handling. |
| src/BloomExe/FreezeDoctor/ApiActivityTracker.cs | Adds lock-free in-flight request diagnostics; the public disposal method is missing repository-required documentation. |
| src/BloomExe/web/BloomApiHandler.cs | Integrates request scopes and lock-state annotations around the existing synchronized API dispatch lifecycle. |
| src/BloomExe/Program.cs | Starts health publishing and the optional Doctor launcher before the message loop and records ordered shutdown phases afterward. |
| src/BloomExe/FreezeDoctor/DoctorLauncher.cs | Launches the companion executable when enabled and safely no-ops when this first stacked PR does not yet provide it. |
| src/BloomFreezeDoctor.Protocol/DoctorChannel.cs | Defines the shared-memory channel and synchronization behavior used by Bloom and the companion process. |
| src/BloomFreezeDoctor.Protocol/DoctorSession.cs | Defines persistent run and exit records plus retention and atomic session-store behavior. |
| src/BloomBrowserUI/react_components/TopBar/TopBarContextMenu.tsx | Adds a persisted debug-menu toggle whose endpoint matches the new AppApi registration. |
| src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs | Pins protocol constants, layout, signal behavior, session serialization, and long-operation semantics. |
Reviews (1): Last reviewed commit: "Teach Bloom to publish its health, and a..." | Re-trigger Greptile
Found by Devin on #8259 — its first completed review of this work, which is the point of having split the PR. Long-operation scopes do not always nest. A starts, B starts, A finishes, B finishes: on the way out, B restored "whatever was showing when I started", which is A — an operation that had by then been over for some time. A report gathered afterwards therefore named work that had already completed, which is the one thing the activity string exists to prevent. The existing guard handled the other ordering correctly (a scope that no longer owns the slot leaves it alone), so nesting was safe; only overlap was wrong. Devin's suggested fix was to track active scopes. That is more machinery than this needs, and it would have missed the opposite error: falling back to the empty string when the last scope closes silently discards a standing activity set OUTSIDE any scope — and there is one, because recording a video says what it is doing and only then opens a scope to merge the result. So instead the outermost scope remembers what Bloom was saying before any scope opened, and the last one out restores that; anyone closing while others still run restores what they interrupted, as before. Two tests, one per direction, and the overlap one verified to fail when the fix is undone. Bloom-side FreezeDoctor tests 27 pass; full C# suite 3321 pass.
|
[Claude Opus 5 from Thomson's machine during preflight] Consulted Devin — up to
|
|
[Claude Opus 5 from Thomson's machine during preflight] Correction to my comment above. I claimed the split is what got Devin to complete. That is not supported, and I should not have asserted it. Checking the job API for #8229 afterwards, The split still stands on its own merits — this half is 4,349 lines against 15,296, it is the half that runs inside Bloom, and it can be reviewed and merged independently. But "it made Devin work" is not one of them. Everything else in that comment — the bug, the fix, and the two assessments — is unaffected. |
Found by Devin on #8259 — its first completed review of this work, which is the point of having split the PR. Long-operation scopes do not always nest. A starts, B starts, A finishes, B finishes: on the way out, B restored "whatever was showing when I started", which is A — an operation that had by then been over for some time. A report gathered afterwards therefore named work that had already completed, which is the one thing the activity string exists to prevent. The existing guard handled the other ordering correctly (a scope that no longer owns the slot leaves it alone), so nesting was safe; only overlap was wrong. Devin's suggested fix was to track active scopes. That is more machinery than this needs, and it would have missed the opposite error: falling back to the empty string when the last scope closes silently discards a standing activity set OUTSIDE any scope — and there is one, because recording a video says what it is doing and only then opens a scope to merge the result. So instead the outermost scope remembers what Bloom was saying before any scope opened, and the last one out restores that; anyone closing while others still run restores what they interrupted, as before. Two tests, one per direction, and the overlap one verified to fail when the fix is undone. Bloom-side FreezeDoctor tests 27 pass; full C# suite 3321 pass.
|
[Claude Opus 5 from John Thomson's machine during preflight] Closing this. It was a temporary split, made to give the automated reviewers a smaller diff, and it has served its purpose — Devin reviewed it and found one real bug (overlapping long operations leaving finished work on display), which is fixed. The Freeze Doctor goes back to being a single pull request, #8229, which carries all the machine and human review history. #8229 now targets Nothing is lost by closing this: every commit that was only on this branch is in #8229, and I verified the Doctor's own code is byte-identical after the rebase. The one thing deliberately dropped is master-only work that came along when this branch was cut from |
1 participant
This is the first of two stacked PRs. It contains everything that goes inside Bloom, plus the wire format both sides share. The Doctor process itself is #8229, which targets this branch.
Problem
When Bloom freezes, we get almost nothing. The user kills it and carries on, and freezes are exactly the failure people don't report — so a bug like BL-16697 can only be guessed at. Worse, Bloom cannot reliably tell us it is frozen, and nor can Windows: a WinForms UI thread blocked in a managed wait still dispatches sent messages, so
IsHungAppWindowandProcess.Respondingreport a thoroughly wedged Bloom as healthy. That was measured at nine minutes frozen and reported responsive.What this PR changes
Bloom starts publishing enough about itself that a separate process can tell it has stopped, and can say something useful about why:
Log.txteach run and falls back to a random name only when another Bloom holds it, so from outside the newest log is the wrong answer exactly in the restart-after-a-freeze case.ApiActivityTracker) recording which requests are running and which lock each is waiting on, so a server deadlock can be read off a report rather than reproduced.DoctorLauncherand a debug-menu toggle, which start the companion process when the setting is on. Off by default.FreezeSimulator, which makes Bloom break itself on purpose so the detection and crash paths can be exercised at all. It refuses to arm on the release channels.BloomFreezeDoctor.Protocolis a plain project both sides reference rather than a published package, so one wire format has one definition instead of two hand-maintained copies. Its layout is pinned by value in tests on both sides.Nothing here does anything on its own. With no Doctor installed — the default — Bloom writes a heartbeat that nobody reads.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16719
Why the split: at ~19,600 lines the combined change defeated every automated reviewer we have — Devin's analysis never completed on it across four attempts on four separate commits. It also mixes two very different risk profiles: the Doctor is a separate process, so if it is wrong a card is wrong, whereas this half runs inside the shipping application. This is the half that deserves the scrutiny.
Devin review
This change is