Skip to content

feat: connection recovery - #1841

Open
isekovanic wants to merge 11 commits into
release-v10from
feat/connection-recovery
Open

feat: connection recovery#1841
isekovanic wants to merge 11 commits into
release-v10from
feat/connection-recovery

Conversation

@isekovanic

Copy link
Copy Markdown
Contributor

CLA

  • I have signed the Stream CLA (required).
  • Code changes are tested

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:

  • one queryChannelsAndHydrate({ cid: { $in: activeChannels }, limit: 30 }), so it used a query shape no list actually had, and quietly stopped at 30 channels
  • never looked at watchStatus
  • wasn't ordered against offline pending-task replay or sync()
  • only called from _reconnect(), so it never ran when the app backgrounded (closeConnection()openConnection()), which is the common case on mobile

ChannelWatchStatus.WasWatching and channel.active were already there and read by nothing. They were
added for this.

What recovery does

ConnectionRecoveryManager does three things:

  1. every loaded channel list re-runs its own first-page query (ChannelManager.recover()). Since queryChannels watches by default, that re-watches the channels on that page too. Non-destructive, the list never blanks.
  2. every active channel reloads (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.
  3. every active thread reloads its replies (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: ChannelManager re-watches a routed channel marked WasWatching.

Triggers differ per surface, so lists use connection.changed { online: true }, because ChannelPaginator.executeQuery already defers its own first page. Channels and threads use the offline DB's sync-status edge case when offline support is on and connection.changed otherwise, because reload() has no deferral of its own and has to run after replay and sync(). OfflineDBSyncManager publishes that edge unconditionally after syncAndExecutePendingTasks(), which is what makes the ordering hold on every path.

connection.recovered now 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.

mergeNewestPage returned 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. replyCount tells the two apart, and unlike deletedAt it 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, resetting wsPromise/setUserPromise, moved to client._settleConnectPromises() and is called from _reconnect() in the same place.

recoverStateOnReconnect keeps its name, type and true default. It gates the new flow instead of the old bulk query. If you set it false and recover yourself, nothing changes.

Added

client.connectionRecovery · ChannelManager.recover() · channel.state.lastReloadError (plus the channel.lastReloadError getter) · thread.state.lastReloadError · isDoesNotExistError in errors.ts. Thread.activate()/deactivate() are refcounted now, like Channel.activate().

The lastReloadError fields exist because the manager runs reloads inside Promise.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.threadsById filtered on state.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 after ThreadManager.reload() evicts a thread the user doesn't participate in, misses it. There's a test named KNOWN GAP pinning this. The fix is ThreadManager's commented-out threadCache (probably in the form of a StoreBackedItemIndex or 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 hasMoreHead says true, which isn't true. Three places each work around it differently (mergeNewestPage got it wrong, jumpToTheLatestMessage queries to recover, reconcileHeadAgainstPage returns 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.

isDoesNotExistError looks the code up in APIErrorCodes by name instead of comparing to 16, so the table stays the only place that mapping lives. name is typed string there, so a typo compiles and silently returns false. Making those names a union would be nice.

Changelog

  • Added client.connectionRecovery. On reconnect it re-runs each loaded channel list's first-page query,reloads every active channel and thread, then dispatches connection.recovered. Runs after offline replay and sync() on every reconnect path, including backgrounding.
  • Added ChannelManager.recover(), a non-destructive re-query of every initialized list.
  • Added channel.state.lastReloadError (and channel.lastReloadError) and thread.state.lastReloadError so a failed reconnect refresh stays visible to the UI.
  • Added isDoesNotExistError to errors.ts.
  • ChannelManager re-watches a routed channel whose watchStatus is WasWatching, restoring a watchlost with the socket. Narrower than v9, which re-watched any routed channel. Skipped forchannel.hidden, pending disposal and NotWatching.
  • connection.recovered is dispatched by ConnectionRecoveryManager after the reloads, so it fires onevery reconnect path instead of only from _reconnect().
  • Thread.activate()/deactivate() are refcounted, matching Channel.activate().
  • Fixed the reconnect refresh requesting only as many items as were loaded, which hid messages that arrived while offline and could drop the loaded window.
  • Fixed mergeNewestPage discarding the fetched page when no page had ever anchored the window.
  • Fixed opening a thread on a reply-less parent raising an error state.
  • Removed client.recoverState(). Use client.connectionRecovery.recover().
  • recoverStateOnReconnect now gates client.connectionRecovery instead of the removed bulk query. Same name, type and default.

Comment thread src/pagination/paginators/MessageIntervalPaginator.ts
Comment thread src/pagination/paginators/MessageIntervalPaginator.ts Outdated
Comment thread src/channel_state.ts Outdated
Comment thread src/channel.ts
Comment thread src/ChannelManager.ts
Comment thread src/client.ts Outdated
Comment thread src/client.ts Outdated
Comment thread src/client.ts
*/
private get recoverableActiveChannels(): Channel[] {
const channels: Channel[] = [];
for (const cid in this.client.activeChannels) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to top it all off, all of this happens only on reconnect.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1841 (comment) same as here

Comment thread src/thread.ts Outdated
Comment thread src/channel.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants