Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,22 @@ import java.util.concurrent.atomic.AtomicReference
private const val QUERIES_TO_RETRY = 3
private const val SYNC_MAX_CIDS = 100

/**
* Maximum number of events replayed from a single `/sync` response. Above this count, event replay
* is skipped and the actively watched channels are refreshed with `queryChannels` instead.
*
* Deliberately conservative and well below the backend cap: a payload can be accepted by the
* backend and still be too expensive to replay on the device.
*/
private const val SYNC_EVENT_REPLAY_MAX_COUNT = 250

/**
* Page size of the `queryChannels` fallback. The API caps `queryChannels` at 30 channels per
* request, so watched cids are refreshed in chunks of this size rather than in [SYNC_MAX_CIDS]
* batches like `/sync` is.
*/
private const val QUERY_CHANNELS_MAX_LIMIT = 30

/**
* This class is responsible to sync messages, reactions and channel data. It tries to sync then, if necessary,
* when connection is reestablished or when a health check event happens.
Expand All @@ -99,6 +115,7 @@ internal class SyncManager(
private val userPresence: Boolean,
private val isAutomaticSyncOnReconnectEnabled: Boolean,
private val syncMaxThreshold: TimeDuration,
private val eventReplayMaxCount: Int = SYNC_EVENT_REPLAY_MAX_COUNT,

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.

Small thing: only tests pass this. Could it get @VisibleForTesting like performSync?

private val now: () -> Long,
private val serverClockOffset: ServerClockOffset,
scope: CoroutineScope,
Expand Down Expand Up @@ -254,8 +271,8 @@ internal class SyncManager(
}
if (isAutomaticSyncOnReconnectEnabled) {
logger.i { "[onConnectionEstablished] performing sync and restoring active channels" }
performSync()
restoreActiveChannels()
val refreshedCids = performSync()
restoreActiveChannels(alreadyRefreshedCids = refreshedCids)
} else {
logger.i { "[onConnectionEstablished] skipping sync, isAutomaticSyncOnReconnectEnabled=false" }
}
Expand Down Expand Up @@ -285,23 +302,31 @@ internal class SyncManager(
logger.i { "[connectionLost] failed: $e" }
}

private suspend fun performSync() {
/**
* @return The channel ids refreshed by the `queryChannels` fallback, empty when events were
* replayed normally.
*/
private suspend fun performSync(): Set<String> {
logger.i { "[performSync] no args" }
val cids = logicRegistry.getActiveChannelsLogic().map { it.cid }.ifEmpty {
logger.w { "[performSync] no active cids found" }
repos.selectSyncState(currentUserId)?.activeChannelIds ?: emptyList()
}
mutex.withLock {
return mutex.withLock {
performSync(cids)
}
}

/**
* @return The channel ids refreshed by the `queryChannels` fallback, empty when events were
* replayed normally.
*/
@VisibleForTesting
internal suspend fun performSync(cids: List<String>) {
internal suspend fun performSync(cids: List<String>): Set<String> {
logger.d { "[performSync] cids.size: ${cids.size} " }
if (cids.isEmpty()) {
logger.w { "[performSync] rejected (cids is empty)" }
return
return emptySet()
}
val syncState = syncState.value ?: repos.selectSyncState(currentUserId)
val lastSyncAt = syncState?.lastSyncedAt ?: Date(now())
Expand All @@ -314,17 +339,29 @@ internal class SyncManager(
chatClient.getSyncHistory(cappedCids, lastSyncAt).await()
}
if (result.isTooManyEventsToSyncError()) {
// Event replay is impossible here, so refresh the watched surfaces instead. The payload
// is unavailable, hence the timestamp moves to now - but only once the refresh
// succeeded, otherwise the skipped events would become unrecoverable.
logger.e { "[performSync] failed (too many events to sync): $result" }
val refresh = refreshActiveWatchedChannels()
if (refresh !is Result.Success) {

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.

This can leave a permanent 400 stuck. Before, the branch always advanced lastSyncedAt, so the next /sync asked for a fresh range. Now if the refresh fails nothing moves, and since isTooManyEventsToSyncError() is just 400 plus validation error, the same request keeps failing on every reconnect. I ran three reconnects locally with a failing queryChannelsInternal and lastSyncedAt never moved.

The hold-on-failure rule makes sense in skipEventReplay, where you can ask for the payload again. Here the server already refused it, so could this branch keep the unconditional updateLastSyncedDate(now())? Happy to be told otherwise.

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.

Answering the "happy to be told otherwise": I don't think unconditional advance is right either, though the stuck-400 concern is real.

iOS holds the checkpoint on fallback failure in this same 400 path. syncMissingEvents routes the 400 into handleSyncEventReplayFallback (SyncRepository.swift:417), and on .failure that completes without ever calling updateLastSyncAt (:459-461). So the hold isn't an Android deviation.

What keeps it from wedging over there is two things we don't have:

  • maximumDaysSinceLastSync = 30: when lastSyncAt is older than 30 days it skips /sync altogether and advances the checkpoint unconditionally (SyncRepository.swift:291-304). That's the backstop.
  • SyncError.failedFetchingChannels.shouldRetry == true, so the operation retries twice within the same reconnect before giving up (:18-19, SyncOperations.swift:20).

On Android there's no equivalent. Every lastSyncedAt write is on the /sync path, and syncMaxThreshold only gates the channel/message/reaction retries (lines 768-930), never the checkpoint. So I'd rather add the max-age backstop than make this branch advance unconditionally — otherwise we diverge from the implementation we're porting and still have no floor under a stuck checkpoint.

One thing that makes your concern worse than it looks: isTooManyEventsToSyncError() is just 400 + serverErrorCode == VALIDATION_ERROR (NetworkError.kt:36). Any /sync validation error — bad cid, malformed date — now holds the checkpoint, not only genuinely oversized ranges.

logger.e { "[performSync] fallback refresh failed, keeping lastSyncedAt: ${refresh.errorOrNull()}" }
return emptySet()
}
updateLastSyncedDate(latestEventDate = Date(now()), rawLatestEventDate = null)
return
return refresh.value
}
if (result !is Result.Success) {
logger.e { "[performSync] failed($result)" }
return
return emptySet()
}
val sortedEvents = result.value.sortedBy { it.createdAt }
logger.v { "[performSync] succeed; events.size: ${sortedEvents.size}" }

if (sortedEvents.size > eventReplayMaxCount) {
return skipEventReplay(sortedEvents)
}

val latestEvent = sortedEvents.lastOrNull()
val latestEventDate = latestEvent?.createdAt ?: Date(now())
val rawLatestEventDate = latestEvent?.rawCreatedAt
Expand All @@ -337,14 +374,43 @@ internal class SyncManager(
}
if (sortedEvents.isEmpty()) {
logger.w { "[performSync] rejected (no events to emit)" }
return
return emptySet()
}
if (rawLastSyncAt == rawLatestEventDate) {
logger.w { "[performSync] rejected (rawLatestEventDate equals to rawLastSyncAt)" }
return
return emptySet()
}
events.emit(sortedEvents)
logger.v { "[performSync] events emission completed" }
return emptySet()
}

/**
* Skips the replay of an oversized `/sync` payload.
*
* Replaying this many events would hold the state and persistence pipeline long enough to slow
* down the requests that also need it, so the actively watched channels are refreshed with
* `queryChannels` instead.
*
* The last-synced timestamp only moves to the newest skipped event once that refresh succeeded:
* it is what makes discarding the payload safe. When the refresh fails the checkpoint is kept,
* so the next reconnect asks for the same range again and can retry - at the cost of re-fetching
* a payload we discard, which is a request and a parse rather than a stalled pipeline.
*
* @return The channel ids that were refreshed, empty when the refresh failed.
*/
private suspend fun skipEventReplay(sortedEvents: List<ChatEvent>): Set<String> {

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.

Not blocking, and I suspect this is an iOS-side design question rather than something to fix here.

The fallback is inert in exactly the case that triggers it. The "no active cids found" branch above is only reached when getActiveChannelsLogic() is empty — so when the fallback re-reads that same source it contributes nothing by construction, and watchedCids reduces to getTrackedWatchedChannels(), itself empty before any screen has subscribed. Cold start after a long offline stretch therefore gives Success(emptySet()): the checkpoint jumps to the newest skipped event and nothing is refreshed.

The impact is mild — those channels have no state objects and aren't on screen, so the cost is a stale Room cache until the next query refreshes them. But it does mean the KDoc's "the actively watched channels are refreshed with queryChannels instead" doesn't hold for the main trigger of an oversized payload, which is worth saying out loud somewhere.

iOS is inert here too: refreshActiveWatchedChannels collects from the same kind of weak live-controller set (SyncRepository.swift:320-323), so this is a shared gap rather than an Android deviation. Probably one to raise with them rather than diverge on quietly.

logger.i {
"[performSync] skipping event replay; events.size: ${sortedEvents.size} exceeds $eventReplayMaxCount"
}
val refresh = refreshActiveWatchedChannels()
if (refresh !is Result.Success) {
logger.e { "[skipEventReplay] fallback refresh failed, keeping lastSyncedAt: ${refresh.errorOrNull()}" }
return emptySet()
}
val skippedLatestEvent = sortedEvents.last()

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.

Not blocking: the replay path calls updateAllReadStateForDate for each MarkAllReadEvent, this one drops them. Looks low impact since markedAllReadAt is only stored and compared, but a quick scan of sortedEvents before discarding would keep the two paths the same. Worth it?

updateLastSyncedDate(skippedLatestEvent.createdAt, skippedLatestEvent.rawCreatedAt)
return refresh.value
}

/**
Expand Down Expand Up @@ -407,9 +473,16 @@ internal class SyncManager(
logger.e(e) { "[retryFailedEntities] failed: $e" }
}

private suspend fun restoreActiveChannels() {
/**
* @param alreadyRefreshedCids Channel ids already brought up to date earlier in the recovery
* flow, e.g. by the `/sync` event replay fallback. They are not queried a second time.
*/
private suspend fun restoreActiveChannels(alreadyRefreshedCids: Set<String> = emptySet()) {
val recoverAll = !isFirstConnect.compareAndSet(true, false)
logger.d { "[restoreActiveChannels] recoverAll: $recoverAll" }
logger.d {
"[restoreActiveChannels] recoverAll: $recoverAll, " +
"alreadyRefreshedCids.size: ${alreadyRefreshedCids.size}"
}

val allLogics = logicRegistry.getActiveQueryChannelsLogic()
val hasGroupedQueries = allLogics.any { it.groupKey() != null }
Expand All @@ -433,7 +506,7 @@ internal class SyncManager(
is Result.Success -> {
val updatedCids = result.value
logger.v { "[restoreActiveChannels] standardCids.size: ${result.value.size}" }
updateActiveChannels(recoverAll, updatedCids + groupedHandledCids)
updateActiveChannels(recoverAll, updatedCids + groupedHandledCids + alreadyRefreshedCids)

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.

alreadyRefreshedCids only gets to updateActiveChannels. rewatchTrackedWatchedChannels() on line 497 still goes over the whole tracked set and does one queryChannel per cid, so on the grouped path the channels the fallback just refreshed get fetched again one by one. The cidsToExclude param on refreshActiveWatchedChannels is also unused right now.

Could alreadyRefreshedCids be passed there too?

}
is Result.Failure -> {
logger.e { "[restoreActiveChannels] standard query failed: ${result.value}" }
Expand All @@ -445,7 +518,7 @@ internal class SyncManager(
// --- Active Channels created outside of QueryChannels requests
if (!hasStandardQueries && !hasGroupedQueries) {
// Check for active channels created outside of a QueryChannels requests
updateActiveChannels(recoverAll, cidsToExclude = emptySet())
updateActiveChannels(recoverAll, cidsToExclude = alreadyRefreshedCids)
}
}

Expand Down Expand Up @@ -609,6 +682,83 @@ internal class SyncManager(
}
}

/**
* Refreshes the actively watched channels through `queryChannels` instead of replaying an
* oversized `/sync` payload event by event.
*
* A `queryChannels` response carries the latest channel state, messages, members, watchers and
* read state, and re-registers the watch for the requested cids, which is what the currently
* visible surfaces need. Channels that are not actively watched are left to a future explicit
* query.
*
* Having nothing to refresh is a success: with no watched channels there is nothing visible that
* can go stale. A failed request is not, and the caller must not advance the last-synced date on
* it, or the events it skipped become unrecoverable.
*
* @param cidsToExclude Channel ids already refreshed elsewhere in the recovery flow.
*
* @return The channel ids that were refreshed, or a failure when any request failed.
*/
private suspend fun refreshActiveWatchedChannels(
cidsToExclude: Set<String> = emptySet(),
): Result<Set<String>> {
val online = clientState.isOnline
val watchedCids = buildSet {
addAll(stateRegistry.getTrackedWatchedChannels())
logicRegistry.getActiveChannelsLogic().mapTo(this) { it.cid }

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.

getActiveChannelsLogic() is channels.values (LogicRegistry.kt:300), and that cache gets an entry for every channel in every queryChannels result (QueryChannelsStateLogic.kt:184). So this is every channel the list has paged through, not the ones on screen, and there is no cap. With 300 cids I get 10 sequential queryChannelsInternal calls, each with watch=true and a full state write.

That can cost more than the 251 events it avoids, and it re-watches channels the user is not viewing. Any reason not to cap it at 30 like updateActiveChannels does, or limit it to getTrackedWatchedChannels() plus channels with recoveryNeeded?

} - cidsToExclude
logger.d {
"[refreshActiveWatchedChannels] watchedCids.size: ${watchedCids.size}, online: $online, " +
"cidsToExclude.size: ${cidsToExclude.size}"
}
if (watchedCids.isEmpty()) {
return Result.Success(emptySet())
}
if (!online) {
logger.w { "[refreshActiveWatchedChannels] rejected (offline)" }
return Result.Failure(Error.GenericError("Cannot refresh watched channels while offline"))
}

val failed = AtomicReference<Error>()
val refreshedCids = mutableSetOf<String>()
watchedCids.chunked(QUERY_CHANNELS_MAX_LIMIT).forEach { batch ->

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.

A failed batch neither breaks the loop nor rolls back. The remaining ceil(n/30) requests still fire after a known failure, failed.set() keeps only the last error, and — because the function then returns Result.Failure — the batches that did succeed are dropped from the return value, so restoreActiveChannels re-queries them.

iOS breaks on the first error instead of recursing into the remaining batches: startWatchingChannelsInBatches returns .failure(.failedFetchingChannels) immediately (SyncRepository.swift:478-482). Doing the same here avoids both the wasted requests and the redundant re-query — in the path this PR exists to make cheaper.

val request = QueryChannelsRequest(
filter = Filters.`in`("cid", batch),
offset = 0,
limit = batch.size,
).apply { presence = userPresence }
logger.v { "[refreshActiveWatchedChannels] request: $request" }
chatClient.queryChannelsInternal(request)
.await()
.onError {
logger.e { "[refreshActiveWatchedChannels] request failed: $it" }
failed.set(it)
}
.onSuccessSuspend { queryResult ->
val foundChannels = queryResult.channels
foundChannels.forEach { channel ->
ChannelId.fromTypeAndId(channel.type, channel.id)
?.let(logicRegistry::channel)
?.updateDataForChannel(channel, channel.messages.size)

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.

Small thing: shouldRefreshMessages stays false here, so this takes the upsertMessages branch (ChannelLogicImpl.kt:346), which adds and updates but never removes. A message hard-deleted during the skipped range stays in local state, and the checkpoint has already moved past the message.deleted event. rewatchTrackedWatchedChannels sets shouldRefresh = true for this reason.

A full replace resets scroll position though, so not obvious either way. Which do you prefer?

}
repos.storeStateForChannels(foundChannels)
foundChannels.mapTo(refreshedCids, Channel::cid)
}
}
// A cid the server did not return is not a failure: it can be deleted or no longer visible
// to this user, and holding the checkpoint for it would re-fetch the payload on every
// reconnect forever. Only a failed request blocks the checkpoint.
val missedCids = watchedCids - refreshedCids
if (missedCids.isNotEmpty()) {
logger.w { "[refreshActiveWatchedChannels] not returned by the server; cids.size: ${missedCids.size}" }
}
logger.v { "[refreshActiveWatchedChannels] refreshedCids.size: ${refreshedCids.size}" }
return when (val error = failed.get()) {
null -> Result.Success(refreshedCids)
else -> Result.Failure(error)
}
}

private suspend fun retryChannels() {
val cids = repos.selectChannelCidsBySyncNeeded()
logger.d { "[retryChannels] cids.size: ${cids.size}" }
Expand Down
Loading
Loading