Give console mode a real message loop, so command-line Bloom can use a browser (BL-16773) - #8251
Give console mode a real message loop, so command-line Bloom can use a browser (BL-16773)#8251JohnThomson wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| src/BloomExe/Program.cs | Replaces the outermost DoEvents polling loop with an STA WinForms message loop that starts, monitors, and returns the console command task. |
| src/BloomExe/Publish/OffScreenBrowser.cs | Checks the inner browser’s recorded initialization fault before its timeout, completing the prior review fix. |
| src/BloomExe/WebView2Browser.cs | Records and reports asynchronous initialization faults and makes readiness and navigation paths surface the underlying cause. |
| src/BloomExe/Spreadsheet/SpreadsheetImporter.cs | Awaits the Task<Browser> returned by invoking the asynchronous browser factory on the control thread. |
| src/BloomTests/ConsoleCommandLoopTests.cs | Verifies STA continuation affinity, WebView2 readiness, exit-code propagation, and synchronous and asynchronous command failures. |
| src/BloomTests/WebView2BrowserInitFailureTests.cs | Verifies that initialization failure is recorded, externally observable, and surfaced before navigation timeout. |
Reviews (3): Last reviewed commit: "Give console mode a real message loop, s..." | Re-trigger Greptile
|
[Claude Opus 5 (1M context) from John Thomson's machine during preflight] Thanks — the P1 was right, and it was the most important place to be right about. The earlier commits recorded a WebView2 initialization failure and made the waits inside Fixed in Also added the doc comments the three remaining test methods were missing, per this project's convention that public methods carry one. |
|
[Claude Opus 5 (1M context) from John Thomson's machine during preflight] Consulted Devin on 2026-08-27 up to commit bab4d7f. Its review of 3c02261 found no bugs, one Investigate flag (blocking console commands can now deadlock) and five informational items. The Investigate flag is posted as its own thread, assessed, and acted on: the existing blocking console paths were checked individually and are safe, and RunConsoleCommandLoop now carries a warning about the hazard for future code. One informational item was also acted on -- the loop now fails loudly rather than hanging if something other than the command ends it. Greptile's review covers 3c02261 and its one P1 is fixed; it has not re-run against that fix. CI (pr-automation) passed and the full C# suite is green. |
…a browser (BL-16773) Command-line Bloom could not reliably use a browser. Bulk upload was the visible casualty (BL-16767, fixed separately in #8246): every book after the first failed with "The instance of CoreWebView2 is uninitialized." The same trap sat under console `spreadsheetImport` of a spreadsheet carrying audio, and under anything else a console verb might want a browser for. Worse, when it sprang it said nothing useful -- the real error was thrown away, and what surfaced twenty seconds later was a navigation timeout somewhere else. The cause was the console wait loop. Console mode waited for its command by spinning `Application.DoEvents()`, and as the OUTERMOST message loop that uninstalls the WindowsFormsSynchronizationContext and leaves a plain one, whose Post queues to the thread pool. So from the first await that actually yielded, console work moved onto MTA thread-pool threads -- where a WebView2 cannot even be created, because CoreWebView2Environment.CreateAsync needs an STA thread and throws RPC_E_CHANGED_MODE. Program.Main's own comment said a synchronous Main was kept precisely to stop this happening; the wait loop immediately below it undid it. And because the browser constructors started initialization as "_ = InitWebView()", nothing ever observed the resulting exception. Nested inside Application.Run, DoEvents does not discard the context, which is why only console mode was ever affected. What this changes: - Console commands now run inside a real message loop, Program.RunConsoleCommandLoop, and are started from INSIDE it, because an await captures whichever context is current at the moment it suspends -- starting the command any earlier would already be too late for its first await. Their awaits therefore resume on the pumping STA main thread. This is what fixes the class of bug, console spreadsheet import included. Parsing and dispatch move to ParseAndDispatchConsoleCommand, unchanged apart from becoming a method. - A failed WebView2 initialization is recorded and reported rather than discarded: log, Sentry, and stderr in console mode, naming the creating thread and its apartment state. Every ready-wait now gives up at once with the real cause instead of spinning out its timeout and then blaming the timeout -- including OffScreenBrowser's own, which runs its readiness loop on the thread it owns and so cannot rely on the checks inside WebView2Browser. - SpreadsheetImporter.GetBrowserAsync no longer casts the Task<Browser> that Control.Invoke hands back to (Browser). Control.Invoke returns what the delegate returned, and the delegate is async, so the cast was an InvalidCastException waiting to happen. It now awaits the Task, as GetMd5Async in the same file already did. - Removed about forty lines of dead shared-WebView2-environment machinery (BeginSharedEnvironmentBatch / EndSharedEnvironmentBatch and their statics); its only consumer moved to OffScreenBrowser. Two comments that had explained themselves in terms of those statics are corrected, including one in ExternalApi that claimed process-book needs the UI thread "because it creates and pumps an off-screen WebView2" -- no longer the reason. The one new hazard, and what was done about it: because awaits now come back to the main thread, sync-over-async on that thread can deadlock, where under the old wait it could not. Every blocking wait reachable from a console verb was checked and is safe -- each waits on a library task that uses ConfigureAwait(false) internally (the AWS SDK, HttpClient), on work owned by another thread (OffScreenBrowser completes its own), or through AsyncUtil.RunSync, which pins to TaskScheduler.Default for exactly this reason. The one path that could have waited on the main thread cannot: ApiRequest marshals only to a form from Application.OpenForms, which is empty in console mode. RunConsoleCommandLoop's doc comment records the hazard, which calls are safe and why, and that blocking on one of Bloom's own async methods is the case to avoid. Program.MainContext is deliberately not published by the new loop, with a comment saying why: code keyed off it (RabProjectService, CommonApi, ToastService) behaves differently when it is set, and RabProjectService has a null branch precisely for the no-UI case. Tests: ConsoleCommandLoopTests covers the property that matters -- awaits resume on the calling STA thread -- and, most to the point, that a WebView2 created by a console command AFTER an await now becomes ready, which is the exact thing bulk upload could not do. It also records, as a characterization test, the WinForms behaviour the whole change is built around, so we find out if DoEvents ever stops discarding the context. WebView2BrowserInitFailureTests covers the reporting and the fast failure, including that the recorded error is readable from outside the class, which is what OffScreenBrowser depends on. Verified by hand as well as by tests: a three-book bulk upload, and a full createArtifacts run on a real book producing a valid .bloompub, an ePUB with all five page files, the complete bloomdigital folder and all three thumbnail sizes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bab4d7f to
5714016
Compare
|
Closing: this PR targets the wrong branch. BL-16773 is a Retargeting it in place was not viable: this branch was cut from Superseded by #8261, which is the same change cherry-picked onto Closing rather than force-pushing a rebase, to preserve the review history on this PR. |
|
Reopened as a draft. Supersedes my earlier "closing: wrong branch" note above — that call has been reversed. Whether BL-16773 ships in 6.5 or waits for 6.6 is still undecided, so both candidates are being kept open as drafts and exactly one will be merged:
They are the same change; #8261 is a verified byte-for-byte cherry-pick of this branch onto This PR is the 6.6 route. Merge it only if the decision is to wait for 6.6; in that case close #8261. Do not merge both. Note for whoever picks: this branch was cut from |
1 participant
Problem. Command-line Bloom could not reliably use a browser. Bulk upload was the visible casualty (BL-16767, fixed separately in #8246): every book after the first failed with "The instance of CoreWebView2 is uninitialized." The same trap sat under console
spreadsheetImportof a spreadsheet carrying audio, and under anything else a console verb might want a browser for. Worse, when it sprang it said nothing useful — the real error was thrown away, and what surfaced twenty seconds later was a navigation timeout somewhere else.Cause. Console mode waited for its command by spinning
Application.DoEvents(). As the outermost message loop, that uninstalls theWindowsFormsSynchronizationContextand leaves a plain one, whosePostqueues to the thread pool — so from the firstawaitthat actually yielded, console work moved onto MTA thread-pool threads. A WebView2 cannot even be created there:CoreWebView2Environment.CreateAsyncneeds an STA thread and throwsRPC_E_CHANGED_MODE.Program.Main's own comment said a synchronousMainwas kept precisely to stop this happening; the wait loop immediately below it undid it. And because the browser constructors started initialization as_ = InitWebView(), nothing ever observed the resulting exception.Fix.
Program.RunConsoleCommandLoop), and are started from inside it, so their awaits resume on the pumping STA main thread. This is what actually fixes the class of bug, console spreadsheet import included.OffScreenBrowser's own (which runs its readiness loop on the thread it owns), now gives up at once with the real cause instead of spinning out a timeout and then blaming the timeout.SpreadsheetImporter.GetBrowserAsyncno longer casts theTask<Browser>thatControl.Invokehands back to(Browser).The one new hazard, and what was done about it. Because awaits now come back to the main thread, sync-over-async on that thread can deadlock, where under the old wait it could not. Every blocking wait reachable from a console verb was checked and is safe — each waits on a library task that uses
ConfigureAwait(false)internally, on work owned by another thread, or throughAsyncUtil.RunSync, which pins toTaskScheduler.Defaultfor exactly this reason.RunConsoleCommandLoop's doc comment records the hazard, which calls are safe and why, and that blocking on one of Bloom's own async methods is the case to avoid.Not a behavior change for the desktop app: nested inside
Application.Run,DoEventsnever discarded the context, which is why only console mode was affected.Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16773
Devin review
This change is