BL-16690 Honor a collection's minimum Bloom version, upgrading in place - #8206
BL-16690 Honor a collection's minimum Bloom version, upgrading in place#8206andrew-polk wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
[Devin] Investigate: Blocking the UI thread on the download also blocks the embedded server and WebView content
UpgradeBloom deliberately blocks the UI thread (Task.Run(...).GetAwaiter().GetResult()), which is safe from deadlock because the awaited continuations run on the thread pool. Two side effects worth confirming with a live feed:
Application.UseWaitCursor = truenormally needs a message pump turn to take effect, so the wait cursor may never actually appear during the blocked period.- In the mid-session lock-out case, any WebView2/BloomServer work that needs the UI thread stalls for the whole download, which can look like a hang for a large full-release download.
A modal progress dialog that pumps messages would fix both.
There was a problem hiding this comment.
[Claude Opus 5] Confirmed, not fixed — this is on the report going to the developer as a decision, along with your point about the wait cursor.
We already knew the download blocks the UI thread and noted in the code that a progress window is wanted; your first point sharpens it usefully, because if UseWaitCursor never gets its pump turn then the one mitigation we do have may not exist at all. Both are solved by the same thing: a modal progress dialog that pumps messages. That is new UI rather than a review fix, so it is the developer's call whether it lands here or as a follow-up.
There was a problem hiding this comment.
[Claude Opus 5] Investigated at Andrew's request, no code changed yet. Summary: the wait cursor almost certainly does nothing, so "good enough" is not available, and Bloom already has the right component to fix it properly.
Does the wait cursor appear? Two reasons to think not, and they are independent.
The first is the one you raised: Application.UseWaitCursor = true marks the open forms, but the cursor only actually changes when the window processes WM_SETCURSOR. We set the flag and then immediately block the UI thread, so nothing is processed until the download is over, by which time we have set it back to false.
The second is worse and applies to the startup case regardless: Application.UseWaitCursor acts on Application.OpenForms, and at the startup gate there are no open forms to act on. There is no Shell yet, and StartupScreenManager.HideSplashScreenForDialog only hides the splash. So even with a pumping message loop there would be nothing to show a wait cursor on.
I could not prove this by observation: a developer build refuses to update itself, so the live run returned from TryDownloadUpdateWithoutToasts immediately and there was no window during which to look. That part is analysis, and it is the part worth checking against a real download.
What we already have. BrowserProgressDialog.DoWorkWithProgressDialog (src/BloomExe/MiscUI/BrowserProgressDialog.cs:43) is built for exactly this shape: it runs the work on a BackgroundWorker while the dialog owns the UI thread, so the window keeps painting. TeamCollection.cs:2919 is a working example — a ReactDialog on the progressDialogBundle, a doWhat returning bool, and a fixed size. It needs SocketServer, which the startup path already has, since we call BloomServer.EnsureListening() before showing the too-old dialog.
Recommendation. Replace the Task.Run(...).GetAwaiter().GetResult() in MinimumBloomVersionCheck.UpgradeBloom with BrowserProgressDialog.DoWorkWithProgressDialog, moving TryDownloadUpdateWithoutToasts into the doWhat callback and dropping the wait cursor entirely. That fixes the frozen window and the cursor together, and it uses a component the codebase already trusts rather than inventing one.
Two things to decide when it is written: Velopack's DownloadUpdatesAsync takes a progress callback, so we can report real percentages rather than a spinner; and the dialog would want showReportButton = "never" and no cancel, since a half-cancelled update is not a state we want to reason about.
Effort: moderate but contained — one method reworked, no new UI component, no change to what the user is told.
There was a problem hiding this comment.
[Claude Opus 5] Devin has now promoted this from a flag to a bug on the latest commit, which matches what we found: the wait cursor is not a mitigation, so this is a plain freeze with nothing on screen.
Andrew's decision was to investigate before coding, and that investigation is in the reply above. The short version: Application.UseWaitCursor cannot help here, because beyond the message-pump problem it acts on Application.OpenForms and at the startup gate there are none — no Shell yet, and the splash is only hidden. So there is nothing to show a wait cursor on even in principle.
The recommendation stands: BrowserProgressDialog.DoWorkWithProgressDialog, which already exists for exactly this shape and is used by the Team Collection sync. Leaving this thread open until Andrew decides whether it lands in this PR or a follow-up.
There was a problem hiding this comment.
[Claude Opus 5] Now settled by observation, not analysis. Andrew ran the real upgrade himself and reports: no wait cursor, and no indication of progress of any kind.
Worth being precise about how that was obtained, because a developer build normally refuses to update. IsDev is decided by the path of Bloom.dll — /output/Debug/ means developer, /Bloom<channel>/current/ means an installed channel — so this branch's binaries were laid over a copy of an installed Beta. It then reported channel Beta, found 6.4.105 → 6.4.108 on the real feed, and genuinely downloaded a 90 MB package. That is the first time this code path has run end to end.
So the question you raised is answered in the worst direction: during that download there is nothing on screen at all. At the startup gate no main window exists — the collection never opened — and the only form in Application.OpenForms is the hidden splash, so Application.UseWaitCursor has nothing visible to mark even before the blocked message pump is considered. The user simply sees the dialog vanish and the machine sit there.
(One correction to my earlier reply: I said there were no open forms at that point. Too strong — a hidden form is still in OpenForms. The conclusion holds for the more precise reason above.)
The recommendation is unchanged but no longer speculative: BrowserProgressDialog.DoWorkWithProgressDialog, which runs the work on a background worker while the dialog owns the UI thread, as TeamCollection.cs:2919 already does. Velopack's DownloadUpdatesAsync takes a progress callback, so a real percentage is available rather than a spinner. Leaving this thread open until Andrew decides whether it lands in this PR or a follow-up.
There was a problem hiding this comment.
[Devin] Investigate: Follow-up message boxes may be owned by the hidden splash form at startup
ReportCollectionNeedsNewerBloom correctly passes a null owner for the main dialog, but the follow-up messages go through BloomMessageBox.ShowInfo/ShowWarning, which use Shell.GetShellOrOtherOpenForm(). At startup there is no Shell, and StartupScreenManager.HideSplashScreenForDialog only Hide()s the splash form rather than closing it, so Application.OpenForms.LastOrDefault() can return the still-open-but-hidden splash as owner.
Worth verifying these post-upgrade messages actually appear (and are positioned sensibly) in the pre-collection case.
There was a problem hiding this comment.
[Claude Opus 5] Noted, and on the report as something that needs live testing rather than a code fix.
You're right that HideSplashScreenForDialog only hides the splash, so it stays in Application.OpenForms. Nothing in this branch has been exercised against a live update feed yet, so which owner these follow-up boxes actually land on — and whether they appear sensibly — is one of the things that first real run has to check.
There was a problem hiding this comment.
[Devin] Investigate: Returning true from OpenProjectWindow with a null project context leaves other startup actions running during shutdown
When the user chooses to upgrade, OpenProjectWindow returns true after ProgramExit.Exit() has been requested. Application.Exit() does not necessarily end things promptly (Shell.OnClosing cancels the first close while it saves), and the remaining queued startup actions still run on Idle — notably CheckRegistration, which can put up its own modal dialog while Bloom is trying to quit for the install.
Worth a look at whether the queue should be drained or disabled once we have decided to quit for an upgrade.
There was a problem hiding this comment.
[Claude Opus 5] Confirmed as plausible, not fixed — it is on the report for Andrew rather than something I should change unasked, and this thread stays open.
The concern is real: CheckRegistration raising its own modal dialog while Bloom is trying to quit for an install would be a poor experience and could conceivably interfere with the install. But the fix means changing how the startup action queue behaves once we've decided to quit, which is a startup-sequencing change well beyond a review fix, and we have also hit this run's cap on fix-and-re-review cycles. Flagging it rather than reaching for it.
There was a problem hiding this comment.
[Claude Opus 5] Investigated at Andrew's request, no code changed yet.
How the queue works. StartupScreenManager.StartManaging subscribes DoStartupAction to Application.Idle (src/BloomExe/MiscUI/StartupScreenManager.cs:59). It runs one queued action per idle event and unsubscribes itself only when the queue is empty (:201). Nothing in it knows or cares that Bloom has been asked to quit, so after the user chooses to upgrade and we call ProgramExit.Exit(), the remaining actions keep being handed out on every idle turn while shutdown proceeds. Application.Exit() is not instant — Shell.OnClosing cancels the first close while it saves — so there is a real window, and CheckRegistration is the action most likely to use it to put up a dialog.
Three options.
-
A quit flag consulted by the queue.
ProgramExitalready knows: it sets_forceShutdownThreadthe momentExit()is called, so it needs onlypublic static bool IsExiting => _forceShutdownThread != null;and one early return at the top ofDoStartupAction. About five lines, in the place that already owns the fact, and it covers every future path that quits during startup rather than just ours. Downside: it changes shared startup behaviour for all callers, so it wants a moment's thought about whether any queued action should still run while quitting (I did not find one that should). -
Stop the queue explicitly when we decide to upgrade. A
StartupScreenManager.StopRunningStartupActions()that unsubscribes and clears the list, called fromMinimumBloomVersionCheckjust beforeProgramExit.Exit(). Around ten lines, and the blast radius is exactly our feature. Downside: it is a second mechanism for "we are quitting", and the next path that quits mid-startup has to remember to call it. -
Leave it. Nobody has seen this happen; it is reasoning from the code, and the window is a second or two.
Recommendation: option 1. It is smaller than option 2, it puts the knowledge where it already lives, and "do not start new startup work once we have decided to quit" is a rule that is true independently of this feature. I would pair it with a quick look at the queued actions to confirm none of them needs to run during shutdown.
Not doing it in this PR unless you say so: it changes shared startup sequencing, which is a different kind of risk from the rest of this branch, and it cannot be verified here — the upgrade path that triggers it has never run on a build that can actually update itself.
There was a problem hiding this comment.
[Devin] Investigate: Lock-out flow depends on the topmost open form actually being the Shell
LockUserOutOfOpenCollection passes Shell.GetShellOrOtherOpenForm() as Shell to Program.ChooseACollection. GetShellOrOtherOpenForm falls back to the last open form when no Shell is found, in which case the as yields null and ChooseACollection(null) takes the startup branch: on cancel it exits Bloom, and on selection it calls OpenCollection while _projectContext is still alive, which trips Debug.Assert(_projectContext == null) in OpenProjectWindow and would leave a second Shell/ProjectContext.
In practice the Shell is open whenever Team Collection monitoring runs, so this is unlikely, but passing the real Shell explicitly (or bailing if it isn't found) would make the invariant safe.
There was a problem hiding this comment.
[Claude Opus 5] Confirmed as a real edge, not fixed — leaving this thread open for Andrew.
The reasoning holds, and it got slightly more interesting with the change we just made: cancelling the chooser now quits Bloom, which is what the null branch does anyway, so half of your scenario is no longer a divergence. The remaining half stands — if the topmost form is not the Shell and the user does pick a collection, we would open one while a project context is still alive.
Not acting because this run has reached its cap on fix-and-re-review cycles, and because the honest fix is to decide what should happen when there is no Shell (bail? assert?) rather than to paper over the cast. Recorded on the preflight report.
|
[Claude Opus 5] Consulted Devin through this branch, up to commit Six reviews ran, one per commit — Devin restarts on every push, so this is the record of the whole conversation rather than a single pass. It earned its keep: three of its findings were real bugs that neither I nor the local review had caught, and two more were bugs in fixes I had made earlier in the same run. Fixed and closed on their own threads:
Left open for Andrew, and on the preflight report:
Around ten informational items were assessed and not mirrored, the substantive ones being that the command-line and bulk-upload paths remain ungated (a deliberate decision, recorded in the code), that CI is green. CodeRabbit is switched off on this repo ( |
288a261 to
e580da0
Compare
7901985 to
f14d7ab
Compare
f14d7ab to
c7a0448
Compare
c7a0448 to
69225b1
Compare
There was a problem hiding this comment.
[Devin] Investigate: Repository copy overrides a local requirement entirely, not "whichever demands more"
Note the asymmetry: when the Team Collection repository can be read, its answer is used verbatim, including an empty value, so a requirement still present in the local .bloomCollection is ignored. The MoreDemandingOf logic only applies when the repository cannot be reached.
This is deliberate (documented in the comment, and covered by IsThisBloomTooOld_RepoHasWithdrawnTheRequirement_LetsThemBackIn) so an administrator can undo a mistake, but it also means anyone able to write the shared folder can lift the protection instantly for every member, and that a repository zip that is momentarily complete-but-old silently wins over the local file.
Worth confirming that is the intended trust model before this is relied on for Cloud syncing.
There was a problem hiding this comment.
[Claude Opus 5] Confirmed as deliberate, and left open for Andrew, because the second half of your question is genuinely his to answer rather than mine.
On the mechanics you describe: yes, and it was chosen with the alternative in front of us. Making the repository authoritative is what stops a mistaken requirement locking a team out permanently — a member who is refused never opens the collection, so their own copy never refreshes, and "whichever demands more" left them stranded for ever. Between briefly trusting a stale-but-readable repository copy and permanently stranding a team, the first is much the better failure.
On the trust model, which is the part worth Andrew's eye: "anyone who can write the shared folder can lift the protection" is not really a new power this introduces. The shared folder is where an administrator sets the requirement in the first place, and anyone who can write there can already edit the collection settings, add and remove books, and change checkout state. The protection is against an old Bloom mangling files, not against a hostile teammate.
That said, your framing is the right one for what comes next. This is preparation for Cloud syncing, and a cloud service is a different setting: there the requirement would presumably be something the service asserts rather than a line in a file every member can rewrite. If the flag ever needs to be tamper-resistant rather than advisory, this is the decision to revisit, and the code comment already says as much for the command-line paths that are ungated for the same reason.
A collection can declare, in its .bloomCollection file, the oldest Bloom allowed to open it. There is deliberately no UI: an administrator hand-edits <MinimumBloomVersion>6.6</MinimumBloomVersion> into the file, and in a Team Collection that reaches everyone through the shared folder. This is in preparation for Cloud syncing, where letting an older Bloom loose on a collection a newer one has touched could do real damage. An older Bloom is refused the collection and offered two ways forward: upgrade, or open a different one. Upgrading happens in place, through Velopack -- the same machinery behind the usual "a new version is available" toast, driven directly so it works before any collection is open, where there is no toast host to render one. We never send anyone to the website: if there is nothing newer on their channel, or the update fails, they are told so in a message box, in the words the toast would have used, and left to pick another collection. An update that does not reach the required version is still installed, because getting as far as the channel allows is real progress, and the message says plainly that they will be asked again next launch. Team Collections are locked out mid-session too: if the administrator sets a minimum while a teammate is working, the same modal dialog appears as soon as the change arrives, and the only ways out are to upgrade or to switch collections. Cancelling the collection chooser quits Bloom rather than leaving them where they are not allowed to be -- quitting has to stay possible, or someone with nothing newer to upgrade to and no other collection to open would have no way out but Task Manager. Keeping the setting alive turned out to be most of the work, because CollectionSettings.Save() rebuilds the file from what is in memory: - The value is read in Load and written in Save, so an ordinary save no longer discards a hand-added flag. - What the repository says is copied into the settings we hold, whenever we read it. Without that, a Bloom that started before the administrator's change would write the file back without the element and push it up, removing the protection for the whole team -- silently, and from the very Bloom it was meant to keep out. - That includes the administrator's own machine, which never gets a change notification for its own edit, so it is re-read straight after we push local files up. - A failed read means "we don't know", never "there is no minimum", so a part-written zip or a sync in progress cannot clear the setting. For a Team Collection the gate reads the repository's own copy, straight out of the shared folder, and that answer governs. Two things force this. Reading it at all is what makes the very first launch work: Bloom does not copy the repository's collection files down until later in startup, long after the gate has decided, so otherwise a member would be judged on yesterday's file and let in for the whole session. And letting it govern -- including when it says there is no requirement -- is what lets an administrator undo a mistake: a member who is being refused never opens the collection, so the sync that would refresh their own copy never runs, and a requirement typed in error would shut them out on every launch for ever. When we cannot reach the shared folder, or it is not a Team Collection at all, we fall back to the local file together with anything the repository told us earlier in the session, taking whichever demands more. Not being able to see the repository is not the same as the repository saying "no requirement", so an offline member keeps the protection rather than quietly losing it. That session-long memory is also what stops someone we have just shut out picking the same collection again and walking back in past a gate reading the stale file. The message shown after an upgrade that doesn't go far enough is deliberately terse. Every Bloom message box is a fixed 500x200 and this is the longest thing we ask it to hold, so it says what it must and stops. Version comparison is major.minor only, matching BookStorage's feature requirements and the book-download gate, so 6.5 is satisfied by any 6.5.x or later. An empty or unparseable value counts as no requirement -- nobody should lose their own collection to a typo. The channel plays no part: a version number means the same thing on Alpha, Beta and Release. Only the interactive ways of opening a collection are gated. The command-line commands and BulkUploader build a ProjectContext directly and are deliberately left alone: they have no user to read a dialog, and failing an overnight upload job is its own kind of damage. Not yet exercised against a live update feed or a real Team Collection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
69225b1 to
cbc0cb5
Compare
|
[Claude Fable 5 from Andrew Polk's machine during preflight] Superseded: this commit is now the first of the two commits on #8213, which is based directly on Version6.4 — the stacked arrangement (and the BL-16690-velopack-spike branch) is no longer needed. Closing to keep one PR for BL-16690. |
2 participants
Honors a
MinimumBloomVersionelement in a.bloomCollectionfile (set by hand today, inpreparation for Cloud syncing). If the running Bloom is older than that, it refuses to open the
collection and offers two ways forward: upgrade Bloom, or choose a different collection.
Upgrading happens in place, through Velopack — the same machinery behind the usual "a new
version is available" toast, driven directly so it works before any collection is open (there is no
toast host that early). We never send the user to the website: if there is nothing newer on their
channel, or the update fails, they are told so in a message box using the wording the toast would
have used, and are left to pick another collection.
An update that doesn't reach the required version is still installed — getting as far as the channel
allows is real progress, and the message says plainly that they will be asked again next launch.
Team Collections are locked out mid-session too: if the administrator sets a minimum version
while a teammate is working, the same modal dialog appears as soon as the change arrives, and the
only ways out are to upgrade or to switch collections. The repository's copy of the settings is read
straight out of the repo zip, since Bloom deliberately doesn't overwrite local collection settings
mid-session. A read failure never locks anyone out.
Version comparison is major.minor only, matching
BookStorage.CurrentBloomDesktopVersionand thebook-download gate. An empty or unparseable value is treated as "no requirement" — we never lock
someone out of their own collection over a typo. Channel plays no part: a version number means the
same thing on every channel.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16690
Caveat for the reviewer: none of the Velopack path or the Team Collection lock-out has been
exercised against a live update feed or a real Team Collection.
This change is
Devin review