Notice when Team Collection monitoring stops working, and go Disconnected (BL-16729) - #8338
StephenMcConnel wants to merge 15 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>
… (BL-16729) Same failure mode as the _writeBookInProgress bug fixed earlier on this branch, and the new heartbeat depends on this flag too. SyncAtStartup sets _syncIsRunning and clears it on its normal return and on its two explicit abort paths. Any other exception escapes to the broad catch in SynchronizeRepoAndLocal, which reports the problem and lets Bloom carry on monitoring -- with the flag still set. Since IsWritingToRepo consults it, the periodic connection check then skipped every tick for the rest of the session: Dropbox could stop later and nothing would ever look. Rather than re-indent a 570-line method into a try/finally, SyncAtStartup is now a thin wrapper that owns the flag in a finally and delegates the work to SyncAtStartupInternal. The inner method's existing assignments are left alone; they are now redundant but harmless, and leaving them keeps the diff to the actual fix. SyncAtStartup_Throws_StillClearsTheBusyFlag covers it, using Assert.Catch so the test fails rather than passes vacuously if the sync stops throwing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves the open review question about a Team Collection whose repo has no Books folder. The folder is created when a collection is set up, so the only way to be missing it is to be a joiner whose Dropbox has not delivered it yet. In that state there is nothing to miss: no teammate can have checked a book in. So this is deliberately NOT treated as a disconnection -- reporting one would be wrong and, for a first-time join, routine. What was genuinely broken is that we gave up for the whole session. The folder can arrive minutes later, and nothing looked again, so book changes went unseen until Bloom restarted. StartMonitoring now records that watching is deferred, and the periodic connection check retries: once the folder appears it starts the watcher and announces whatever is already sitting in it, since from the watcher's point of view those books are all new since Bloom started. The old early return also skipped the Other watcher, so a missing Books folder silently cost us collection-settings notifications too. Now only the books watcher is deferred. Considered and rejected: a single recursive watcher on the whole repo folder, which would notice the Books folder instantly and need no catch-up. One watcher means one NotifyFilter, so the Other folder would gain FileName/DirectoryName events it does not get today -- and that path reaches CheckWhetherRepoNowRequiresANewerBloom, which blocks in a modal and can shut the user out of the collection. It would also make the debounce helpers, which are per-watcher-per-event-type, share state between the two kinds of change, and add Lost and Found and our own Books/Temp write traffic to the buffer. That puts the risk on the path that works today for every collection, to fix a case that only affects a mid-sync joiner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…6729) Devin's follow-up on the message-log locking: AfterMessageAdded appended to log.txt outside _messagesLock, so two threads writing at the same moment could reach disk in a different order than the in-memory list, and could collide over the file itself (RobustFile.AppendAllText retries IO errors but does not serialize callers). The append now happens inside the lock, alongside adding to the list, so the file ends up in the same order as memory and only one thread is ever appending. It is a short append; holding the lock across it costs little. The status-changed event still goes outside the lock, because that one reaches WinForms and the websocket server and holding a lock across it invites deadlock -- which is why the two were split in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while working through a manual test report. RunOnUiThreadLater called Shell.GetShellOrOtherOpenForm() outside its try. That reaches Application.OpenForms, which is not thread-safe, and every caller of this method is on a file system watcher's thread or the heartbeat's. If it threw, the exception unwound past NoticeConnectionProblem and ReportConnectionProblem into ConnectionHeartbeat.Tick's catch, which reports to Sentry, resets the tracker and moves on -- so the disconnect was silently swallowed and Bloom went on looking connected. It is now inside the try, and a failure falls back to running the action inline, which is what the no-window case already did. Also log when the periodic check finds and then confirms a problem. Without this, somebody testing a real outage cannot tell "the check ran and decided" from "the check never ran at all" -- which is exactly the ambiguity that came up in testing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Devin's follow-up on the previous commit: making the failed OpenForms lookup fall back to running inline traded "the disconnect never happens" for "the disconnect happens on a watcher thread", which is not obviously the better bargain -- this work is marshalled precisely because it should not run there. Removed the form lookup instead. RunOnUiThreadLater now posts to Program.MainContext, the WinForms synchronization context captured once at startup, which is what ToastService already uses for its callbacks. That is better than either version: it never enumerates Application.OpenForms, so the thread-safety problem this started with cannot arise; Post is asynchronous even when called from the UI thread, which is a hard requirement because TryStartWatching calls in from the middle of StartMonitoring; and there is no form whose liveness has to be checked and which can be disposed between the check and the call. Neither remaining path runs UI work on a background thread. A null MainContext means no UI thread exists at all -- unit tests, or before Application.Run -- so nothing can be racing us and the action runs inline. If Post throws, the context is being torn down, so that reports to Sentry and deliberately does not fall back to inline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Manual testing (2026-09-10) disproved the justification I had written. The comment claimed the deferred case exists for a joiner whose Dropbox has not delivered the Books folder yet. You cannot join a collection in that state at all: the join fails earlier, in GetBookList, which throws on the missing folder. What is actually reachable is opening or reloading a collection already joined, at a moment when Books is absent -- Dropbox still restoring the shared folder onto a new machine, for instance. StartMonitoring runs on every collection open, not only at join. SyncAtStartup will have failed and told the user; what the retry avoids is having to restart Bloom once the folder finally arrives. Comment only; no behaviour change. Kept deliberately after weighing it: the failure mode is inert, and the same restructure stops a missing Books folder from silently skipping the Other watcher as well, which is a separate real gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6790f07 to
2bfde49
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 12 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on StephenMcConnel).
src/BloomExe/TeamCollection/ConnectionHeartbeat.cs line 66 at r8 (raw file):
/// no timer involved. /// </summary> internal void Tick(object unused)
Let's give this a name that reflects what it actually does...something like UpdateTcFileWatching or UpdateTcConnectionStatus.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1159 at r8 (raw file):
HandleModifiedFile( new BookRepoChangeEventArgs { BookFileName = bookName + ".bloom" } );
This probably mirrors the normal watching code, but is it consistent with that for us to ignore deleted books? There's also the case of a rename, which will typically look like a new book (plus a deletion), though in pathological cases involving multiple renames some might come through as modifications.
src/BloomExe/TeamCollection/TeamCollectionManager.cs line 557 at r8 (raw file):
/// and which every caller here would be enumerating from a background thread. /// </summary> internal static void RunOnUiThreadLater(Action action)
This feels like a generic function that doesn't particularly belong in TCM. Is there a better place for it?
src/BloomExe/TeamCollection/TeamCollectionMessageLog.cs line 271 at r8 (raw file):
try { RobustFile.AppendAllText(_logFilePath, toPersist);
This is normally a short operation, but if there are problems RobustFile will retry over a period of seconds. Is it a problem to hold the lock that long? WriteError might also involve RobustFile-type possible delays.
One possible strategy, if it's a problem, would be to make a member variable with a list of messages to write. Inside the lock, we push new messages. Persist can then lock only long enough to copy and empty the list; outside the lock, it can take its time writing them.
…-16729) TeamCollectionMessageLog.WriteMessage appended to log.txt while holding _messagesLock, so a slow append blocked every reader of the in-memory log -- Messages, CurrentErrors, TeamCollectionStatus -- and TeamCollectionStatus is read on the UI thread whenever the Team Collection button refreshes. The append is normally a matter of microseconds, but when something else has the file open RobustFile retries over a period of seconds. Messages are now queued for writing while _messagesLock is held, which is what makes the queue's order the same as the in-memory list's, and the file is appended to after the lock is released, under a separate _fileLock. Because the queue is FIFO and is drained only under _fileLock, the file still ends up in exactly the order of _messages. A burst of messages now costs one append rather than one per message. That alone would still stall whichever thread is doing the writing, and the UI thread is routinely that thread: every remote change is handled from Application.Idle. So an ordinary append now gives up after a couple of quick tries rather than retrying for seconds, and whatever it could not write waits in _carryOver, to be written ahead of everything else by the next attempt -- so nothing is lost and the order still holds. Only IOException is retried; a permissions failure (the read-only collection folder of BL-16772) will not get better while we wait. _carryOver is capped so that a log file that stays unwritable cannot grow it for the rest of the session. At shutdown there is no next message to carry those lines out, so TeamCollection.Dispose calls the new Flush(), which does use RobustFile: by then there is no UI left to keep responsive. Tests cover the ordering guarantee under eight concurrent writers (with non-ASCII titles, so that the two append paths disagreeing about encoding would show up as a failure), the carry-over, Flush, and the flush on Dispose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also move the RunOnUiThreadLater method from TeamCollectionManager to Program. Handling deleted TC books was co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oxStops' into BL-16729-TeamCollectionWhenDropboxStops
**A vanished Books folder could take Bloom down.** RetryDeferredWatching tests that the folder exists on the heartbeat's thread, then posts the work to the UI thread. Dropbox can take the folder away again in between, and giving a FileSystemWatcher a Path that is not there throws -- out of a posted callback, where nothing catches it. Re-test inside the callback and stay deferred if it has gone, and guard the much narrower remaining window so that even then we only lose the retry, not the process. **Disconnect messages could be lost at shutdown.** When we disconnect part way through a session, CurrentCollection becomes null and the DisconnectedTeamCollection standing in for it gets a message log of its own, which is where the "you are now disconnected" messages go. Nothing disposed that object, so its log was never flushed. That did not matter while every append was synchronous, but now that an ordinary append gives up quickly on a busy file and leaves the message for later, the later never came. Dispose it too -- only when it is not simply the same object as CurrentCollection. 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 4 comments.
Reviewable status: 4 of 15 files reviewed, 8 unresolved discussions (waiting on JohnThomson).
src/BloomExe/TeamCollection/ConnectionHeartbeat.cs line 66 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Let's give this a name that reflects what it actually does...something like UpdateTcFileWatching or UpdateTcConnectionStatus.
[Claude Opus 5 (1M context)] Renamed to UpdateTcConnectionStatus.
That is what it actually does: it runs the connection probe and updates our connected/disconnected state. The references to it in nearby comments (including TestFolderTeamCollection) were updated to match.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1159 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This probably mirrors the normal watching code, but is it consistent with that for us to ignore deleted books? There's also the case of a rename, which will typically look like a new book (plus a deletion), though in pathological cases involving multiple renames some might come through as modifications.
[Claude Opus 5 (1M context)] You were right that deletions were being ignored, and they now are not. Renames turn out to have been covered already.
Deletions. The catch-up loop only walked the books that are in the repo, so a book deleted while we had no watcher was invisible: no Deleted event had ever been raised for it, and nothing told us the local copy was stale. There is now a second pass over the local collection that raises the same DeleteRepoBookFile event the watcher would have, for each local book with no counterpart in the repo. It skips folders that are not Bloom books, books with no local TeamCollection.status file (created here and never checked in, so they never had a repo counterpart to lose), and books renamed here but not yet checked in, which are still in the repo under their old name.
Deliberately, that pass makes no judgement about whether the book really was deleted -- a Books folder that Dropbox is still restoring is missing books that are perfectly alive. That judgement already lives in HandleDeletedRepoFile, which waits, re-checks the repo, and only recycles the local copy when it finds a tombstone showing someone deliberately deleted the book.
Renames. These already worked, by the route you describe. A remote rename arrives as a .bloom under the new name, which the existing loop sees has no local folder, so it announces it as a new book -- exactly as OnCreated would -- and HandleNewBook then matches it by book id in HandlePossibleRename and logs "renamed by a teammate". The delete half is raised too now, and is harmless: a rename leaves no tombstone, so HandleDeletedRepoFile declines it.
New tests cover the tombstoned and un-tombstoned deletion cases and the books that must not be announced (never-checked-in, and renamed-here).
src/BloomExe/TeamCollection/TeamCollectionManager.cs line 557 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This feels like a generic function that doesn't particularly belong in TCM. Is there a better place for it?
[Claude Opus 5 (1M context)] Agreed -- moved to Program, as Program.RunOnUiThreadLater.
That is where MainContext, the synchronization context it posts to, already lives, so it now sits next to the thing it depends on and no longer implies that marshalling to the UI thread is a Team Collection concern.
src/BloomExe/TeamCollection/TeamCollectionMessageLog.cs line 271 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This is normally a short operation, but if there are problems RobustFile will retry over a period of seconds. Is it a problem to hold the lock that long? WriteError might also involve RobustFile-type possible delays.
One possible strategy, if it's a problem, would be to make a member variable with a list of messages to write. Inside the lock, we push new messages. Persist can then lock only long enough to copy and empty the list; outside the lock, it can take its time writing them.
[Claude Opus 5 (1M context)] It was a problem, and I have implemented essentially the strategy you describe.
Messages are now queued while the messages lock is held -- which is what makes the queue's order the same as the in-memory list's -- and the file is appended to after that lock is released, under a separate file lock. Because the queue is FIFO and is only ever drained under that second lock, the file still ends up in exactly the order of the in-memory log, and a burst of messages costs one append instead of one per message. Readers (Messages, CurrentErrors, TeamCollectionStatus, the last of which is read on the UI thread whenever the Team Collection button refreshes) no longer wait on the file at all.
Two things beyond your sketch, both prompted by the same worry you raised:
The writing thread is usually the UI thread, since every remote change is handled from Application.Idle -- so simply moving the write off the lock would still have stalled it for seconds. An ordinary append now gives up after a couple of quick tries (RetryUtility, retrying only IOException -- a permissions failure, as in BL-16772's read-only collection folder, will not improve while we wait), and anything it could not write waits in a carry-over list to be written ahead of everything else by the next attempt. So nothing is lost and the order still holds; the carry-over is capped so an unwritable file cannot grow it all session.
At shutdown there is no next message to carry those lines out, so TeamCollection.Dispose calls a new Flush() that does use RobustFile and its long retry -- by then there is no UI left to keep responsive.
You were right about WriteError too: that is Logger.WriteEvent/WriteError in AfterMessageAdded, which was already outside the lock and stays there.
Worth flagging: Devin caught a real regression from this change -- the stand-in collection we create when disconnecting has a message log of its own, and nothing disposed it, so its "you are now disconnected" messages would never have been flushed. Fixed in f38cfff5c, with a test.
Covered by tests for the ordering guarantee under eight concurrent writers (with non-ASCII titles, so the two append paths disagreeing about encoding would show up), the carry-over, Flush, and the flush on dispose.
|
[Claude Opus 5 (1M context) from Steve McConnel's machine during preflight] Consulted Devin on 2026-09-17, twice: up to The first of today's rounds raised four findings that were not already on the PR. Three are now closed, each on its own thread above:
One is left open for the developer, because it is a question about when Bloom should decide a collection is disconnected rather than a clear-cut defect:
The second round, against Also in this round: the nine Investigate flags are unchanged from earlier rounds and were already mirrored and resolved; no informational items. CI ( |
StephenMcConnel
left a comment
StephenMcConnel
left a comment
There was a problem hiding this comment.
@StephenMcConnel reviewed 14 files and all commit messages, and made 4 comments.
Reviewable status: 4 of 15 files reviewed, 8 unresolved discussions (waiting on JohnThomson and StephenMcConnel+CLAU).
src/BloomExe/TeamCollection/ConnectionHeartbeat.cs line 66 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Let's give this a name that reflects what it actually does...something like UpdateTcFileWatching or UpdateTcConnectionStatus.
Done. I chose the second proposed name.
src/BloomExe/TeamCollection/FolderTeamCollection.cs line 1159 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This probably mirrors the normal watching code, but is it consistent with that for us to ignore deleted books? There's also the case of a rename, which will typically look like a new book (plus a deletion), though in pathological cases involving multiple renames some might come through as modifications.
I've had the AI attempt to address this comment.
src/BloomExe/TeamCollection/TeamCollectionManager.cs line 557 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This feels like a generic function that doesn't particularly belong in TCM. Is there a better place for it?
I moved it to program, where it fits as a static function. I didn't move it to anything in Utils because that namespace is used by Program, and would introduce a circular dependency of sorts.
src/BloomExe/TeamCollection/TeamCollectionMessageLog.cs line 271 at r8 (raw file):
Previously, JohnThomson (John Thomson) wrote…
This is normally a short operation, but if there are problems RobustFile will retry over a period of seconds. Is it a problem to hold the lock that long? WriteError might also involve RobustFile-type possible delays.
One possible strategy, if it's a problem, would be to make a member variable with a list of messages to write. Inside the lock, we push new messages. Persist can then lock only long enough to copy and empty the list; outside the lock, it can take its time writing them.
Done. The AI thought that reducing the time delay and repeat count from that used in RobustFile.AppendAllText was better than spinning off the write to a separate thread. I wasn't totally convinced, but went along with its suggestion.
StephenMcConnel
left a comment
StephenMcConnel
left a comment
There was a problem hiding this comment.
The target has changed to 6.6.
@StephenMcConnel made 1 comment.
Reviewable status: 4 of 15 files reviewed, 8 unresolved discussions (waiting on JohnThomson and StephenMcConnel+CLAU).
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