From aab1ecf1587e07d52a5020d2869d058352ab93a2 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 7 Aug 2026 13:05:24 -0700 Subject: [PATCH 1/2] Don't listen for ApplicationExit when there is no GUI application (BL-16668) `Bloom.exe createArtifacts` died with ObjectDisposedException on the epub step, so the harvester failed every book with epubs enabled and uploaded no artifacts at all -- not even the .bloompub it had already built successfully (BH-7836). A command-line verb never calls Application.Run, so the only WinForms message loop in the process belongs to a worker: the dedicated thread of the off-screen browser PublishHelper uses for page checks, created and disposed once per batch. Building the .bloompub disposes a PublishHelper, which releases that browser, which ends the loop -- and WinForms, seeing its only message loop end, raises Application.ApplicationExit. ApplicationContainer believed it and disposed itself, and it is the parent scope of the ProjectContext still in use, so the epub step (which runs next) got ObjectDisposedException from ProjectContext.BookServer. The harvester treats any non-FontProblems exit code as failure, hence losing the whole book. In the GUI this never fired, because the main thread's loop is still running; it only ever bit a non-GUI entry point. d41fa15f9e had already met this mechanism and made ProjectContext.Dispose tolerate the container being disposed first, but the premature disposal itself remained. It was harmless there only because it happened after all the requested work. Rather than act on a signal we know to be wrong, don't subscribe to it. Program.RunningInConsoleMode is set by Program.Main for every command-line verb (hydrate, upload, download, getfonts, changeLayout, createArtifacts, spreadsheetExport/Import, sendFontAnalytics) before it dispatches to any of them. Two notes on the choice of flag and location: - RunningInConsoleMode, not RunningHarvesterMode. It covers every verb -- the others never set the harvester flag and would have stayed exposed -- and it has no ordering hazard, because it is true before any container exists, whereas CreateArtifactsCommand sets RunningHarvesterMode AFTER constructing one, so a guard on that flag works only because the event happens to fire later. - Gating the subscription rather than the handler, because the question is settled before we would subscribe, so there is no reason to sign up and then ignore the answer. Verified that the console-mode branch of Main is self-contained -- it parses, dispatches, pumps with Application.DoEvents() and returns, never reaching RunBloom's Application.Run -- so RunningInConsoleMode being true really does mean there is no GUI application to listen for, and no GUI path is affected. One knock-on, documented next to the guard: OnApplicationExit was the only caller of Program.FinishLocalizationHarvesting(), so a CLI verb no longer runs it. That code is #if DEBUG and inert unless LocalizationManager.IgnoreExistingEnglishTranslationFiles is set, so release runs are unaffected. The regression test is the other half of this. Every pre-existing case in CreateArtifactsCommandTests requested a single artifact, which is exactly why CI stayed green through this bug: the harvester always passes --bloomdOutputPath/--bloomDigitalOutputPath AND --epubOutputPath in one run, and only that combination exposes it, because with one artifact nothing runs after the premature disposal. The test asks for both and fails on unmodified master with the production stack trace frame for frame. It needs a SetUp that sets RunningInConsoleMode, because the tests call HandleInternal directly and so bypass Main, which sets it for real runs -- mirroring what the fixture already did for RunningHarvesterMode in TearDown. Re-verified that the test still fails with ApplicationContainer.cs at exact master state and that SetUp in place, so it is the fix passing the test rather than the harness. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/ApplicationContainer.cs | 24 +++- .../CLI/CreateArtifactsCommandTests.cs | 105 ++++++++++++++++++ 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/BloomExe/ApplicationContainer.cs b/src/BloomExe/ApplicationContainer.cs index a20bb7cfae8a..b07afac403ed 100644 --- a/src/BloomExe/ApplicationContainer.cs +++ b/src/BloomExe/ApplicationContainer.cs @@ -81,7 +81,25 @@ public ApplicationContainer() _container = builder.Build(); - Application.ApplicationExit += OnApplicationExit; + // Only listen for the application exiting when there IS an application in the GUI sense. + // A command-line verb never calls Application.Run, so the only WinForms message loop in the + // process belongs to some worker -- currently the dedicated thread of the off-screen browser + // PublishHelper uses for page checks, created and disposed once per batch. When that loop + // ends, WinForms decides the application is exiting and raises ApplicationExit, mid-run. Acting + // on that disposed this container, the parent scope of the still-in-use ProjectContext, so the + // next artifact step died with ObjectDisposedException (BL-16668). Not subscribing is safe + // because in that flow the container's lifetime is already bounded by the `using` blocks in the + // CLI command handlers, and the process exits as soon as those unwind. + // + // One knock-on worth knowing: OnApplicationExit is the only caller of + // Program.FinishLocalizationHarvesting(), so a command-line verb no longer runs it. That is + // #if DEBUG code which does nothing unless LocalizationManager.IgnoreExistingEnglishTranslationFiles + // is set, so release CLI runs are unaffected -- but a DEBUG localization-harvesting run driven + // through a CLI verb would no longer merge the English translation files. If we ever want that, + // call it from the CLI path explicitly rather than by leaning on a shutdown event that is not + // really telling us the application is shutting down. + if (!Program.RunningInConsoleMode) + Application.ApplicationExit += OnApplicationExit; // Register the API Handlers that are global to the application (not dependent on knowing a particular project). // Note: it is is a work in progress to transfer more API handlers from ProjectContext to here. @@ -98,6 +116,10 @@ public ApplicationContainer() server.ApiHandler.RecordApplicationLevelHandlers(); } + /// + /// The application is really shutting down, so tear the container down. Only ever subscribed + /// when Bloom is running as a GUI application -- see the constructor for why. + /// private void OnApplicationExit(object sender, EventArgs e) { Application.ApplicationExit -= OnApplicationExit; diff --git a/src/BloomTests/CLI/CreateArtifactsCommandTests.cs b/src/BloomTests/CLI/CreateArtifactsCommandTests.cs index 6ceb4c15d0a8..15964c1d8e93 100644 --- a/src/BloomTests/CLI/CreateArtifactsCommandTests.cs +++ b/src/BloomTests/CLI/CreateArtifactsCommandTests.cs @@ -11,11 +11,23 @@ namespace BloomTests.CLI [TestFixture] public class CreateArtifactsCommandTests { + [SetUp] + public void SetUp() + { + // Program.Main sets this for a real command-line run, before it dispatches to the verb. These + // tests call HandleInternal directly and so bypass Main, which means they have to stand in for + // it — exactly as TearDown below already does for RunningHarvesterMode. ApplicationContainer + // reads this flag to know there is no GUI application whose exit it should listen for; without + // it set, the container tears itself down mid-run and the epub step fails (BL-16668). + Program.RunningInConsoleMode = true; + } + [TearDown] public void TearDown() { // Without this, subsequent tests will fail because they think the harvester is still running. Program.RunningHarvesterMode = false; + Program.RunningInConsoleMode = false; } [Test] @@ -238,6 +250,99 @@ public void CreateArtifacts_LegacyBookWithInvalidXmatter_ReportsLegacyBookCannot } } + // The harvester never asks for just one artifact: it passes --bloomdOutputPath, + // --bloomDigitalOutputPath AND --epubOutputPath in a single createArtifacts run. That + // combination is what BL-16668 broke, and why every other test here missed it. Making the + // bloomdigital spins up (and then tears down) PublishHelper's off-screen browser, whose + // dedicated thread runs the only WinForms message loop a CLI process has. Ending that loop + // made WinForms raise Application.ApplicationExit, which disposed the ApplicationContainer -- + // the parent scope of our still-in-use ProjectContext -- so the epub step, which runs + // afterwards, died with ObjectDisposedException resolving ProjectContext.BookServer and + // createArtifacts returned EpubException. A test that requests a single artifact cannot + // catch that, because nothing runs after the premature disposal. + [Test] + public void CreateArtifacts_BloomDigitalAndEpubRequestedTogether_BothCreated() + { + using ( + var testFolder = new TemporaryFolder( + "CreateArtifacts_BloomDigitalAndEpubRequestedTogether_BothCreated" + ) + ) + { + var collectionFolderPath = testFolder.Combine("collection"); + + var bookFolderPath = Path.Combine(collectionFolderPath, "book"); + System.IO.Directory.CreateDirectory(bookFolderPath); + var collectionFilePath = Path.Combine( + collectionFolderPath, + "collection.bloomCollection" + ); + var settings = new CollectionSettings(collectionFilePath); + settings.Save(); + var metaData = new BookMetaData(); + metaData.WriteToFolder(bookFolderPath); + var bookPath = System.IO.Path.Combine(bookFolderPath, "book.htm"); + System.IO.File.WriteAllText( + bookPath, + @" + +
+
+
+
+ Hello +
+
+
+
+ + " + ); + + var bloomDigitalOutputPath = Path.Combine(testFolder.FolderPath, "bloomdigital"); + var epubOutputPath = Path.Combine(testFolder.FolderPath, "epub", "book.epub"); + // Sanity check: neither artifact exists yet, so finding them later really does mean + // this run made them. + Assert.That( + Directory.Exists(bloomDigitalOutputPath), + Is.False, + "test setup: bloomdigital output should not exist before the run" + ); + Assert.That( + File.Exists(epubOutputPath), + Is.False, + "test setup: epub output should not exist before the run" + ); + + var result = CreateArtifactsCommand.HandleInternal( + new CreateArtifactsParameters() + { + BookPath = bookFolderPath, + CollectionPath = collectionFilePath, + BloomDigitalOutputPath = bloomDigitalOutputPath, + EpubOutputPath = epubOutputPath, + NoAnalytics = true, + } + ); + + Assert.That( + result, + Is.EqualTo(CreateArtifactsExitCode.Success), + "createArtifacts should succeed when both a bloomdigital and an epub are requested" + ); + Assert.That( + File.Exists(Path.Combine(bloomDigitalOutputPath, "index.htm")), + Is.True, + "the bloomdigital artifact should have been created" + ); + Assert.That( + File.Exists(epubOutputPath), + Is.True, + "the epub artifact should have been created" + ); + } + } + [Test] public void CreateArtifacts_WithJsonOutput_CreatesJsonFile() { From 4e57a679e8d8e92a0edc0234a4a220184468bc10 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Fri, 7 Aug 2026 13:10:36 -0700 Subject: [PATCH 2/2] Accumulate the epub failure flag instead of overwriting the others (BL-16668) CreateArtifactsExitCode is [Flags], and its own comment says the values exist "so they can be bit-or'd together". Every step in CreateArtifacts accumulates with |= -- except the epub worker, which assigned. So an epub failure discarded whatever had already been recorded: most importantly BookHtmlNotFound from the bloomdigital step immediately above, but also UnhandledException or LegacyBookCannotHarvest. The process exited with just 4, GetErrorsFromExitCode reported only "EpubException", and the harvester's record of the book lost the rest. It is most likely to bite in exactly the configuration this card is about, since the harvester always asks for both artifacts. Checked whether the assignment might have been deliberate; it was not: - The commit that introduced it, dd8a08724c, is titled "Fix incorrect exit codes if ThreadPool throws exception" -- its stated purpose was to make the exit code correct. - At that same commit, `exitCode |= CreateBloomDigitalArtifacts(...)` was already 19 lines above, and could already have set BookHtmlNotFound. So it was never lossless; it was inconsistent with its own method from the day it was written. - "Epub failure should dominate" is not the design either: GetErrorsFromExitCode reports every flag as a list. The one real argument for a plain write is atomicity, since |= is a read-modify-write on a variable captured by a worker thread. It does not apply: the main thread is blocked spinning on countdownEvent.IsSet for the whole window and does not touch exitCode until after the worker signals. (And if there were a concurrent writer, the answer would be a lock, not a lossy write.) Not covered by a test: provoking it needs a book that fails the bloomdigital step AND throws in epub creation in one run, which no existing fixture arranges. Found by Devin during preflight review of this PR; Andrew's call to fix it here rather than on its own card. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/CLI/CreateArtifactsCommand.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/BloomExe/CLI/CreateArtifactsCommand.cs b/src/BloomExe/CLI/CreateArtifactsCommand.cs index 6791cab6f360..2b0ffab4884e 100644 --- a/src/BloomExe/CLI/CreateArtifactsCommand.cs +++ b/src/BloomExe/CLI/CreateArtifactsCommand.cs @@ -195,7 +195,15 @@ private static CreateArtifactsExitCode CreateArtifacts(CreateArtifactsParameters catch (Exception ex) { Console.WriteLine(ex.ToString()); - exitCode = CreateArtifactsExitCode.EpubException; + // |=, not =: these are [Flags] values and every other step here accumulates. + // A plain assignment discarded whatever had already been recorded -- most + // notably BookHtmlNotFound from the bloomdigital step just above -- so the + // harvester was told only "EpubException" and lost the fact that the book's + // HTML was also wrong. That is the combination the harvester actually hits, + // since it always asks for both artifacts. Safe to accumulate from this + // worker: the main thread is blocked on countdownEvent below and does not + // touch exitCode until after we signal. + exitCode |= CreateArtifactsExitCode.EpubException; } countdownEvent.Signal(); // Decrement by one });