Add the Bloom Freeze Doctor (BL-16719) - #8229
Conversation
|
| Filename | Overview |
|---|---|
| src/BloomFreezeDoctor.Core/Outbox/ReportOutbox.cs | Implements persistent report queuing and cross-process drain serialization; the latest lock-timeout fix now exits without entering the drain when another process holds the gate. |
| src/BloomFreezeDoctor/DoctorSupervisor.cs | Coordinates process discovery, evidence gathering, report-now requests, queued submission, and watcher lifecycle without a remaining blocking issue from the reviewed threads. |
| src/BloomFreezeDoctor.Core/FreezeDetector.cs | Implements heartbeat-based freeze, crash, and windowless-process classification with slow-activity and debugger handling. |
| src/BloomFreezeDoctor.Protocol/DoctorChannel.cs | Defines the shared-memory health protocol and compatibility behavior used between Bloom and the Doctor. |
| src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs | Publishes Bloom health, activity, debugger, shutdown, logging, and diagnostic-signal state to the companion process. |
| src/BloomFreezeDoctor.Core/Outbox/YouTrackSubmitter.cs | Submits queued diagnostic reports and attachments to YouTrack while supporting fingerprint-based report consolidation. |
| src/BloomFreezeDoctor.Tests/ReportOutboxTests.cs | Covers outbox persistence, submission limits, retention, and refusal of a second drain while another process owns the lock. |
| build/Bloom.proj | Builds, signs, and packages the Freeze Doctor alongside Bloom. |
Reviews (15): Last reviewed commit: "Stop the comments pointing at the abando..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Both found by Devin on PR #8229, and both verified against the code before changing anything. THE 6.3 WEBVIEW2 FALLBACK COULD NEVER FIRE FindCdpPort ended with return children.Count == 0 ? null : (int?)null; Both arms are null, so the documented fallback to the port 6.3 hardcodes was dead code. A Bloom 6.3 with no session file therefore got no CdpPort, and its WebView2 section was dropped from the report with nothing to say why. The guard around it was right and is kept: WebView2 children that advertise no port mean this is NOT the 6.3 arrangement, and guessing 9222 would interrogate some other program's browser and put the answers on a Bloom card. It is the no-children case that looks like 6.3, and that now returns WebView2Processes.LegacyHardcodedPort as the comment always said it did. FOUR DEDUP SETS MUTATED FROM SEVERAL THREADS AT ONCE _dumpsRequested, _exitsExamined, _zombiesReported and _zombiesEnded are shared across watchers, but every watcher raises Observed on ITS OWN timer thread, so with two Blooms being watched - routine on a developer's machine - they were touched concurrently with no lock. An unsynchronised HashSet can corrupt itself or throw, and worst of all can silently lose an entry, which is what these sets exist to prevent: a second dump of a crashing Bloom while it holds its own death open for us, or a second report of one exit. Telling detail: one of the five call sites, the _zombiesReported.Add after a zombie report is queued, ALREADY held _lock. So locking these was the intention and the other sites were simply missed. Each test-and-claim is now inside _lock, and only the test-and-claim - never the work that follows, which includes gathering and killing processes. The zombie pair is under a single lock together, because "the evidence is gathered AND nobody else has taken the one attempt" is one decision; splitting it would let two threads both conclude they were the one to end the process. DumpRequested() was pulled out of its short-circuit so it is not called while holding the lock. 110 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by Greptile on PR #8229, and correct. Three separate things ask for a drain - startup, the five-minute timer, and the end of a gather - and all three were fire-and-forget tasks, so two could overlap. Both would then list the SAME pending bundles and both walk the search-then-create flow against YouTrack, which is not atomic. The results are duplicate cards, or duplicate comments on one card, and a combined total that can exceed the deliberate three-per-day cap. That cap exists so that a machine in a bad state cannot spam the tracker, so quietly exceeding it is the worst version of this. Neither the supervisor nor ReportOutbox had any mutual exclusion, so a SemaphoreSlim(1,1) now serialises it, and it is disposed with the supervisor. It WAITS rather than skipping. Skipping would be cheaper and is tempting, since a second concurrent drain would usually find nothing left to do - but ReportNowAsync awaits this and then looks for its own bundle in the queue. Had its drain been skipped because another was already running, it could have reported failure for a report that was in fact about to be filed perfectly well. 110 tests pass.
Found by Devin on PR #8229, and it is the exact failure _workInFlight exists to prevent - on the one path that never got the guard. The field's own comment says why it is there: a Bloom would crash, the Doctor would notice the process was gone, conclude there was nothing left to watch, and exit, cancelling the examination of the very crash it had just seen. It is incremented for the report-gathering job and for the exit examination. The crash DUMP job never incremented it, so ConsiderExiting could see zero work in flight and shut the Doctor down mid-dump. This is also the likeliest path for it to happen, not the least: the dump runs because Bloom is in the act of crashing, so the process is about to disappear - which is precisely the event that makes the Doctor look around and find nothing left to watch. And it is the worst one to lose, because Bloom is holding its own death open for about three seconds waiting for us, so the dump either happens now or never. The decrement goes after SignalDumpComplete, so Bloom is released before our bookkeeping rather than after it. 110 tests pass.
776a56e to
7269268
Compare
|
[Claude Opus 5 during preflight] Consulted Devin up to Devin had timed out on this PR five times over two days — jobs reported Most of it was already dealt with. Devin itself marks 7 of the 10 bugs resolved by earlier commits on this branch. Of what was still live:
Every current Bug and Investigate flag has its own thread above with its outcome. The informational flags are not mirrored. Greptile has reviewed this branch 15 times and its threads are all resolved; its most recent review predates today's commits. CI is green. One thing Devin cannot tell us, and says so itself: the crash-dump path is still unexercised end to end. |
A probe running in a separate process, calling the identical
Process.GetProcessesByName("Bloom") once a second, settles the question the last
five runs could not:
Bloom 81632 really started at 16:51:48.595
the same API first saw it at 16:51:48.947 (+0.35s)
the Doctor adopted it at 16:52:02 (+14s, sweep 82)
So nothing is blind. A new Bloom is visible to that call within 350 milliseconds,
and the whole delay is the sweep's own latency. It also explains the range: the
sweep counter shows sweeps running 5.0 seconds apart while idle and 12.75 seconds
apart while Bloom was starting, so "wait up to one sweep" IS fourteen seconds, and
the 96-second case is several sweeps at that cadence or a few much slower ones
during the build.
That leaves one question - where a sweep spends twelve seconds - and two suspects
with nothing to choose between them by reading:
- looking for Bloom: one enumeration of every process on the machine;
- the bookkeeping afterwards: RaiseStatusChanged calls Pending(), which lists the
outbox directory and deserialises every meta.json in it, on EVERY tick, and has
nothing to do with discovery.
The second is the obvious suspect and would explain the load-dependence, since it
is disk-bound and a Bloom build saturates the disk. But it is a guess, and this
session has had enough of my guesses stated as findings, so the sweep is now timed
in those two halves and reports when it takes longer than the interval between
sweeps. Durations are bucketed coarsely before the repeat-suppressor sees them, so
repeated slow sweeps read as one shape of slowness rather than a fresh number every
five seconds.
Instrumentation only. One run should now name the half.
242 tests pass; the exe builds.
…-16719)
The instrumentation named the half, and it was not the one I predicted:
sweep 31 took 18905ms, longer than the 5s between sweeps:
18903ms looking for Bloom, 2ms on bookkeeping afterwards
My outbox theory was wrong - the bookkeeping is 2 milliseconds. And it is not the
process enumeration either, which measures 8-22ms and which the sweeps that found
nothing all did in under five seconds. The difference on sweep 31 is that it FOUND
Bloom, and finding it meant describing it, and describing it meant a WMI
Win32_Process query for the command line. For a process a few hundred milliseconds
old, on a machine busy launching it, that query took 18.9 seconds.
Caching it, two commits ago, could not help: the FIRST call is the one on the
adoption path.
The command line is wanted for exactly one decision - is this one of Bloom's
console verbs, which legitimately have no window and would otherwise earn a false
zombie report. That decision does not have to be made before adopting. So:
- describing a process no longer reads the command line at all;
- adoption happens on the strength of the process name;
- a background task then reads the command line and LETS GO of the Bloom if it
turns out to be a headless run;
- gathering reads it for the report, which is the right place to pay: gathering
already takes seconds and is racing nothing.
Safe because nothing is reported quickly. The freeze rules want twenty seconds of
silence before they even suspect, sixty before they report, and the zombie rule
thirty - so letting go after a few seconds, or even nineteen, lands well inside.
The release checks it is still the same Bloom before dropping it, since by then that
one may have gone and another been adopted.
This also matters beyond the delay itself, and John made the point: adoption
latency is the window in which Bloom's startup problems cannot be doctored at all,
and it is why one run lost a crash dump - Bloom asks to be dumped only if a Doctor
is already watching, and it asked twenty seconds before we had noticed it existed.
Not the whole answer. A poll still costs up to one interval, which is what a Bloom
too old to announce itself will always be stuck with - worth keeping, per John,
because those are the Blooms that most need watching. A signal from Bloom on
startup would make adoption immediate for everything current; that is the next
piece, not this one.
242 tests pass; the exe builds.
…lease" (BL-16719) **1. Adoption no longer waits for a poll.** Bloom already starts a Doctor when none is running, but a Doctor that is ALREADY running never learned a new Bloom existed: the one Bloom starts is a duplicate, which exits on the singleton mutex without telling the original anything. So adoption waited for the next sweep - five seconds now that WMI is off that path, but five seconds in which Bloom's own startup cannot be doctored at all. That is not hypothetical: on one measured run Bloom crashed and asked to be dumped twenty seconds before the Doctor had noticed it, and since Bloom only asks when a Doctor is already watching, the dump was never taken. So Bloom announces itself on a named event and the Doctor waits on it, using the same mechanism that already carries the dump request. The signal is the only one with no process id in its name, and cannot have one: the Doctor is waiting before it knows which Bloom will appear. It is also the only one Bloom sets without first checking whether anyone is listening, because there is nobody to check for - hence Announce, which creates the event if needed, rather than TrySignal, which reports "nobody there". A manual-reset event set into an empty room is still set when a Doctor arrives. Polling stays, and must: a Bloom too old to know about any of this cannot announce itself, and John's point stands that those are the ones most worth watching. This is an accelerator on top, not a replacement. If the announcement lands while a sweep is already running, the re-entrancy guard makes it a no-op - correct rather than a missed chance, since that sweep is doing the work being asked for, and the timer covers the sliver where a sweep listed the processes just before ours appeared. **2. A path we could not read is no longer called "Release".** DeriveFromExePath fell through to Release for anything unmatched, including an empty path - so a Bloom whose executable we could not read was labelled Release on its card, in the log, and inside the fingerprint, where it merged with genuine Release reports. Seen in a real log as "watching Bloom 25736 (Release)" for processes that were nothing of the kind. Only the empty case changes: a path we can read but do not recognise is still Release, because that is what an ordinary installation looks like and that is the answer the method exists to give. 244 Doctor tests pass (was 242), the 27 Bloom-side Freeze Doctor tests pass, and both executables build - BloomExe as well this time, since this is the first change in a while to touch it.
…L-16719) Three faults in the announcement I added an hour ago, all found by running it. **It spun.** The Doctor waits with ThreadPool.RegisterWaitForSingleObject, which re-arms before the callback runs, so a MANUAL-reset event fires the callback over and over until something resets it - and my Reset in the callback was racing that re-arm rather than preventing it. One announcement produced 103 wake-ups and 103 needless sweeps. A pulse wants an auto-reset event, where one Set releases exactly one wait and the kernel does the reset atomically. Both sides must create it the same way, since the first creator fixes the mode, so Announce creates it that way too. Now: one announcement, one wake-up. **It was gated on the wrong thing.** I put it after the RunFreezeDoctor check, so it only fired when Bloom was also configured to START a Doctor. That setting governs starting one, not cooperating with one already watching - nothing else here consults it either, since the heartbeat and the dying request for a dump both key off whether a Doctor is listening. The case it excluded is the one that matters most: support asking a user to start a Doctor by hand, which is also every Doctor in this session's testing. **It was too late to be worth anything.** Announced from where the Doctor is launched, near the end of Program.Main, it was measured arriving 6.2 seconds into startup - after the five-second sweep had already found Bloom. It is now the first thing Main does after Logger.Init, which it can be because it needs nothing: one named event, set, return. Measured at about 1.3 seconds now, that being .NET startup rather than anything we control, so worst-case adoption goes from 5 seconds to under 1.5. The point is not the number but what the number covers: everything before the announcement is time in which a hang or crash cannot be doctored at all, because Bloom only asks for a dump when a Doctor is already watching. And a correction to my own comment, which claimed an announcement into an empty room would still be waiting when a Doctor arrived. It will not: a Windows named event lives only while a handle is open, so create-set-dispose destroys it on the way out. Observed - a Bloom announcing 9 seconds before a Doctor started was never heard. Not a hole, since "no Doctor running" is the case Bloom handles by starting one, which then finds Bloom by sweeping. Verified end to end on a real run: adoption, dump requested and taken, exit examination standing down, one report filed. 244 tests pass; both executables build.
**Stop the Doctor before building.** A running Freeze Doctor loads BloomFreezeDoctor.Core.dll and .Protocol.dll from output/Debug/AnyCPU and holds them open, so any build that has to refresh them fails with MSB3027. That is not confined to building the Doctor: BloomExe's build copies Protocol.dll, so a running Doctor stops Bloom building at all. It cost John a whole test run - the build failed, Bloom never started, and the only symptom was a Doctor sitting there watching for a Bloom that could not come. Killing it is safe: reports are written to the outbox on disk as they are gathered, an interrupted send is picked up by the next drain, and Bloom starts a new Doctor moments later if the user has it on. Two things about that were got wrong first, both worth recording because both were invisible in the code: - It ran with stderr PIPED and never read. "close" waits for every stdio stream to end, so the promise never settled - and since the launcher awaits it, Bloom was never started at all. A convenience that can hang the launcher is worse than the problem it solves. It now ignores stderr, resolves on "exit", and has a timeout that cannot be missed. - Then it ran, but timed out. `taskkill /IM` walks the whole process table, and called just before the build it was doing that while Vite, LESS and seven file watchers were all starting; it needed longer than the five seconds allowed. It is now the first thing main does, while the machine is still quiet, where it returns at once. It only ever needed to happen before the BUILD, not immediately before. **--nowatch** runs Bloom under `dotnet run` instead of `dotnet watch run`. C# edits then do not rebuild by themselves, which is the right trade whenever you are not editing C#. The front end is untouched - Vite still serves it, so TypeScript and LESS remain live. Measured, twice, on a warm build: 35 seconds to a running Bloom with watch, 27 without. A real gain and worth having, but I should say plainly that it is not the halving we hoped for - most of the wall clock is Vite starting and Bloom itself, not the watcher. The saving should be larger on a cold or changed build, and it also stops dotnet watch competing for the machine for the rest of the session.
…e (BL-16719)
Found by running the simulated failures through a Doctor that Bloom launched
itself and reading what came out. For a SPIN the report named the thread burning a
whole core - correct and useful - and then said nothing whatever about where it
was spinning, which is the only thing a developer needs. Two faults behind that:
**The UI thread was not recognised at all.** It was found by looking for a
"RunMessageLoop" frame, and on a thread that is RUNNING rather than waiting the
stack walk yields "(native)" where those frames should be. So the one failure mode
where the UI thread IS the story had no UI-thread section in its report. It is now
also recognised by Bloom.Program.Main at the base, which survives when nothing
above it does, and which no other thread has.
**And recognising it would have made the report worse.** The old description fell
through to the first frame beginning "Bloom.", which on such a stack is
Bloom.Program.Run - the bottom of every UI thread there has ever been. The report
would have announced "The UI thread is blocked in Bloom.Program.Run": wrong twice,
since it is not blocked and that frame means nothing. A stack that cannot be read
is a fact worth reporting as one, so it now says so and names the thread to open
the attached dump at.
The two headlines then corroborate each other instead of one being absent:
- The UI thread's stack could not be read - which is itself a clue, because a
stack walk fails on a thread that is RUNNING far more often than on one that is
waiting. Thread 109948 in the attached dump is the one to open.
- Thread 109948 is using a whole core, so this looks like a spin rather than a
deadlock.
The reading is now a pure function with its own tests, built on two REAL stacks
copied from reports - one blocked, one spinning - rather than stacks written to
suit the code. Verified by re-running the simulation.
250 tests pass.
Running the simulated zombie and reading the result. Bloom alive, pumping, window
gone - and the report said:
Verdict: alive with no visible window for 31s
No thread is burning CPU, so this is a wait rather than a spin.
WebView2 answers normally, so the block is in Bloom's .NET UI thread, not the browser.
There is no block. The last line asserts one, and a reader could easily come away
hunting a deadlock instead of asking where the window went - which is the actual
question, and one the report answers a few lines later by listing both of Bloom's
windows as hidden.
The cause is that IsAboutAFreeze counts a zombie as a freeze, and several
collectors use it to decide whether an observation supports a CONCLUSION. For a
frozen UI thread "no CPU, so a wait not a spin" is a real deduction; for a zombie
it is a non sequitur about a thread that is running perfectly well.
So the concept is split. IsAboutTheUiBeingStuck covers only the three states where
the UI thread is actually suspect, and the two deductions use that. IsAboutAFreeze
stays where the distinction being drawn is merely live-versus-dying - notably "the
UI thread is in its message loop", which on a zombie report is not a reassurance
but the key finding: Bloom is fine, its window is not.
The zombie report now reads:
Verdict: alive with no visible window for 30s
The UI thread is in its message loop (idle or pumping).
WebView2 answers normally.
Also, while in here: the UI-thread section heading now carries the thread's OS id.
Every other section that mentions a thread uses it - the wait chains name "thread
83756", the CPU table "thread 109948" - so without it the reader cannot tell
whether the thread in the wait chain IS the UI thread, which is usually the whole
question. Noticed on the mutexchain run, whose wait chain named a thread the report
never connected to anything.
250 tests pass. Verified by re-running the simulation.
…L-16719)
Running the simulated unhandled exception. The Doctor caught it exactly as
designed - Bloom's fatal handler asked to be dumped, the dump was taken, the stack
showed the whole crash path - and the report's headlines were:
Verdict: Bloom was crashing and asked to be dumped before it died
The UI thread is blocked in System.Threading.WaitHandle.WaitOneCore.
Not one word about WHAT was thrown, which for a crash is the first question anybody
asks. The answer was in the report all along, 370 lines down inside the tail of
Bloom's log:
exception = System.ApplicationException: FreezeSimulator was asked to throw
A fact that is present but unfindable is barely better than a missing one, and
headlines are what a reader reads. BloomsOwnException pulls the last exception out
of the log tail and the report now leads with it.
The other headline was worse than useless. The UI thread WAS blocked in
WaitHandle.WaitOneCore - inside Bloom's fatal handler, waiting for the Doctor to
finish the dump. That is this tool doing its job, reported as though it were the
fault, and it invites a reader to go hunting a deadlock. It now says what it is:
the UI thread is in Bloom's own fatal-error handler waiting for this dump, so the
stack below is the path to the crash rather than a deadlock.
The report now opens:
Verdict: Bloom was crashing and asked to be dumped before it died
The UI thread is inside Bloom's own fatal-error handler, waiting for this dump -
so the stack below is the path to the crash, not a deadlock.
WebView2 answers normally.
Bloom's own error handling recorded: System.ApplicationException: FreezeSimulator
was asked to throw
Tests use a real log tail, and cover the traps: taking the LAST exception when a
session logged several, not swallowing the stack that follows it, cutting a message
long enough to wreck a headline, and refusing to announce an exception when the
marker has nothing after it.
256 tests pass. Verified by re-running the simulation.
…16719)
**FailFast now says why.** It was the one crash kind whose report never stated
what went wrong. It runs no managed handlers by design, so there is no dump and
nothing in Bloom's log - the .NET Runtime event is the sole record, and its
"Message:" line was sitting a hundred lines down in the evidence while the
headlines said only that the process had called FailFast. The report now opens:
Verdict: Bloom crashed: ... it called FailFast (0x80131623)
Bloom called FailFast: FreezeSimulator was asked to fail fast
Only when Bloom's own log has not already supplied a headline, which is a closer
account of itself than Windows' record of it.
**And "Send it anyway".** John asked for a way to really send a report from a dev
build. The Doctor declines to file on a developer build, on an automation run, and
when the failure was simulated on purpose; all three are right by default, and all
three are ones a developer sometimes wants to override for the report in front of
them. Until now the only way was "Report now", which gathers a WHOLE NEW report -
impossible once the Bloom in question has died, and about a different moment even
when it has not.
The button appears beside "Show report" whenever a report was saved and not sent,
and it asks first, naming the project. That matters: a Doctor that Bloom started
uses the ordinary project rather than the test one - being a developer build is
precisely why the report was held back - so this is not a rehearsal, and the person
clicking should know before rather than after.
ReportOutbox.SendThisAfterAll lifts only a deliberate refusal. Anything already
pending, uploading or filed is left exactly as it is: this is not a way to re-send,
and a button that quietly re-sent a filed report would make duplicate cards from
one click. Tested, including that it declines a bundle it should not touch and
reports failure for a folder that is not there.
262 tests pass. Both headline changes verified by re-running the simulations.
Found reviewing my own change from an hour ago. The new "Bloom's own error handling recorded: ..." headline was not gated to crash reports, and the log tail it reads is a couple of hundred lines of a real session - which routinely contains handled exceptions that came to nothing. On a FREEZE report that would put one of them at the top as though it explained the freeze. A plausible wrong answer in the headlines is worse than no answer, because the headlines are where the reader starts and a named exception is exactly the kind of thing they would go and investigate. Now only on reports that are not about a freeze, which is where it earns its place: a crash, where what was thrown is the first question anybody asks. On a freeze the log is still in the section below, unchanged. 262 tests pass.
Two conflicts, both where base and this branch added to the same place. AppApi.cs and the debug menu: base added the "Use dev.BloomLibrary.org" choice exactly where this branch added the "Run Freeze Doctor" toggle - the endpoint registration, the React state, the mount-effect read, the menu item and the useMemo deps. Independent features throughout; both kept. One of those needed more than keeping both sides. Base refactored the menu from `return [ ... ]` into `const items = [ ... ]` followed by a conditional push and `return items;`, so that it could add its item only on the builds that offer it. The merge had already taken base's opening line, which left this branch's items inside an array nothing returned. Resolved to base's structure, with the Freeze Doctor item where it was in the literal - so the array is built once, base's item is pushed when that build allows it, and the whole thing is returned. Typecheck clean, BloomExe builds, 262 Doctor tests pass on the merged tree.
Decision from the preflight report: leave it as it is. John's words - "we want to catch all the real freezes we can, including older Blooms that happen to be adopted." Written down next to the code because it reads like a bug and is not. A Doctor Bloom launched is not narrowed to the channel that started it, so once that Bloom has gone it will take up whatever other Bloom it finds - which is what you want when you restart Bloom after a freeze, and is also how a Doctor started by a development build came to adopt the BetaInternal install that happened to be running. That one is not a developer channel, so a genuine freeze in it would have been filed as a real card about work nobody asked us to watch. Accepted: a freeze noticed by accident is still a freeze we would otherwise never have heard about, which is the whole point of the tool. Also noted in the card's tester notes as expected behaviour, so a card about a Bloom that was not being deliberately tested is not filed as a mix-up. 262 tests pass.
John took apart the reasoning I had written for leaving it ungated, and he is right. The claim was that support may have had a user start a Doctor by hand with the setting off. Both support routes are covered without ungating: - told to switch the Doctor on and relaunch Bloom, the setting is on by the time Main runs; - told to start a Doctor while Bloom is already running, Main has long since finished and it is the Doctor's own sweep that finds Bloom. What ungating actually bought was at most one poll interval - five seconds - in the narrow case of a Doctor left running while the setting is off. Against that it ran a line in the startup of every user who has never switched the feature on. That is a bad trade, and I made it on a scenario that does not hold. Also moved from the top of Main to just after CheckForCorruptUserConfig, because it now reads a setting and that call is what makes reading one safe. The cost is microseconds and it is still far earlier than the 6.2 seconds measured when the announcement lived down beside the launcher. Verified with a Doctor already running: Bloom started at 09:41:10.244, the Doctor logged "a Bloom has just started and said so" and adopted it within the same second. 262 Doctor tests pass; BloomExe builds.
|
Too many files changed for review (110 files, 100 file limit). Bypass the limit by tagging |
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk partially reviewed 110 files and all commit messages, made 13 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 35 unresolved discussions (waiting on JohnThomson).
build/Bloom.proj line 517 at r19 (raw file):
no longer has one
Never had one, so this seems worth cleaning up or just removing
src/BloomExe/FreezeDoctor/DoctorLauncher.cs line 102 at r19 (raw file):
// notification area": a window appearing every time you started Bloom would get the
// Doctor uninstalled inside a week.
Not sure what this means. Is the AI just being cheeky? The user can't even uninstall it, can they (without uninstalling Bloom)?
src/BloomFreezeDoctor.Core/BloomChannel.cs line 76 at r19 (raw file):
whether the Doctor will ship enabled for them
/// is not settled
Is that true?
I'm unsure what this means for one-off channels like BloomFAL. Maybe we don't care today.
Bloom.sln line 33 at r19 (raw file):
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "WebView2PdfMaker", "src\WebView2PdfMaker\WebView2PdfMaker.csproj", "{F697DD7A-2D74-4850-8381-7E4ADB1E4431}" EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{827E0CD3-B72D-47B6-A68D-7590B98EB39B}"
Adding "src" as a project can't be right, can it?
Bloom.sln line 40 at r19 (raw file):
EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BloomFreezeDoctor", "src\BloomFreezeDoctor\BloomFreezeDoctor.csproj", "{DB569C6F-CD1A-4F6C-AD1D-5768749F2B68}" EndProject
Do we really want/need 3 new production projects for this?
build/check-csharp-robustfile.sh line 42 at r19 (raw file):
# excused, because a diagnostic tool writing to a user's disk has no more business losing a file to # a passing virus scanner than Bloom does - and the first thing the conversion found was a real # one, an outbox rename that failed about one run in three and threw away a gathered report.
Not sure the comment is helpful. I would just drop it.
src/BloomExe/BloomExe.csproj line 239 at r19 (raw file):
<!-- The protocol Bloom and the Bloom Freeze Doctor share: the shared-memory health page, the session file, and the named events. This was three files copied into this repo by hand, 750
Again, the comment only makes sense in light of development churn. Worth cleaning up.
src/BloomExe/FreezeDoctor/DoctorLauncher.cs line 49 at r19 (raw file):
/// /// Gated on <see cref="Settings.RunFreezeDoctor"/>, so it costs nothing at all for the users who /// have never switched the Doctor on. It briefly was not, on the argument that support may have had
No reason to keep the historical comment, imo.
Unless you want to restate it more simply as, we decided not to x because y.
src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs line 622 at r19 (raw file):
/// are not the ones we care about. It does also refresh the session file, which is cheap and keeps /// that work off the UI thread. /// </summary>
I think this summary (above) is vestigial.
src/BloomExe/WebLibraryIntegration/AccessKeys.cs line 46 at r19 (raw file):
if (BookUpload.IsDryRun) return new AccessKeys(null, null); return UploaderDev(lines);
I think we better discuss this.
If I understand what is happening, it is using credentials we are about to shut down.
src/BloomFreezeDoctor/BloomFreezeDoctor.csproj line 1 at r19 (raw file):
<Project Sdk="Microsoft.NET.Sdk">
For all the new files in new projects, I pretty much just skimmed. And that may be too strong a word for it...
src/BloomFreezeDoctor.Tests/AlreadyFrozenTests.cs line 4 at r19 (raw file):
using NUnit.Framework; namespace BloomFreezeDoctor.Tests;
Just noting that I didn't really review any of these tests. Let me know if there is something I should look at.
src/BloomFreezeDoctor.Tests/WhereTheFileStillIsTests.cs line 34 at r19 (raw file):
[Test] public void It_says_where_the_file_is()
These test names don't seem to fit our normal C# unit test naming.
Seems like we do something to prevent all these FreezeDoctor tests from running every time. Mark as Nightly or make a new category?
…convention (BL-16719) Comments across the Freeze Doctor projects, their project files, Bloom.proj and the RobustFile check script now describe the code as it is rather than how it got here: plan-section and spike references, measurement logs, "used to" and "no longer" narratives, and name-and-date attributions are gone, with the rationale for each design choice kept in present tense. About a dozen doc comments that had drifted away from their members are back on the methods they describe. The "uninstalled inside a week" figure of speech now just says a window on every start would be annoying, and the channel comment in BloomChannel.cs records that the Doctor is part of the next Beta with Release still open. Every test in BloomFreezeDoctor.Tests and BloomTests/FreezeDoctor is renamed to Method_Scenario_Result, and ten theme-named fixtures are renamed after the class they exercise (e.g. OneBloomAtATimeTests -> DoctorSupervisorTests). Test bodies are unchanged apart from two stale assertion messages. Bloom.sln no longer nests the Doctor projects under a "src" solution folder. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…-in-repo # Conflicts: # PAPERCUTS.md
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson+AGNT made 9 comments.
Reviewable status: 24 of 110 files reviewed, 35 unresolved discussions (waiting on andrew-polk and JohnThomson).
Bloom.sln line 33 at r19 (raw file):
Previously, andrew-polk wrote…
Adding "src" as a project can't be right, can it?
[Claude Fable 5.1] You're right. It was an artifact of dotnet sln add, which mirrors the on-disk directory as a solution folder unless told otherwise. Removed in 34f5ea1; the four Doctor projects now sit at the solution root beside BloomExe, BloomTests and WebView2PdfMaker.
build/Bloom.proj line 517 at r19 (raw file):
Previously, andrew-polk wrote…
no longer has one
Never had one, so this seems worth cleaning up or just removing
[Claude Fable 5.1] Reworded in 34f5ea1. The comment now just says the Doctor has no installer of its own and ships inside Bloom's, so this is the only place it gets signed.
build/check-csharp-robustfile.sh line 42 at r19 (raw file):
Previously, andrew-polk wrote…
Not sure the comment is helpful. I would just drop it.
[Claude Fable 5.1] Dropped in 34f5ea1. The exemption line now stands bare, matching the BloomTests one above it.
src/BloomExe/BloomExe.csproj line 239 at r19 (raw file):
Previously, andrew-polk wrote…
Again, the comment only makes sense in light of development churn. Worth cleaning up.
[Claude Fable 5.1] Trimmed in 34f5ea1 to what the reference is and why it is a ProjectReference rather than a package. The same commit removed development-history narrative across all the Doctor's source, project files and tests, so the other instances of this pattern should be gone as well.
src/BloomExe/FreezeDoctor/DoctorLauncher.cs line 49 at r19 (raw file):
Previously, andrew-polk wrote…
No reason to keep the historical comment, imo.
Unless you want to restate it more simply as, we decided not to x because y.
[Claude Fable 5.1] Removed in 34f5ea1. The comment now states only the current rule: the announcement is gated on the setting so it costs nothing for users who never switched the Doctor on, and a Doctor started by hand still finds Bloom by its own sweep.
src/BloomExe/FreezeDoctor/DoctorLauncher.cs line 102 at r19 (raw file):
Previously, andrew-polk wrote…
// notification area": a window appearing every time you started Bloom would get the// Doctor uninstalled inside a week.Not sure what this means. Is the AI just being cheeky? The user can't even uninstall it, can they (without uninstalling Bloom)?
[Claude Fable 5.1] Agreed, it was a figure of speech and a confusing one. Both places that used it (here and in StatusForm) now just say that a window appearing every time you start Bloom would quickly become annoying. Fixed in 34f5ea1.
src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs line 622 at r19 (raw file):
Previously, andrew-polk wrote…
I think this summary (above) is vestigial.
[Claude Fable 5.1] It was the heartbeat method's summary, which had drifted away from WatchdogLoop. It is back on that method now, and about a dozen similar strays elsewhere in the Doctor's files were reattached in the same commit (34f5ea1).
src/BloomFreezeDoctor.Core/BloomChannel.cs line 76 at r19 (raw file):
Previously, andrew-polk wrote…
whether the Doctor will ship enabled for them
/// is not settledIs that true?
I'm unsure what this means for one-off channels like BloomFAL. Maybe we don't care today.
[Claude Fable 5.1] Updated in 34f5ea1: the Doctor is part of the next Beta, and whether to offer it to Release users is still open. The comment also now says explicitly that the filing guards do not distinguish Beta or Release from any other installed channel.
On one-off channels like BloomFAL: the Doctor derives the channel name from the install folder, so a BloomFAL install reads as channel "FAL". BloomFAL is not in the list of process names the Doctor sweeps for, but Bloom launches the Doctor with its own process id, and adopting a process adds that process name to the sweep set, so a BloomFAL user who switches the Doctor on is watched like anyone else and their reports carry FAL as the channel.
src/BloomFreezeDoctor.Tests/WhereTheFileStillIsTests.cs line 34 at r19 (raw file):
Previously, andrew-polk wrote…
These test names don't seem to fit our normal C# unit test naming.
Seems like we do something to prevent all these FreezeDoctor tests from running every time. Mark as Nightly or make a new category?
[Claude Fable 5.1] The names now follow Method_Scenario_Result throughout both test projects, and ten theme-named fixtures (this one included, now YouTrackSubmitterTests) are renamed after the class they exercise. Fixed in 34f5ea1.
On the category question, I measured the run rather than guess. The 262 Doctor tests take 5.4 s in total (about 6 s wall clock). One test takes 1.9 s, RobustFileCopy_LogHeldOpenForWriting_Throws, which waits on a real file lock; everything else is under a second, and only one other is over half a second. The 27 Bloom-side FreezeDoctor tests take 0.1 s together. John will decide whether that warrants a Nightly or separate category.
…(BL-16719) The announcement at the top of Main reads RunFreezeDoctor before Bloom migrates settings from the previous version, so on the first run after an upgrade it saw the default (off) and a running Doctor had to wait for its next sweep. Announce once more after the migration; a repeat is harmless. Devin raised this on the previous commit. Also drops a development-history measurement from the announcement comment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…outbox cap (BL-16719) "Restart Bloom" asked the Doctor to end the Bloom holding the single-instance token and then started a new one regardless of whether that worked. A Bloom that could not be ended still holds the token, so the new one would exit a few seconds later: the very "Bloom will not start" the user came about. The window now says the old Bloom could not be ended and leaves it there. The outbox's prune kept the newest MaxBundles bundles and skipped any that was uploading, so a queue with an upload in flight could hold one more than the cap. It now takes the surplus off the deletable bundles, as its comment already claimed, and a test pins that. Both were raised by Devin on the previous commit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
[Claude Fable 5.1 during preflight] Consulted Devin up to It raised three things across the run, each now fixed with its own resolved thread:
On |
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson partially reviewed 33 files and made 13 comments.
Reviewable status: 24 of 110 files reviewed, 37 unresolved discussions (waiting on andrew-polk).
Bloom.sln line 33 at r19 (raw file):
Previously, andrew-polk wrote…
Adding "src" as a project can't be right, can it?
Fixed, I believe.
Bloom.sln line 40 at r19 (raw file):
Previously, andrew-polk wrote…
Do we really want/need 3 new production projects for this?
I questioned this pretty hard, too. It seems fairly natural that we're adding a new exe, so we need a new project for it. Then, the doctor shares some code with Bloom itself, so the idea of the Protocol project is to contain that shared code. We could just make the doctor reference Bloom.dll, but then you can't rebuild Bloom while a doctor is running. I'm not sure how much that matters. Claude doesn't think it would increase the doctor's memory footprint, which was one concern I had.
Apparently there's a way we could just make the doctor build include some source files from the EXE project. That would be an alternative way to share code without a separate project. I can do that if you think it's worth it.
Claude also suggested that it's a good discipline that most of the doctor's code doesn't reference WinForms and other things like AutoFac that Bloom uses, and can't start using more pieces of Bloom without a conscious decision to move them into the protocol project. This automatically guards against dependencies creeping into the doctor by accident. This is a secondary argument for the Protocol project, and the main argument for the Core project. In particular, everything the freeze doctor needs WinForms for is in the very small EXE project. The doctor's test code does not reference WinForms.
(Claude originally told me that it needed the Core project because test code couldn't reference an EXE. That, of course, is simply false.)
Maybe we should talk about this. I don't feel that I have a strong opinion, so if you do, maybe we should go with that.
build/Bloom.proj line 517 at r19 (raw file):
Previously, andrew-polk wrote…
no longer has one
Never had one, so this seems worth cleaning up or just removing
Done.
build/check-csharp-robustfile.sh line 42 at r19 (raw file):
Previously, andrew-polk wrote…
Not sure the comment is helpful. I would just drop it.
Done.
src/BloomExe/BloomExe.csproj line 239 at r19 (raw file):
Previously, andrew-polk wrote…
Again, the comment only makes sense in light of development churn. Worth cleaning up.
I had Claude do a sweep for unhelpful historical comments. Hopefully it got most of them.
src/BloomExe/FreezeDoctor/DoctorLauncher.cs line 49 at r19 (raw file):
Previously, andrew-polk wrote…
No reason to keep the historical comment, imo.
Unless you want to restate it more simply as, we decided not to x because y.
Hope it got cleaned up in the historical sweep. I want to get this back to you, so I'm not checking specifically.
src/BloomExe/FreezeDoctor/DoctorLauncher.cs line 102 at r19 (raw file):
Previously, andrew-polk wrote…
// notification area": a window appearing every time you started Bloom would get the// Doctor uninstalled inside a week.Not sure what this means. Is the AI just being cheeky? The user can't even uninstall it, can they (without uninstalling Bloom)?
I'm not sure why it put it that way. The user can, at least, turn it off. I think the point is that if every launch of Bloom adds this doctor window as well, taking up screen space, it will be too annoying to use.
src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs line 622 at r19 (raw file):
Previously, andrew-polk wrote…
I think this summary (above) is vestigial.
Moved to its proper place
src/BloomExe/WebLibraryIntegration/AccessKeys.cs line 46 at r19 (raw file):
Previously, andrew-polk wrote…
I think we better discuss this.
If I understand what is happening, it is using credentials we are about to shut down.
I think we're just moving the code related to submitting YouTrack cards into the shared Protocol library. It shouldn't be any harder to fix there than where it is now, if we change the approach.
src/BloomFreezeDoctor/BloomFreezeDoctor.csproj line 1 at r19 (raw file):
Previously, andrew-polk wrote…
For all the new files in new projects, I pretty much just skimmed. And that may be too strong a word for it...
Me too. I'm worried in both directions about the Freeze Doctor:
- it feels like it's taken way too much time and attention already. (JohnH thought it would take half a day)
- it feels like it's had way too little review for something that runs on end user machines and is allowed to do dangerous things like starting and stopping Bloom and submitting YouTrack issues.
I don't know what to do about it. It would take days of work to thoroughly review all the doctor code.
src/BloomFreezeDoctor.Core/BloomChannel.cs line 76 at r19 (raw file):
Previously, andrew-polk wrote…
whether the Doctor will ship enabled for them
/// is not settledIs that true?
I'm unsure what this means for one-off channels like BloomFAL. Maybe we don't care today.
At this point we're getting set to ship it with Beta, so I guess that's settled. We'll have a few months to decide whether it's worth any problems it causes and want it in the release build.
If we make new releases of on-off channels and want them to have Freeze Doctor, at this point we'll have to add them here.
src/BloomFreezeDoctor.Tests/AlreadyFrozenTests.cs line 4 at r19 (raw file):
Previously, andrew-polk wrote…
Just noting that I didn't really review any of these tests. Let me know if there is something I should look at.
I've been assuming for weeks that reviewing vibe-coded tests is not doable if we want AI to speed up our work.
src/BloomFreezeDoctor.Tests/WhereTheFileStillIsTests.cs line 34 at r19 (raw file):
Previously, JohnThomson+AGNT (Agent AGNT for John Thomson) wrote…
[Claude Fable 5.1] The names now follow Method_Scenario_Result throughout both test projects, and ten theme-named fixtures (this one included, now YouTrackSubmitterTests) are renamed after the class they exercise. Fixed in 34f5ea1.
On the category question, I measured the run rather than guess. The 262 Doctor tests take 5.4 s in total (about 6 s wall clock). One test takes 1.9 s, RobustFileCopy_LogHeldOpenForWriting_Throws, which waits on a real file lock; everything else is under a second, and only one other is over half a second. The 27 Bloom-side FreezeDoctor tests take 0.1 s together. John will decide whether that warrants a Nightly or separate category.
Probably will take a bit longer in CI, but I'm inclined to leave them as regular unit tests for now. We could reconsider moving them when the doctor has been stable for a while.
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk partially reviewed 86 files and all commit messages, made 3 comments, and resolved 37 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on JohnThomson).
Bloom.sln line 40 at r19 (raw file):
Previously, JohnThomson (John Thomson) wrote…
I questioned this pretty hard, too. It seems fairly natural that we're adding a new exe, so we need a new project for it. Then, the doctor shares some code with Bloom itself, so the idea of the Protocol project is to contain that shared code. We could just make the doctor reference Bloom.dll, but then you can't rebuild Bloom while a doctor is running. I'm not sure how much that matters. Claude doesn't think it would increase the doctor's memory footprint, which was one concern I had.
Apparently there's a way we could just make the doctor build include some source files from the EXE project. That would be an alternative way to share code without a separate project. I can do that if you think it's worth it.
Claude also suggested that it's a good discipline that most of the doctor's code doesn't reference WinForms and other things like AutoFac that Bloom uses, and can't start using more pieces of Bloom without a conscious decision to move them into the protocol project. This automatically guards against dependencies creeping into the doctor by accident. This is a secondary argument for the Protocol project, and the main argument for the Core project. In particular, everything the freeze doctor needs WinForms for is in the very small EXE project. The doctor's test code does not reference WinForms.
(Claude originally told me that it needed the Core project because test code couldn't reference an EXE. That, of course, is simply false.)
Maybe we should talk about this. I don't feel that I have a strong opinion, so if you do, maybe we should go with that.
No, I'm ok with leaving as is. Thanks for thinking through it.
src/BloomExe/WebLibraryIntegration/AccessKeys.cs line 46 at r19 (raw file):
Previously, JohnThomson (John Thomson) wrote…
I think we're just moving the code related to submitting YouTrack cards into the shared Protocol library. It shouldn't be any harder to fix there than where it is now, if we change the approach.
ok; I was thinking it was trying to share the credentials for uploading books. But if the YouTrack submission is already using the same credentials, we already have to modify those rather than remove them. So this should be fine, then.
src/BloomFreezeDoctor.Tests/WhereTheFileStillIsTests.cs line 34 at r19 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Probably will take a bit longer in CI, but I'm inclined to leave them as regular unit tests for now. We could reconsider moving them when the doctor has been stable for a while.
Ok, let's leave them for now.
2 participants
Problem
Users tell us Bloom froze, and we get almost nothing to work with. Their problem report is written after they killed Bloom, so it describes a healthy new process, and the log holds only what Bloom managed to write before it stopped responding. Three quite different failures arrive looking identical — the UI stops responding; Bloom exits without reporting anything; or Bloom's window is gone while the process lives on, so the user cannot start Bloom again. BL-16697 is the live example.
Cause
Nobody is watching at the moment it happens, and the worst case cannot be watched from outside at all.
The measurement the whole design rests on — check this first if you check one thing: a WinForms UI thread blocked in a managed wait on an STA thread still dispatches sent messages, so the window answers probes,
IsHungAppWindowreports it healthy andProcess.Respondingreturnstruewhile Bloom is completely stuck. Measured on a real Bloom: nine minutes frozen, reported responsive throughout. Since Bloom's UI thread awaits WebView2 constantly, that is likely the common shape of freeze rather than an exotic one.WM_TIMERis not dispatched, which is why detection rests on a UI-thread timer heartbeat published through shared memory, and not on either API that looks built for the job.What this PR does
Adds a companion Windows app — the Freeze Doctor, four projects in this repository — and teaches Bloom to publish enough about itself that the Doctor can tell it has stopped and say something useful about why.
Bloom's side:
Log.txteach run and falls back to a random name only when another Bloom holds it, so guessing from outside picks the wrong file in exactly the restart-after-a-freeze case); in-flight API requests and which lock each is waiting on; and long-operation scopes at the half-dozen places Bloom legitimately stops answering for minutes, which raise the freeze threshold rather than letting a publish look like a freeze.Main, so a Doctor already running adopts it at once rather than at its next sweep. Everything before that announcement is time in which a hang or a crash cannot be doctored at all, because Bloom only asks for a crash dump when a Doctor is already watching. For the users who have never switched the Doctor on it costs one setting read.The Doctor's side: it is already running when the trouble starts, gathers what can only be gathered at that instant, and files a YouTrack card by itself.
It ships inside Bloom's installer with no installer of its own, and is off by default, switched on from the debug menu on the Collections tab. It is part of the next Beta; whether to offer it to Release users is still open.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16719
Devin review
This change is