Re-watch channels when /sync refuses the reconnect catch-up - #227
Open
harlan wants to merge 2 commits into
Open
Conversation
FetchAndProcessEventsSinceLastReceivedEvent has a guard meant to skip /sync
when the last received event is older than 30 days, past which the server
rejects LastSyncAt. It computed the age backwards:
var diff = lastEventReceivedAt - _timeService.Now; // past - now => NEGATIVE
if (diff.TotalDays > 30) { return; } // therefore never true
lastEventReceivedAt is always in the past, so the branch is unreachable and
/sync is issued regardless of age. The request then fails server-side, and
because the only caller (StreamChatClient.RestoreStateLostDuringDisconnect)
invokes it fire-and-forget through LogIfFailed, the exception reaches the
logger and nothing else — the client silently gets no replay at all.
Corrected to `_timeService.Now - lastEventReceivedAt`. DateTimeOffset
subtraction compares UtcDateTime, so a local-offset vs server-offset mismatch
is handled correctly.
Also removes the dead `currentServerTime` local directly above it. It was
never read, and was almost certainly the intended left operand — the tell
that this comparison was written wrong and never exercised.
Note this only skips a request that was certain to fail; it is not a recovery
path. The SDK has no re-hydrate fallback of its own, so bridging a gap this
large remains the consumer's job.
On reconnect the SDK catches up by calling /sync with the timestamp of the last event received before the disconnect. The server refuses the request when the gap is too large — code 4 / HTTP 400, "Too many events to sync, please use a more recent last_sync_at parameter" — which the ~1000-event limit reaches long before the 30-day bound the guard above it checks. That cap counts events, not messages, across every cid passed in the one call, so a single message can contribute a message.new plus a message.read per member. A player who leaves the app backgrounded on a busy channel and returns hours later hits it every time. The failure had no handler. FetchAndProcessEventsSinceLastReceivedEvent is called fire-and-forget through LogIfFailed, so the exception reached the logger and nothing else: the watched channels kept the state they had before the disconnect, missing every message since, until something unrelated happened to re-fetch them. And _disconnectionLastEventReceivedAt stayed stale, so the next reconnect failed exactly the same way. In one of our production titles this is 6k+ such warnings across 4.2k users in 30 days; for a live room or an open feed it is a silent correctness gap, not just noise. Now the low-level client drops the stale sync point and rethrows, and RestoreStateLostDuringDisconnect catches the input error and re-watches every watched channel — the same full state fetch the initial watch does, which is the only way to recover once the events are past replay. Each channel is attempted independently: this runs after the stale sync point has been dropped, so it is the only recovery this reconnect gets, and a single failure escaping the loop would leave every remaining channel silently stale for the rest of the session. Failures are expected here, not exotic — a channel torn down while offline returns 403 on every read, and a long watched list can trip a 429 part-way. Known limitation, flagged in a comment: GetOrCreateChannelWithIdAsync is get-OR-create, so re-watching a channel that was hard-deleted while offline recreates it server-side as an empty channel. Fixing that properly means consulting SyncResponse.InaccessibleCids — already returned by /sync and currently ignored — to skip channels the server says are gone, rather than discovering it one 403 at a time. Happy to take that on in this PR if you would rather not merge the get-or-create behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On reconnect the SDK catches up by calling
/syncwith the timestamp of the last event received before the disconnect. The server refuses the request when the gap is too large — code 4 / HTTP 400, "Too many events to sync, please use a more recent last_sync_at parameter".That limit (~1000 events, the one the existing
StreamTodoin this method mentions) is reached long before the 30-day bound the guard above it checks. It counts events, not messages, across every cid passed in the single call, so one message can contribute amessage.newplus amessage.readper member. A player who leaves the app backgrounded on a busy channel and returns hours later hits it every time.The failure had no handler.
FetchAndProcessEventsSinceLastReceivedEventis called fire-and-forget throughLogIfFailed, so the exception reached the logger and nothing else:_disconnectionLastEventReceivedAtstayed stale, so the next reconnect failed identically. It never self-heals.In one of our production titles this is 6k+ such warnings across 4.2k users in 30 days. For a live room or an open feed it is a silent correctness gap, not just log noise.
Change
The low-level client drops the stale sync point and rethrows;
RestoreStateLostDuringDisconnectcatches the input error and re-watches every watched channel — the same full state fetch the initial watch performs, and the only way to recover once the events are past replay.Each channel is attempted independently. This runs after the stale sync point has been dropped, so it is the only recovery this reconnect gets and there is no later retry — a single failure escaping the loop would leave every remaining channel silently stale for the rest of the session. Failures are expected here, not exotic: a channel torn down while offline returns 403 on every read, and a long watched list can trip a 429 part-way. Each is logged and the loop continues.
Known limitation (please weigh in)
GetOrCreateChannelWithIdAsyncis get-or-create, so re-watching a channel that was hard-deleted while we were offline recreates it server-side as an empty channel. This is flagged in a comment rather than fixed.The proper fix is to consult
SyncResponse.InaccessibleCids— already returned by/syncand currently ignored by the SDK — to skip channels the server says are gone, instead of discovering it one 403 at a time. That's a slightly larger change touching how/syncresponses are consumed, so I left it out of the first pass. Say the word and I'll do it in this PR if you'd rather not merge the get-or-create behavior.Testing
No test added: both edited paths are internals on the reconnect path, and the existing test setup has no seam that reaches them. Glad to add coverage with a pointer to the approach you'd prefer.