feat: connection recovery - #1841
Conversation
| */ | ||
| private get recoverableActiveChannels(): Channel[] { | ||
| const channels: Channel[] = []; | ||
| for (const cid in this.client.activeChannels) { |
There was a problem hiding this comment.
Wouldn't it be more performant to keep a registry (Array or Set) of active channels instead of keeping the prop on instance and having to iterate over the whole cache of activeChannels? I think there will be 1 or 2 active channels at any time, so going through 100 cached channels may be too much unnecessary work.
Another thing is that we should be probably deprecating client.activeChannels and look up inside the corresponding EntityStore instead.
There was a problem hiding this comment.
I would not agree with this statement. It was evaluated and discarded, because it's simply due to the reason that this is never going to be a set of god knows how many instances (it naturally cannot).
may be too much unnecessary work
Let's please evaluate what the unnecessary work might mean. 100s of channels (and even 1000s) of channels are a piece of cake for V8/Hermes to run through shallowly and check for active ones. So we're talking nanoseconds of extra compute time that happens only on reconnects.
Adding a set for this introduces bookkeeping that is extremely unnecessary for something like this. Additionally that would then later completely prevent us acting on state.active if we need it elsewhere, as it belongs to a channel as part of the state.
At a certain point we have to stop and think what we're optimizing against and what gains we'd get as compared to readability.
Another thing is that we should be probably deprecating client.activeChannels and look up inside the corresponding EntityStore instead.
This is something that can be done whenever activeChannels is gone, until then we have to stick to it. Doesn't make sense to pollute the PR with more changes doing that.
There was a problem hiding this comment.
And to top it all off, all of this happens only on reconnect.
There was a problem hiding this comment.
The only thing I am saying that all the operations add up. Iterations over what we have, then ingestions which neither are light operations etc. We can take a look at the optimization later, in few weeks time.
There was a problem hiding this comment.
Yeah, they could add up - however we don't do any ingestion here at all, we just extract the active: true channels from whatever caching mechanism is present pretty much. I am all for performance naturally, but probably where it counts and on the actually hot paths, which this is probably not.
Iterations over what we have
This is extremely cheap, not sure why we think these aren't light operations. The ingestions are going to happen regardless of how we consume the currently active channels (for which, keep in mind there's going to be 1 active channel most of the time and tens of loaded channels at all).
If we can see some potential in which this might balloon to a very high number I'd be more than happy to add alternative ways to bookmark what active means, but otherwise I don't think we should be optimizing stuff that do not hurt us and complicate the code more. If you have some specific way in mind where this could explode please let me know, I might just be missing some edge case
| if (!thread) continue; | ||
| // Read off state rather than a getter: `Thread` has no `active` accessor the way `Channel` | ||
| // does, and adding one just for this would grow the public surface for a single internal read. | ||
| const { active } = thread.state.getLatestValue(); |
There was a problem hiding this comment.
I would keep the active threads again in a set or array as I proposed with the channel due to the same reasons described with channels.
There was a problem hiding this comment.
If the active info is needed only for purposes of state recovery I would even keep the active channels and threads registry (sets) inside the ConnectionRecoveryManager
There was a problem hiding this comment.
I think the unread state does somewhere rely on whether the channel is active, but we could be looking into a set of active channel CIDs to verify it as well.
CLA
Description of the changes, What, Why and How?
The client handles connection recovery now, so the UI SDKs don't have to.
recoverState()was unusable, because of the fact that it never guaranteed proper rewatching of channels (nor reordering of the channel list properly and so we had an implementation that ended up spread across three places that didn't know about each other.What was wrong with the old one:
queryChannelsAndHydrate({ cid: { $in: activeChannels }, limit: 30 }), so it used a query shape no list actually had, and quietly stopped at 30 channelswatchStatussync()_reconnect(), so it never ran when the app backgrounded (closeConnection()→openConnection()), which is the common case on mobileChannelWatchStatus.WasWatchingandchannel.activewere already there and read by nothing. They wereadded for this.
What recovery does
ConnectionRecoveryManagerdoes three things:ChannelManager.recover()). SincequeryChannelswatches by default, that re-watches the channels on that page too. Non-destructive, the list never blanks.channel.reload()). A list page carries far fewer messages per channel than an open channel's window, so the list query can't cover it.thread.reload()).It doesn't loop over
activeChannels. That cache gets large after any scrolling and watches are a limited server-side resource. Channels outside the refreshed pages come back when an event proves them relevant:ChannelManagerre-watches a routed channel markedWasWatching.Triggers differ per surface, so lists use
connection.changed { online: true }, becauseChannelPaginator.executeQueryalready defers its own first page. Channels and threads use the offline DB's sync-status edge case when offline support is on andconnection.changedotherwise, becausereload()has no deferral of its own and has to run after replay andsync().OfflineDBSyncManagerpublishes that edge unconditionally aftersyncAndExecutePendingTasks(), which is what makes the ordering hold on every path.connection.recoverednow fires from the manager once the reloads settle, on every reconnect path rather than only from_reconnect().Bug fixes
Three, all pre-existing, found while testing the above. Each has regression tests.
Reload asked for as many items as were already loaded. A thread with one reply asked the server for one reply, so anything that arrived while offline couldn't be discovered, and the single result was disjoint from the loaded window so the merge rebuilt and dropped what was there. Asks for at least a page now.
mergeNewestPagereturned early when there was no anchored head interval, throwing away the page it had just been handed. Happens when the first query comes back empty and the only content was ingested live, i.e. a thread or channel created in the same session. It anchors the page now, and doesn't move the view if the reader has jumped somewhere else.Opening a thread on a parent with no replies 404s, since there's no thread server-side yet. That was published as
lastReloadError, and RN ORs that into its channel error state, so a brand-new thread looked broken. Treated as the expected answer now:reload()resolves quietly. A thread that did have replies and comes back 404 still reports it.replyCounttells the two apart, and unlikedeletedAtit still works when you missed the delete event while offline.Breaking
client.recoverState()is gone rather than a no-op, so callers fail loudly. Use.client.connectionRecovery.recover(). The one thing it did that mattered, resettingwsPromise/setUserPromise, moved toclient._settleConnectPromises()and is called from_reconnect()in the same place.recoverStateOnReconnectkeeps its name, type andtruedefault. It gates the new flow instead of the old bulk query. If you set itfalseand recover yourself, nothing changes.Added
client.connectionRecovery·ChannelManager.recover()·channel.state.lastReloadError(plus thechannel.lastReloadErrorgetter) ·thread.state.lastReloadError·isDoesNotExistErrorinerrors.ts.Thread.activate()/deactivate()are refcounted now, likeChannel.activate().The
lastReloadErrorfields exist because the manager runs reloads insidePromise.allSettled, so a failure never reaches the UI as a throw. Cleared on entry, set in the catch, and still rethrown.Testing
Known gaps
Recovery finds threads via
client.threads.threadsByIdfiltered onstate.active. That's the thread list, not a cache - a thread opened from a message list only gets there because the UI SDKs adopt it once its replies load. A reconnect before that, or afterThreadManager.reload()evicts a thread the user doesn't participate in, misses it. There's a test namedKNOWN GAPpinning this. The fix isThreadManager's commented-outthreadCache(probably in the form of aStoreBackedItemIndexor something) that is anyway in our todo list.Underneath that: live-ingested content produces a window with no head interval at all. It renders fine and
hasMoreHeadsaystrue, which isn't true. Three places each work around it differently (mergeNewestPagegot it wrong,jumpToTheLatestMessagequeries to recover,reconcileHeadAgainstPagereturns early). Anchoring at ingest time instead was tried and reverted: it breaks optimistic message placement, since an unsent message would claim it came from a fetched page, and it stops scroll-to-bottom loading the real head. The fix within the page merging mechanism should still stay for a variety of different edge cases, but this one should also be addressed. Needs its own PR.isDoesNotExistErrorlooks the code up inAPIErrorCodesby name instead of comparing to16, so the table stays the only place that mapping lives.nameis typedstringthere, so a typo compiles and silently returns false. Making those names a union would be nice.Changelog
client.connectionRecovery. On reconnect it re-runs each loaded channel list's first-page query,reloads every active channel and thread, then dispatchesconnection.recovered. Runs after offline replay andsync()on every reconnect path, including backgrounding.ChannelManager.recover(), a non-destructive re-query of every initialized list.channel.state.lastReloadError(andchannel.lastReloadError) andthread.state.lastReloadErrorso a failed reconnect refresh stays visible to the UI.isDoesNotExistErrortoerrors.ts.ChannelManagerre-watches a routed channel whosewatchStatusisWasWatching, restoring a watchlost with the socket. Narrower than v9, which re-watched any routed channel. Skipped forchannel.hidden, pending disposal andNotWatching.connection.recoveredis dispatched byConnectionRecoveryManagerafter the reloads, so it fires onevery reconnect path instead of only from_reconnect().Thread.activate()/deactivate()are refcounted, matchingChannel.activate().mergeNewestPagediscarding the fetched page when no page had ever anchored the window.client.recoverState(). Useclient.connectionRecovery.recover().recoverStateOnReconnectnow gatesclient.connectionRecoveryinstead of the removed bulk query. Same name, type and default.