Notice when Team Collection monitoring stops working, and go Disconnected (BL-16729) - #8338
StephenMcConnel wants to merge 23 commits into
Conversation
…artbeat (BL-16729) Two findings from Devin's review of PR #8338. Thread safety of the buffer-overflow path. HandleRepoWatcherError wrote to TeamCollectionMessageLog directly from a FileSystemWatcher callback. Both repo watchers can overflow at the same moment on different thread-pool threads, and the message log keeps one unsynchronized list which it enumerates to de-duplicate and then appends to -- while the UI reads that same list. Two simultaneous overflows, or one overflow during a UI read, could duplicate entries or throw. Worse, an exception escaping a watcher callback takes the process down. The overflow handling now goes to the UI thread the same way the disconnect path already does, and both watcher error handlers wrap their whole body so nothing can escape into the callback. Heartbeat integration was untested -- the existing tests exercised ConnectionFailureTracker's policy but never drove ConnectionHeartbeat.Tick, so the guards, the confirm-then-act sequencing, and disposal were all uncovered. Added eight tests that drive Tick directly with no timer, no network and no real repo: connection fine, one failure (waits), two in a row (disconnects), recovery in between (starts over), writing-to-repo and not-the-live-collection (skip and reset), post-dispose ticks are inert, and Start is a no-op under unit tests. That needed two seams on TeamCollection: IsLiveCollection (virtual, so a test can say whether this is the manager's current collection without standing up a live TeamCollectionManager) and ReportConnectionProblem (routing through the ITeamCollectionManager interface rather than the concrete TCManager, so it is mockable). Also documented what CheckConnection_QuietProbe_WritesNoMessages does not prove: the History writes it guards need a Dropbox-hosted repo, which a temp folder cannot reproduce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…message-log locking (BL-16729) Three more findings from Devin's re-review of PR #8338. A probe that throws no longer preserves the previous failure. Tick's catch reported to Sentry and fell through, leaving the strike on the record -- so a failure, then a throwing probe, then another failure counted as two consecutive failures and would disconnect a collection that was never actually shown to be unreachable twice running. An exception tells us nothing either way, so it now breaks the run like any other skipped tick. The overflow warning no longer writes into an abandoned collection. If a racing watcher failure disconnected us while the warning was queued for the UI thread, MakeDisconnected had already swapped in a DisconnectedTeamCollection with its own message log, so the warning landed somewhere the status dialog no longer reads. HandleLostNotifications now checks IsLiveCollection before writing -- and "you may have missed some changes" is moot next to "you have lost contact with the collection" anyway. TeamCollectionMessageLog is now internally synchronized. Marshalling the overflow path to the UI thread (previous commit) closed the case Devin originally described, but not the general one: RunOnUiThreadLater runs inline when there is no window, and several API endpoints registered with handleOnUiThread false already reached WriteMessage from server threads via CheckConnection. Since WriteMessage enumerates Messages to de-duplicate and then appends to it, while the status properties enumerate the same list, an overlap could duplicate entries or throw InvalidOperationException. The check-and-append is now one atomic step and the status properties take the same lock. The status-changed event is deliberately raised outside that lock: it reaches WinForms and the websocket server, and holding a lock across that is how deadlocks happen. Not addressed: enumerating the public Messages list directly from outside the class is still unguarded. Nothing mutates it externally, and fixing it properly means returning snapshots rather than the live list -- a wider change than this branch should carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] Consulted Devin on 2026-09-09, five times, most recently up to commit One review per pushed commit. Devin accumulates findings across rounds and re-lists ones already fixed, so the count below is of distinct findings: 13 — 11 closed, 2 still open. Fixed in code (8): watcher-callback thread safety; the untested heartbeat; probe exceptions preserving an outage strike; a stale overflow warning written into an abandoned collection; the non-atomic disconnect claim; message-log locking, then snapshots once Devin showed the readers really are off the UI thread; a silently dropped watcher failure during collection setup; and a failed sync leaving the heartbeat permanently switched off. Closed with an answer rather than a change (3): the Still open, both facets of one question — should a Team Collection we cannot watch be treated as disconnected? Missing book watcher stays connected and New collection watcher failures ignored. The developer scoped that out when this work was planned, so reversing it is their call; both threads stay open until they answer. Three of the fixes above are pre-existing bugs rather than anything this PR introduced — all three the same shape, a busy flag cleared only where the work finishes normally, and all three newly load-bearing because the heartbeat consults those flags. Other reviewers: CI ( |
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] Consulted Devin again on 2026-09-10, up to commit Distinct findings across the whole PR now 16, all 16 closed. New this run:
The two findings that were open awaiting the developer are now closed with their decision recorded on each thread: a Team Collection we cannot watch is not treated as a disconnection. Instead No review thread is left open. CI passed at every commit; CodeRabbit remains switched off on this repo ( |
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] Consulted Devin up to commit Two more since the last log entry, both from the developer's manual test of a lost drive:
Also worth recording from that testing session: No review thread is left open. CI passed at every commit; CodeRabbit remains switched off on this repo. |
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during pr-ready-for-human] Consulted Devin on 2026-09-10 up to commit All 17 distinct findings across this PR remain closed, each with a documented outcome on its own thread. No review thread is open. |
|
@StephenMcConnel |
There was a problem hiding this comment.
I'm inclined to think master/6.6 is more appropriate for a target. I'll go ahead and make that change to the PR.
@StephenMcConnel reviewed 7 files, made 1 comment, and resolved 17 discussions.
Reviewable status: 0 of 12 files reviewed, all discussions resolved.
…cted (BL-16729) Bloom watches the Team Collection's shared folder with FileSystemWatchers so it notices teammates' changes. If that folder dropped out mid-session, nothing noticed: nobody subscribed to FileSystemWatcher.Error, and CheckConnection() only ran when the user did something (checkout, check-in, delete). The user kept working, believing they saw the current state of the collection, while their teammates' work went unseen until Bloom was restarted. Bloom now notices by two routes, both funnelling into the existing disconnected state (yellow Team Collection button, disconnected book-status panel, Reload Collection button), plus a persistent toast, because a recoloured button is easy to miss: - Both repo watchers subscribe to Error. A dead watch disconnects immediately with no retry: .NET never re-establishes one, so even a returning folder would never produce another event. - A new ConnectionHeartbeat re-checks every 60 seconds. This is the only way to notice that Dropbox has stopped syncing, where the folder is still there and we simply stop receiving other people's work. It is owned by Start/StopMonitoring, so it is silent during SyncAtStartup, absent on a DisconnectedTeamCollection, and stops when we disconnect or dispose. ConnectionFailureTracker holds a two-strikes rule (re-check after 15s, act only on a second failure of the same kind), because the things CheckConnection looks at can lie and there is no automatic way back from a wrong disconnect. InternalBufferOverflowException is deliberately not a disconnect: the folder is reachable, we merely lost notifications. Buffers go to 64KB to make it rare, and if it still happens the user gets a warning toast and the Reload button while staying connected. Supporting changes: - CheckConnection gains a quiet-probe overload. Without it, polling would append an un-deduplicated History message and raise a status-changed event on every tick of a healthy LAN-share session. - MakeDisconnected is now idempotent (returns false if we were already disconnected, so racing callers don't double-log or double-toast), stops the outgoing collection's watchers, and keeps it for Dispose. Previously it nulled CurrentCollection without stopping it and Dispose could no longer reach it, so the abandoned collection went on watching and queueing changes forever. - PutBookInRepo and SetBookStatusString now clear _writeBookInProgress in a finally. A throw used to leave it set for the rest of the session, which suppressed change notifications for that book -- and would have silently killed the new heartbeat, in exactly the flaky-share case it exists to catch. - The three EnableRaisingEvents calls are guarded; BL-16679 was a crash from one. - CollectionsTabBookPane.tsx set `disconnected` where the interface field is `isDisconnected`, so a failed status fetch rendered the book as available for checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…artbeat (BL-16729) Two findings from Devin's review of PR #8338. Thread safety of the buffer-overflow path. HandleRepoWatcherError wrote to TeamCollectionMessageLog directly from a FileSystemWatcher callback. Both repo watchers can overflow at the same moment on different thread-pool threads, and the message log keeps one unsynchronized list which it enumerates to de-duplicate and then appends to -- while the UI reads that same list. Two simultaneous overflows, or one overflow during a UI read, could duplicate entries or throw. Worse, an exception escaping a watcher callback takes the process down. The overflow handling now goes to the UI thread the same way the disconnect path already does, and both watcher error handlers wrap their whole body so nothing can escape into the callback. Heartbeat integration was untested -- the existing tests exercised ConnectionFailureTracker's policy but never drove ConnectionHeartbeat.Tick, so the guards, the confirm-then-act sequencing, and disposal were all uncovered. Added eight tests that drive Tick directly with no timer, no network and no real repo: connection fine, one failure (waits), two in a row (disconnects), recovery in between (starts over), writing-to-repo and not-the-live-collection (skip and reset), post-dispose ticks are inert, and Start is a no-op under unit tests. That needed two seams on TeamCollection: IsLiveCollection (virtual, so a test can say whether this is the manager's current collection without standing up a live TeamCollectionManager) and ReportConnectionProblem (routing through the ITeamCollectionManager interface rather than the concrete TCManager, so it is mockable). Also documented what CheckConnection_QuietProbe_WritesNoMessages does not prove: the History writes it guards need a Dropbox-hosted repo, which a temp folder cannot reproduce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…message-log locking (BL-16729) Three more findings from Devin's re-review of PR #8338. A probe that throws no longer preserves the previous failure. Tick's catch reported to Sentry and fell through, leaving the strike on the record -- so a failure, then a throwing probe, then another failure counted as two consecutive failures and would disconnect a collection that was never actually shown to be unreachable twice running. An exception tells us nothing either way, so it now breaks the run like any other skipped tick. The overflow warning no longer writes into an abandoned collection. If a racing watcher failure disconnected us while the warning was queued for the UI thread, MakeDisconnected had already swapped in a DisconnectedTeamCollection with its own message log, so the warning landed somewhere the status dialog no longer reads. HandleLostNotifications now checks IsLiveCollection before writing -- and "you may have missed some changes" is moot next to "you have lost contact with the collection" anyway. TeamCollectionMessageLog is now internally synchronized. Marshalling the overflow path to the UI thread (previous commit) closed the case Devin originally described, but not the general one: RunOnUiThreadLater runs inline when there is no window, and several API endpoints registered with handleOnUiThread false already reached WriteMessage from server threads via CheckConnection. Since WriteMessage enumerates Messages to de-duplicate and then appends to it, while the status properties enumerate the same list, an overlap could duplicate entries or throw InvalidOperationException. The check-and-append is now one atomic step and the status properties take the same lock. The status-changed event is deliberately raised outside that lock: it reaches WinForms and the websocket server, and holding a lock across that is how deadlocks happen. Not addressed: enumerating the public Messages list directly from outside the class is still unguarded. Nothing mutates it externally, and fixing it properly means returning snapshots rather than the live list -- a wider change than this branch should carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d setup failures (BL-16729) MakeDisconnected now claims the transition atomically. Removing the earlier Interlocked gate (it could latch permanently and disable every future disconnect) left the guard non-atomic: callers arrive both directly, from a synchronous CheckConnection on a BloomServer thread, and indirectly, from a watcher or heartbeat failure marshalled onto the UI thread, so two could capture the same live collection, both pass the guard, and both go on to stop it and build a replacement. A short lock now covers just the claim -- read CurrentCollection, null it, set an in-progress flag -- and the rest runs outside the lock, since it writes to the message log and that raises an event reaching WinForms and the websocket server. The flag is cleared in a finally within the same synchronous method, so unlike the old gate it cannot latch. Added MakeDisconnected_ManyThreadsAtOnce_DisconnectsExactlyOnce, which asserts on the winner count and on message counts rather than only the resulting object, because the log de-duplicates Errors and would hide a double disconnect. TeamCollectionMessageLog.Messages is now a locked snapshot. The previous commit synchronized the log's own reads and writes, but callers still received the live list -- and teamCollection/getLog and teamCollection/logImportant are both registered with handleOnUiThread false, with HandleLogImportant enumerating it directly. A status change appending on the UI thread during one of those requests would throw "Collection was modified". The backing list is now private and every internal use is inside the lock. Only one production caller was reading the property, so this is contained. A connection problem raised with no current collection is no longer dropped silently. During ConnectToTeamCollection a brand-new collection is set up -- including StartMonitoring -- before being published as CurrentCollection, so a watcher that fails to start in that window had nowhere to report. It now goes to the log and to Sentry. Whether that case should disconnect outright is the open question already on the PR about collections we cannot watch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 11 files and all commit messages, made 3 comments, and resolved 8 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on StephenMcConnel).
src/BloomExe/Program.cs line 2815 at r11 (raw file):
// back to running inline: this work exists to be done on the UI thread, and // doing it on a watcher thread instead would trade a missed notification for a // data race. At this point there is nobody left to notify anyway.
This comment is strange. It might apply IF the exception is something to do with the uiContext being null or disposed, but is there some reason that's the only exception that could happen here? Will we see exceptions that happen while running the action in the context's thread, or is it up to the action to handle those itself? If the action must handle its own exceptions, that should be documented.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1143 at r11 (raw file):
return; } if (!startedWatching)
I can't find StartBooksWatcher, so I'm not sure what it returns. But if false means we didn't start watching, then it's at least somewhat surprising that we exit this with _booksWatcherDeferred changed to false. If it's not a problem, I think it deserves a comment explaining why we no longer want to keep attempting to watch.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1184 at r11 (raw file):
} NoticeBooksThatLeftTheRepoBeforeWeStartedWatching(bookNames);
It's bizarre that the call to this is part of NoticeBooksThatArrivedBeforeWeStartedWatching. I think it would be more natural to call this from the (one) place that NoticeBooksThatLeftTheRepoBeforeWeStartedWatching is called.
If we need a single master method, it should be something like NoticeChangesToTheRepoBeforeWeStartedWatching, and call both the other methods.
When the Books folder shows up late and we finally start watching it, we have to work out for ourselves everything that happened while no watcher was running. That catch-up work was one method that fetched the book list, announced the books that had arrived, and then called the "books that left" half from its own tail. Give it a named wrapper, NoticeChangesToTheRepoBeforeWeStartedWatching, that fetches the list once and calls both halves, so the two directions read as peers rather than one hiding inside the other. Also clarify two comments that described their code inaccurately: - Program.RunOnUiThreadLater's catch only wraps the Post itself, not the posted action -- Post is asynchronous, so the action's own exceptions never reach it. Say that, and say why the Post can throw at all (a torn-down context). - FolderTeamCollection.RetryDeferredWatching's bare `return` when the watcher declines to start now explains itself: TryStartWatching only returns false after HandleRepoWatcherError has already told the user and moved Bloom's state on, and retrying from there is not worth the complexity, so we stop watching for the rest of the session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tionWhenDropboxStops
The merge commit 04e5b13 staged these three generated fixtures, and the pre-commit hook's pretty-quick formatted them, because at that point master did not yet carry the .prettierignore rule that keeps prettier off them. That broke four OverflowChecker tests: prettier indents the markup inside each test box, and the added whitespace changes the very text metrics the overflow tests measure, so three boxes that are supposed to overflow no longer did. Master has since gained the ignore rule (it arrived in the merge just made), so restoring the files to master's content is now stable -- the hook leaves them alone from here on. These files are generated from their .pug sources and should never be hand- edited or reformatted anyway; the note at the top of each says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-16729) The ignore rule for these fixtures has never actually worked. It was written as bookEdit/OverflowChecker/*Fixture.html which prettier matches only against a path relative to src/BloomBrowserUI. The pre-commit hook runs pretty-quick, and pretty-quick resolves the git root and hands prettier repo-root-relative paths (src/BloomBrowserUI/bookEdit/...), which that pattern never matches. So the rule protected the fixtures when prettier was invoked by hand from the front-end folder, and not at all on the path that matters. The merge commit 04e5b13 is where that bit: it staged the three fixtures, the hook reformatted them, and four OverflowChecker tests started failing -- prettier indents the markup inside each test box, and the added whitespace changes the very text metrics the overflow tests measure, so boxes that are meant to overflow no longer did. Anchoring the pattern with **/ makes it match either spelling. Confirmed with prettier --file-info both ways, and the fixtures now survive being staged. Restores the three files to master's content; OverflowChecker is back to 28/28. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
StephenMcConnel
left a comment
StephenMcConnel
left a comment
There was a problem hiding this comment.
@StephenMcConnel+CLAU made 3 comments.
Reviewable status: 12 of 21 files reviewed, 3 unresolved discussions (waiting on JohnThomson).
src/BloomExe/Program.cs line 2815 at r11 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This comment is strange. It might apply IF the exception is something to do with the uiContext being null or disposed, but is there some reason that's the only exception that could happen here? Will we see exceptions that happen while running the action in the context's thread, or is it up to the action to handle those itself? If the action must handle its own exceptions, that should be documented.
[Claude Opus 5 (1M context)] You were right that the comment did not describe the code, and it now says what you asked for.
The try only wraps uiContext.Post(...), and Post is asynchronous -- it queues the action and returns. So the action runs later, on the UI thread, and its exceptions never come back to this catch. Nothing here could ever have caught them.
That leaves the Post itself, and the realistic way it throws is the one you guessed: the synchronization context (or the underlying window) has been torn down as we are posting, i.e. we are shutting down. We report that to Sentry rather than crashing, because it is unexpected enough to be worth knowing about.
And yes -- it is up to each action to handle its own exceptions, which is now stated outright rather than left to be inferred. The new comment reads:
// Each action() is responsible for its own exception handling as needed.
// This catch is largely a safety net for the Post itself, handling the case where
// the uiContext has been disposed just as the Post is being called. We don't want
// to crash the app because of that, and we do want to report it to Sentry because
// it is unexpected and should be investigated.
Program.cs line 2944 in ec600edd2.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1143 at r11 (raw file):
Previously, JohnThomson (John Thomson) wrote…
I can't find StartBooksWatcher, so I'm not sure what it returns. But if false means we didn't start watching, then it's at least somewhat surprising that we exit this with _booksWatcherDeferred changed to false. If it's not a problem, I think it deserves a comment explaining why we no longer want to keep attempting to watch.
[Claude Opus 5 (1M context)] Fair -- it was surprising, and it now explains itself.
For the record, since you could not find it: StartBooksWatcher is in the same file, just above RetryDeferredWatching. It builds the FileSystemWatcherWrapper and ends with return TryStartWatching(_booksWatcher, booksPath);. TryStartWatching (in TeamCollection.cs) sets EnableRaisingEvents = true and returns true; on an exception it calls HandleRepoWatcherError and returns false.
So a false return is not a quiet failure: by the time we see it, HandleRepoWatcherError has already told the user and moved Bloom into its disconnected state. Retrying from there would mean re-entering a state transition that has already been announced, which is why leaving _booksWatcherDeferred false -- giving up on watching for the rest of the session -- is the deliberate choice rather than an oversight. The added comment now says so:
// The watcher threw an exception instead of starting, which is already reported to
// the user, changing the state of the program and display. It's too tricky to try
// again later, so give up on watching for the rest of this session.
FolderTeamCollection.cs line 1143 in ec600edd2.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1184 at r11 (raw file):
Previously, JohnThomson (John Thomson) wrote…
It's bizarre that the call to this is part of NoticeBooksThatArrivedBeforeWeStartedWatching. I think it would be more natural to call this from the (one) place that NoticeBooksThatLeftTheRepoBeforeWeStartedWatching is called.
If we need a single master method, it should be something like NoticeChangesToTheRepoBeforeWeStartedWatching, and call both the other methods.
[Claude Opus 5 (1M context)] Agreed, and done exactly as you suggested -- including the name.
There is now a master method, NoticeChangesToTheRepoBeforeWeStartedWatching, which fetches the book list once and calls both halves as peers:
NoticeBooksThatArrivedBeforeWeStartedWatching(bookNames);
NoticeBooksThatLeftTheRepoBeforeWeStartedWatching(bookNames);
NoticeBooksThatArrivedBeforeWeStartedWatching now takes the list as a parameter instead of fetching it, and no longer calls the other half from its tail. RetryDeferredWatching calls the master method, which is the one place either half is reached from.
FolderTeamCollection.cs line 1163 in ec600edd2.
Two findings from this run's local review. **A watcher could be left running on a collection nobody is using.** RetryDeferredWatching checks IsMonitoring and then, further down the same posted callback, enables the Books watcher. Those two points are not as close together as they look: the callback runs on the UI thread, but StopMonitoring does not have to -- SynchronizeRepoAndLocal calls it from the sync progress dialog's background worker. If monitoring ends in between, StopMonitoring's watcher teardown has already run and will not run again (Dispose only calls StopMonitoring while _monitoring is true), so the watcher enabled a moment later would go on raising repo events into a collection that has been put aside -- exactly the leak the rest of this change exists to prevent. The callback now re-checks IsMonitoring after starting the watcher and takes it down itself, reading the field into a local so it stays correct if StopMonitoring gets there first. **The test for the "no timers under unit tests" guard could not fail.** HeartbeatStart_UnderUnitTests_DoesNotStartATimer asserted that no connection check had happened yet. But the first tick is IntervalMs -- a minute -- away, so that count is zero whether the guard is there or not: the test passed while proving nothing, and would have leaked the very thread-pool timer it exists to rule out. It now asserts on the timer itself, via a new internal IsTimerArmed. Verified by deleting the guard and watching the test fail, then restoring it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by Devin this run, and real: after a disconnect part way through a session, every book in the collection would have presented as a newly created local book -- no checkout status, nothing showing as being in the repo. DisconnectedTeamCollection answers book-status questions out of the local status files, and GetBookStatusJsonFromRepo only trusts a local status whose collectionId matches its own CollectionId; anything else it reports as "not in the repo", i.e. created locally. Nothing sets that id on a collection object after the collection has been opened -- WorkspaceModel calls SetCollectionId once -- so until now the stand-in was always built early enough in startup for that call to reach it. This PR is what changes that: noticing a dead watcher or a failed heartbeat and going Disconnected is, by design, something that now happens mid-session, long after SetCollectionId has run. The stand-in built at that point kept a null CollectionId, and every book's status comparison failed. CompleteDisconnect now carries the id across from the collection it is replacing, falling back to the manager's settings. Covered by TeamCollectionManagerTests.MakeDisconnected_MidSession_ StandInInheritsTheCollectionId, which fails without the one-line fix (verified by removing it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] Consulted Devin on 2026-09-28, four times, most recently up to commit What this run changed about the Devin record: Devin carries every finding forward across rounds, and twelve of them had never been mirrored to this PR — seven Bugs and five Investigate flags, some dating back to commits from earlier sessions. They each now have their own thread with a documented outcome, so nothing is sitting only on Devin's website any more. Eight are closed:
Four are left open for the developer, because acting on them would widen a change a reviewer has already called large: Repeat overflows lose reload warnings, Remote checkouts remain invisible after recovery, Failed recovery scan permanently loses book changes, and Vanished folder aborts watcher startup. Each thread says what the user would actually experience and what fixing it would cost. CI ( |
**Let the "may have missed changes" warning through again after a Reload.** The message log drops an error whose id already occurred this session. That is right for almost everything -- a bad zip file in the repo produces the same complaint over and over -- but wrong for the one message whose whole content is "please click Reload Collection": once the user has done that, another occurrence is telling them something new, and swallowing it leaves them believing they are up to date when Bloom has just decided they are not. The de-duplication scan now starts at the last Reloaded milestone for ids listed in kErrorsRepeatableAfterReload, which so far means only that one. Ordinary errors still de-duplicate across the whole session, and a test asserts that too. **Notice a teammate's checkout during the late-start catch-up scan.** The scan asked HasBeenChangedRemotely, which compares book checksums -- and checking a book out or in does not change a book's checksum, it changes the status stored beside it. So when the Books folder arrived late and we started watching part way through a session, a book a teammate had checked out meanwhile went on presenting itself as available, which is how two people end up editing the same book. The live watcher never had this gap because it reacts to the repo file being written at all. New HasCheckoutChangedRemotely compares the repo's lockedBy/lockedWhere against the local copy's; lockedWhen is deliberately left out, since a check-in followed by another checkout from the same machine leaves those two the same and any content change is caught by the checksum test anyway. **Guard the startup watcher the way the retry path is already guarded.** StartMonitoring tests that the Books folder exists and then assigns FileSystemWatcher.Path, which throws if the folder has gone in between. That exception escaped StartMonitoring, so nobody was told and Bloom did not go Disconnected. RetryDeferredWatching was taught to catch exactly this race earlier in the review; the startup path now reports it the same way. Also moves the message log's quick append into RobustFileIO, as QuickAppendAllText. That call was written against File.AppendAllText deliberately -- RobustFile's retry runs for seconds, and this often runs on the UI thread -- but the repo's pre-commit check quite rightly refuses a raw File call outside RobustFileIO.cs, which is where Bloom keeps its reviewed uses of that API. It had slipped past the hook when it was first written, and blocked this commit. No behaviour change; the retry policy and the encoding are the same, they just live where the rule says they should. Tests: the message-log narrowing and both directions of the checkout comparison are covered, and I checked the message-log test fails with the narrowing removed. The one-line call-site change in the catch-up scan and the StartMonitoring guard are not separately covered -- the first needs the scan's event plumbing and the second needs the folder to vanish between two adjacent statements. Decision 3 (a failed catch-up scan never retries) was deliberately left as is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] @andrew-polk — answering the second half of your question properly, now that the first half is settled (this is targeted at You're right that it's bigger than it looks for a bug report that reads "Bloom stops noticing changes". Here is what the size is actually made of, so you can judge it rather than take my word for it. Why there has to be a periodic check at all. The obvious fix — "listen harder to the file system watcher" — does not work. When Dropbox stops syncing, nothing happens to the folder: it is still there, still readable, and the watcher sits there perfectly healthy while we simply stop being told about other people's work. There is no event to listen for. The only way to find out is to go and ask, on a timer. That is Why it can't just disconnect on the first bad answer. Going Disconnected is a one-way door for the user: there is no automatic recovery, they have to fix the real problem and click Reload Collection. So a false positive costs someone their whole Team Collection session for no reason. A single probe is not reliable evidence — one dropped packet to dropbox.com, a Wi-Fi roam, a network share that stutters for a second. Hence Why the thread handling is fiddly. The three places that now conclude "we have lost contact" all run on background threads — two file-system watcher callbacks and the heartbeat's timer. What they trigger touches WinForms and the message log. Getting that wrong deadlocks Bloom or corrupts the log, and we found both during review. So the detection, the confirmation delay, and the marshalling are the irreducible core, and together they are most of the new code. The parts that are arguably separable, in the interest of not overselling this:
If you want this smaller, that second one is the piece I'd lift out — say the word and I'll move it to its own card. I'd keep the message-log fix here, since John has already reviewed it in place and it's the sort of thing that never gets picked back up on its own. One thing worth weighing on the other side: preflight found, and this branch now fixes, a bug that only exists because we can now disconnect mid-session — every book in the collection was about to start looking like a newly created local book, with no checkout status at all. That's the kind of thing that surfaces when the whole path is exercised together, and it argues a little for keeping the core intact rather than landing it in slices. |
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] Consulted Devin up to commit Two were new bugs, and neither survived examination:
The other two were its earlier localization and flaky-e2e flags re-raised under new titles; both were answered and resolved earlier in this review, and the threads say so. What changed in this commit, from the decisions:
Full C# suite green at this commit: 3643 passed, 0 failed, 1 skipped. Front-end typecheck and lint clean; the vitest suite was not re-run because nothing in this commit is TypeScript (it was green at 971 on the previous commit). CI ( |
3 participants
Team Collection now notices when it can no longer see the shared folder, goes Disconnected, and tells the user — instead of carrying on looking normal while their teammates' work goes unseen.
The problem
Bloom watches the Team Collection's shared folder (Dropbox or a LAN share) with
FileSystemWatchers. If that folder dropped out mid-session, nothing noticed: nobody subscribed toFileSystemWatcher.Error, andCheckConnection()only ran when the user did something (checkout, check-in, delete). Someone reading and editing their own checked-out book could go a whole session on stale data.How it notices
Two routes, both funnelling into the existing disconnected state (yellow TC button, disconnected book-status panel, Reload Collection button):
ConnectionHeartbeat— re-checks every 60s. This turns out to be the mechanism that does the real work (see Testing): it is the only thing that notices Dropbox has stopped syncing, where the folder is still there and we simply stop receiving other people's work, and in practice it is what catches a vanished folder too. Owned byStart/StopMonitoring, so it is silent duringSyncAtStartup, absent on aDisconnectedTeamCollection, and stops when we disconnect or dispose. One-shot re-arming makes overlapping ticks structurally impossible.Error— both repo watchers now subscribe. Kept as a cheap best-effort second route, and it is what handles buffer overflow, but manual testing showed Windows does not raise it when the media goes away, so it cannot be relied on for that. When it does fire for a dead watch we disconnect immediately with no retry: .NET never re-establishes a dead watch, so even a returning folder would never produce another event.ConnectionFailureTrackerholds a two-strikes rule (re-check after 15s; act only on a second failure of the same kind). The thingsCheckConnectionlooks at can lie — one dropped packet fails the dropbox.com probe, a Wi-Fi roam briefly killsGetIsNetworkAvailable()— and there is no automatic way back from a wrong disconnect.How the user finds out
A persistent, non-modal toast (
ToastService, the same mechanism as the existing TC clobber toast), because a recoloured top-bar button is easy to miss. Clicking it opens the TC dialog. Raised fromNoticeConnectionProblemrather thanMakeDisconnected, so it fires only for mid-session discoveries — not at startup (no workspace yet) or for subscription-tier disabling (wrong wording).InternalBufferOverflowExceptionis deliberately not a disconnect: the folder is reachable, we merely lost notifications. Buffers go to 64KB to make it rare; if it still happens the user gets a warning toast and the Reload button while staying connected.Supporting changes
CheckConnectiongains a quiet-probe overload. Without it, polling would append an un-deduplicated History message and raise a status-changed event on every tick of a healthy LAN-share session.MakeDisconnectedis now idempotent (returns false if already disconnected, so racing callers don't double-log or double-toast), stops the outgoing collection's watchers, and keeps it forDispose. It previously nulledCurrentCollectionwithout stopping it, andDisposecould then no longer reach it — so the abandoned collection went on watching and queueing changes forever.PutBookInRepo/SetBookStatusStringnow clear_writeBookInProgressin afinally. A throw used to leave it set for the rest of the session, suppressing change notifications for that book — and would have silently killed the new heartbeat, in exactly the flaky-share case it exists to catch.EnableRaisingEventscalls are guarded; BL-16679 was a crash from one.CollectionsTabBookPane.tsxsetdisconnectedwhere the interface field isisDisconnected, so a failed status fetch rendered the book as available for checkout.Testing
31 new unit tests (239 in the TeamCollection fixtures); full C# and front-end suites green.
Manually exercised in a running Bloom, which corrected a premise of the original design:
FileSystemWatcher.Errordoes not fire when the media is removed. This is from Bloom's own event log, not inference — the diagnostic lines added in this PR make it visible. So the heartbeat is what actually catches these cases, and the honest detection latency is ~75 seconds, not the "immediate" the watcher route implies. Pulling asubstdrive is slower still: existing handles stay valid, so nothing notices until the next probe or the next user action.Not covered: Dropbox-on-LAN, for want of a setup to test it on.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16729
Devin review
This change is