From e0c829d8878831437cb64673cf15e9e7a194805a Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 19 Nov 2025 16:56:01 +0100 Subject: [PATCH 1/6] First attempt --- .../SyncNotificationWorkManagerRequest.kt | 39 +++++++++---------- .../impl/workmanager/WorkerDataConverter.kt | 23 ++++++++++- .../workmanager/api/WorkManagerRequest.kt | 2 +- .../impl/DefaultWorkManagerScheduler.kt | 18 +++++---- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt index 9492977c3d..cff12cc17d 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt @@ -29,30 +29,29 @@ class SyncNotificationWorkManagerRequest( private val workerDataConverter: WorkerDataConverter, private val buildVersionSdkIntProvider: BuildVersionSdkIntProvider, ) : WorkManagerRequest { - override fun build(): Result { + override fun build(): List> { if (notificationEventRequests.isEmpty()) { - return Result.failure(InvalidParameterException("notificationEventRequests cannot be empty")) - } - val data = workerDataConverter.serialize(notificationEventRequests).getOrElse { - return Result.failure(it) + return listOf(Result.failure(InvalidParameterException("notificationEventRequests cannot be empty"))) } Timber.d("Scheduling ${notificationEventRequests.size} notification requests with WorkManager for $sessionId") - return Result.success( - OneTimeWorkRequestBuilder() - .setInputData(data) - .apply { - // Expedited workers aren't needed on Android 12 or lower: - // They force displaying a foreground sync notification for no good reason, since they sync almost immediately anyway - // See https://developer.android.com/develop/background-work/background-tasks/persistent/getting-started/define-work#backwards-compat - if (buildVersionSdkIntProvider.isAtLeast(Build.VERSION_CODES.TIRAMISU)) { - setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) + return workerDataConverter.serialize(notificationEventRequests).map { + it.map { data -> + OneTimeWorkRequestBuilder() + .setInputData(data) + .apply { + // Expedited workers aren't needed on Android 12 or lower: + // They force displaying a foreground sync notification for no good reason, since they sync almost immediately anyway + // See https://developer.android.com/develop/background-work/background-tasks/persistent/getting-started/define-work#backwards-compat + if (buildVersionSdkIntProvider.isAtLeast(Build.VERSION_CODES.TIRAMISU)) { + setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) + } } - } - .setTraceTag(workManagerTag(sessionId, WorkManagerRequestType.NOTIFICATION_SYNC)) - // TODO investigate using this instead of the resolver queue - // .setInputMerger() - .build() - ) + .setTraceTag(workManagerTag(sessionId, WorkManagerRequestType.NOTIFICATION_SYNC)) + // TODO investigate using this instead of the resolver queue + // .setInputMerger() + .build() + } + } } @Serializable diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt index ac51c6c6a9..71d24cd62d 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt @@ -12,6 +12,7 @@ import androidx.work.Data import androidx.work.workDataOf import dev.zacsweers.metro.Inject import io.element.android.libraries.androidutils.json.JsonProvider +import io.element.android.libraries.core.extensions.mapCatchingExceptions import io.element.android.libraries.core.extensions.runCatchingExceptions import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId @@ -23,12 +24,29 @@ import timber.log.Timber class WorkerDataConverter( private val json: JsonProvider, ) { - fun serialize(notificationEventRequests: List): Result { + fun serialize(notificationEventRequests: List): List> { + return serializeRequests(notificationEventRequests) + .fold( + onSuccess = { + listOf(Result.success(it)) + }, + onFailure = { + // Perform serialization on sublists, workDataOf may have failed because of size limit + Timber.w(it, "Failed to serialize ${notificationEventRequests.size} notification requests, trying with chunks of $CHUNK_SIZE.") + notificationEventRequests.chunked(CHUNK_SIZE).map { chunk -> + serializeRequests(chunk) + } + }, + ) + } + + private fun serializeRequests(notificationEventRequests: List): Result { return runCatchingExceptions { json().encodeToString(notificationEventRequests.map { it.toData() }) } .onFailure { Timber.e(it, "Failed to serialize notification requests") } - .map { str -> + .mapCatchingExceptions { str -> + // Note: workDataOf can fail if the data is too large workDataOf(REQUESTS_KEY to str) } } @@ -51,6 +69,7 @@ class WorkerDataConverter( companion object { private const val REQUESTS_KEY = "requests" + private const val CHUNK_SIZE = 20 } } diff --git a/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt b/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt index 7c3b165102..a6e4a26e3d 100644 --- a/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt +++ b/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt @@ -11,5 +11,5 @@ package io.element.android.libraries.workmanager.api import androidx.work.WorkRequest interface WorkManagerRequest { - fun build(): Result + fun build(): List> } diff --git a/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt b/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt index 70c9b719b2..6b19e48208 100644 --- a/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt +++ b/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt @@ -27,14 +27,16 @@ class DefaultWorkManagerScheduler( private val workManager by lazy { WorkManager.getInstance(context) } override fun submit(workManagerRequest: WorkManagerRequest) { - workManagerRequest.build().fold( - onSuccess = { - workManager.enqueue(it) - }, - onFailure = { - Timber.e(it, "Failed to build WorkManager request $workManagerRequest") - } - ) + workManagerRequest.build().forEach { + it.fold( + onSuccess = { workRequest -> + workManager.enqueue(workRequest) + }, + onFailure = { + Timber.e(it, "Failed to build WorkManager request $workManagerRequest") + } + ) + } } override fun cancel(sessionId: SessionId) { From b976d9deac656d7e8bed68fbd1a67906baf77a62 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 19 Nov 2025 17:30:34 +0100 Subject: [PATCH 2/6] Fix and add test --- .../impl/workmanager/WorkerDataConverter.kt | 3 ++- .../SyncNotificationWorkManagerRequestTest.kt | 13 +++++++------ .../workmanager/WorkerDataConverterTest.kt | 19 ++++++++++++++++++- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt index 71d24cd62d..98aea3f528 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt @@ -25,6 +25,7 @@ class WorkerDataConverter( private val json: JsonProvider, ) { fun serialize(notificationEventRequests: List): List> { + // First try to serialize all requests at once. In the vast majority of cases this will work. return serializeRequests(notificationEventRequests) .fold( onSuccess = { @@ -69,7 +70,7 @@ class WorkerDataConverter( companion object { private const val REQUESTS_KEY = "requests" - private const val CHUNK_SIZE = 20 + internal const val CHUNK_SIZE = 20 } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt index 62acaa16b1..cc4890d93f 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt @@ -21,6 +21,7 @@ import io.element.android.libraries.workmanager.api.workManagerTag import io.element.android.services.toolbox.test.sdk.FakeBuildVersionSdkIntProvider import kotlinx.coroutines.test.runTest import org.junit.Test +import kotlin.collections.first class SyncNotificationWorkManagerRequestTest { @Test @@ -32,8 +33,8 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.isSuccess).isTrue() - result.getOrNull()!!.run { + assertThat(result.first().isSuccess).isTrue() + result.first().getOrNull()!!.run { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType("requests")).isTrue() // True in API 33+ @@ -51,8 +52,8 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.isSuccess).isTrue() - result.getOrNull()!!.run { + assertThat(result.first().isSuccess).isTrue() + result.first().getOrNull()!!.run { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType("requests")).isTrue() // False before API 33 @@ -69,7 +70,7 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.isFailure).isTrue() + assertThat(result.first().isFailure).isTrue() } @Test @@ -80,7 +81,7 @@ class SyncNotificationWorkManagerRequestTest { workerDataConverter = WorkerDataConverter({ error("error during serialization") }) ) val result = request.build() - assertThat(result.isFailure).isTrue() + assertThat(result.first().isFailure).isTrue() } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt index ad6999b4ea..9c3f346a28 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt @@ -10,6 +10,7 @@ package io.element.android.libraries.push.impl.workmanager import com.google.common.truth.Truth.assertThat import io.element.android.libraries.androidutils.json.DefaultJsonProvider +import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.test.AN_EVENT_ID import io.element.android.libraries.matrix.test.AN_EVENT_ID_2 import io.element.android.libraries.matrix.test.A_ROOM_ID @@ -45,9 +46,25 @@ class WorkerDataConverterTest { ) } + @Test + fun `serializing lots of data leads to several work data generated`() { + val data = List(100) { + NotificationEventRequest( + sessionId = A_SESSION_ID, + roomId = A_ROOM_ID, + eventId = EventId(AN_EVENT_ID.value + it), + providerInfo = "info$it", + ) + } + val sut = WorkerDataConverter(DefaultJsonProvider()) + val serialized = sut.serialize(data) + assertThat(serialized.size).isGreaterThan(1) + assertThat(serialized.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE) + } + private fun testIdentity(data: List) { val sut = WorkerDataConverter(DefaultJsonProvider()) - val serialized = sut.serialize(data).getOrThrow() + val serialized = sut.serialize(data).first().getOrThrow() val result = sut.deserialize(serialized) assertThat(result).isEqualTo(data) } From 0355c0bda99402a85759e32ce5cb198b6e489fb3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 19 Nov 2025 17:53:12 +0100 Subject: [PATCH 3/6] Improve code again --- .../workmanager/DataForWorkManagerIsTooBig.kt | 10 +++++++ .../SyncNotificationWorkManagerRequest.kt | 4 +-- .../impl/workmanager/WorkerDataConverter.kt | 29 +++++++++++-------- .../SyncNotificationWorkManagerRequestTest.kt | 12 ++++---- .../workmanager/WorkerDataConverterTest.kt | 8 ++--- .../workmanager/api/WorkManagerRequest.kt | 2 +- .../impl/DefaultWorkManagerScheduler.kt | 18 +++++------- 7 files changed, 48 insertions(+), 35 deletions(-) create mode 100644 libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt new file mode 100644 index 0000000000..e700a58a7e --- /dev/null +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/DataForWorkManagerIsTooBig.kt @@ -0,0 +1,10 @@ +/* + * Copyright (c) 2025 Element Creations Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.libraries.push.impl.workmanager + +class DataForWorkManagerIsTooBig : Exception() diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt index cff12cc17d..a529beba4b 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt @@ -29,9 +29,9 @@ class SyncNotificationWorkManagerRequest( private val workerDataConverter: WorkerDataConverter, private val buildVersionSdkIntProvider: BuildVersionSdkIntProvider, ) : WorkManagerRequest { - override fun build(): List> { + override fun build(): Result> { if (notificationEventRequests.isEmpty()) { - return listOf(Result.failure(InvalidParameterException("notificationEventRequests cannot be empty"))) + return Result.failure(InvalidParameterException("notificationEventRequests cannot be empty")) } Timber.d("Scheduling ${notificationEventRequests.size} notification requests with WorkManager for $sessionId") return workerDataConverter.serialize(notificationEventRequests).map { diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt index 98aea3f528..5efda9647f 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt @@ -24,21 +24,22 @@ import timber.log.Timber class WorkerDataConverter( private val json: JsonProvider, ) { - fun serialize(notificationEventRequests: List): List> { + fun serialize(notificationEventRequests: List): Result> { // First try to serialize all requests at once. In the vast majority of cases this will work. return serializeRequests(notificationEventRequests) - .fold( - onSuccess = { - listOf(Result.success(it)) - }, - onFailure = { - // Perform serialization on sublists, workDataOf may have failed because of size limit + .map { listOf(it) } + .recoverCatching { + if (it is DataForWorkManagerIsTooBig) { + // Perform serialization on sublists, workDataOf have failed because of size limit Timber.w(it, "Failed to serialize ${notificationEventRequests.size} notification requests, trying with chunks of $CHUNK_SIZE.") - notificationEventRequests.chunked(CHUNK_SIZE).map { chunk -> - serializeRequests(chunk) + // TODO Do not split rooms + notificationEventRequests.chunked(CHUNK_SIZE).mapNotNull { chunk -> + serializeRequests(chunk).getOrNull() } - }, - ) + } else { + throw it + } + } } private fun serializeRequests(notificationEventRequests: List): Result { @@ -48,7 +49,11 @@ class WorkerDataConverter( } .mapCatchingExceptions { str -> // Note: workDataOf can fail if the data is too large - workDataOf(REQUESTS_KEY to str) + try { + workDataOf(REQUESTS_KEY to str) + } catch (_: IllegalStateException) { + throw DataForWorkManagerIsTooBig() + } } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt index cc4890d93f..9dae435a9a 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequestTest.kt @@ -33,8 +33,8 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.first().isSuccess).isTrue() - result.first().getOrNull()!!.run { + assertThat(result.isSuccess).isTrue() + result.getOrNull()!!.first().run { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType("requests")).isTrue() // True in API 33+ @@ -52,8 +52,8 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.first().isSuccess).isTrue() - result.first().getOrNull()!!.run { + assertThat(result.isSuccess).isTrue() + result.getOrNull()!!.first().run { assertThat(this).isInstanceOf(OneTimeWorkRequest::class.java) assertThat(workSpec.input.hasKeyWithValueOfType("requests")).isTrue() // False before API 33 @@ -70,7 +70,7 @@ class SyncNotificationWorkManagerRequestTest { ) val result = request.build() - assertThat(result.first().isFailure).isTrue() + assertThat(result.isFailure).isTrue() } @Test @@ -81,7 +81,7 @@ class SyncNotificationWorkManagerRequestTest { workerDataConverter = WorkerDataConverter({ error("error during serialization") }) ) val result = request.build() - assertThat(result.first().isFailure).isTrue() + assertThat(result.isFailure).isTrue() } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt index 9c3f346a28..82e47078f5 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt @@ -58,14 +58,14 @@ class WorkerDataConverterTest { } val sut = WorkerDataConverter(DefaultJsonProvider()) val serialized = sut.serialize(data) - assertThat(serialized.size).isGreaterThan(1) - assertThat(serialized.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE) + assertThat(serialized.getOrNull()?.size).isGreaterThan(1) + assertThat(serialized.getOrNull()?.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE) } private fun testIdentity(data: List) { val sut = WorkerDataConverter(DefaultJsonProvider()) - val serialized = sut.serialize(data).first().getOrThrow() - val result = sut.deserialize(serialized) + val serialized = sut.serialize(data).getOrThrow() + val result = sut.deserialize(serialized.first()) assertThat(result).isEqualTo(data) } } diff --git a/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt b/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt index a6e4a26e3d..af49c3dc87 100644 --- a/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt +++ b/libraries/workmanager/api/src/main/kotlin/io/element/android/libraries/workmanager/api/WorkManagerRequest.kt @@ -11,5 +11,5 @@ package io.element.android.libraries.workmanager.api import androidx.work.WorkRequest interface WorkManagerRequest { - fun build(): List> + fun build(): Result> } diff --git a/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt b/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt index 6b19e48208..6943c1432e 100644 --- a/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt +++ b/libraries/workmanager/impl/src/main/kotlin/io/element/android/libraries/workmanager/impl/DefaultWorkManagerScheduler.kt @@ -27,16 +27,14 @@ class DefaultWorkManagerScheduler( private val workManager by lazy { WorkManager.getInstance(context) } override fun submit(workManagerRequest: WorkManagerRequest) { - workManagerRequest.build().forEach { - it.fold( - onSuccess = { workRequest -> - workManager.enqueue(workRequest) - }, - onFailure = { - Timber.e(it, "Failed to build WorkManager request $workManagerRequest") - } - ) - } + workManagerRequest.build().fold( + onSuccess = { workRequests -> + workManager.enqueue(workRequests) + }, + onFailure = { + Timber.e(it, "Failed to build WorkManager request $workManagerRequest") + } + ) } override fun cancel(sessionId: SessionId) { From 502fd472ed5fe76465ee7a832cc32d3a37182cb3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 19 Nov 2025 18:28:18 +0100 Subject: [PATCH 4/6] Improve chunk algorithm --- .../impl/workmanager/WorkerDataConverter.kt | 45 +++++++++++--- .../workmanager/WorkerDataConverterTest.kt | 58 +++++++++++++++++++ 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt index 5efda9647f..23e66396c6 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverter.kt @@ -28,16 +28,47 @@ class WorkerDataConverter( // First try to serialize all requests at once. In the vast majority of cases this will work. return serializeRequests(notificationEventRequests) .map { listOf(it) } - .recoverCatching { - if (it is DataForWorkManagerIsTooBig) { + .recoverCatching { t -> + if (t is DataForWorkManagerIsTooBig) { // Perform serialization on sublists, workDataOf have failed because of size limit - Timber.w(it, "Failed to serialize ${notificationEventRequests.size} notification requests, trying with chunks of $CHUNK_SIZE.") - // TODO Do not split rooms - notificationEventRequests.chunked(CHUNK_SIZE).mapNotNull { chunk -> - serializeRequests(chunk).getOrNull() + Timber.w(t, "Failed to serialize ${notificationEventRequests.size} notification requests, split the requests per room.") + // Group the requests per rooms + val requestsSortedPerRoom = notificationEventRequests.groupBy { it.roomId }.values + // Build a list of sublist with size at most CHUNK_SIZE, and with all rooms kept together + buildList { + val currentChunk = mutableListOf() + for (requests in requestsSortedPerRoom) { + if (currentChunk.size + requests.size <= CHUNK_SIZE) { + // Can add the whole room requests to the current chunk + currentChunk.addAll(requests) + } else { + // Add the current chunk + add(currentChunk.toList()) + // Start a new chunk with the current room requests + currentChunk.clear() + // If a room has more requests than CHUNK_SIZE, we need to split them + requests.chunked(CHUNK_SIZE) { chunk -> + if (chunk.size == CHUNK_SIZE) { + add(chunk.toList()) + } else { + currentChunk.addAll(chunk) + } + } + } + } + // Add any remaining requests + add(currentChunk.toList()) } + .filter { it.isNotEmpty() } + .also { + Timber.d("Split notification requests into ${it.size} chunks for WorkManager serialization") + it.forEach { requests -> + Timber.d(" - Chunk with ${requests.size} requests") + } + } + .mapNotNull { serializeRequests(it).getOrNull() } } else { - throw it + throw t } } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt index 82e47078f5..2d6d51ecbe 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt @@ -15,6 +15,7 @@ import io.element.android.libraries.matrix.test.AN_EVENT_ID import io.element.android.libraries.matrix.test.AN_EVENT_ID_2 import io.element.android.libraries.matrix.test.A_ROOM_ID import io.element.android.libraries.matrix.test.A_ROOM_ID_2 +import io.element.android.libraries.matrix.test.A_ROOM_ID_3 import io.element.android.libraries.matrix.test.A_SESSION_ID import io.element.android.libraries.matrix.test.A_SESSION_ID_2 import io.element.android.libraries.push.api.push.NotificationEventRequest @@ -60,6 +61,63 @@ class WorkerDataConverterTest { val serialized = sut.serialize(data) assertThat(serialized.getOrNull()?.size).isGreaterThan(1) assertThat(serialized.getOrNull()?.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE) + // All the items are present + val deserialized = serialized.getOrNull()?.flatMap { sut.deserialize(it)!! } + assertThat(deserialized).containsExactlyElementsIn(data) + } + + @Test + fun `serializing lots of data leads to several work data generated case 2`() { + val data = List(101) { + NotificationEventRequest( + sessionId = A_SESSION_ID, + roomId = A_ROOM_ID, + eventId = EventId(AN_EVENT_ID.value + it), + providerInfo = "info$it", + ) + } + val sut = WorkerDataConverter(DefaultJsonProvider()) + val serialized = sut.serialize(data) + assertThat(serialized.getOrNull()?.size).isGreaterThan(1) + assertThat(serialized.getOrNull()?.size).isEqualTo(100 / WorkerDataConverter.CHUNK_SIZE + 1) + // All the items are present + val deserialized = serialized.getOrNull()?.flatMap { sut.deserialize(it)!! } + assertThat(deserialized).containsExactlyElementsIn(data) + } + + @Test + fun `serializing lots of data leads to several work data generated case 3`() { + val data1 = List(15) { + NotificationEventRequest( + sessionId = A_SESSION_ID, + roomId = A_ROOM_ID, + eventId = EventId(AN_EVENT_ID.value + it), + providerInfo = "info".repeat(100) + it, + ) + } + val data2 = List(3) { + NotificationEventRequest( + sessionId = A_SESSION_ID, + roomId = A_ROOM_ID_2, + eventId = EventId(AN_EVENT_ID.value + it), + providerInfo = "info".repeat(100) + it, + ) + } + val data3 = List(7) { + NotificationEventRequest( + sessionId = A_SESSION_ID, + roomId = A_ROOM_ID_3, + eventId = EventId(AN_EVENT_ID.value + it), + providerInfo = "info".repeat(100) + it, + ) + } + val data = (data1 + data2 + data3).shuffled() + val sut = WorkerDataConverter(DefaultJsonProvider()) + val serialized = sut.serialize(data) + assertThat(serialized.getOrNull()?.size).isEqualTo(2) + // All the items are present + val deserialized = serialized.getOrNull()?.flatMap { sut.deserialize(it)!! } + assertThat(deserialized).containsExactlyElementsIn(data) } private fun testIdentity(data: List) { From 3eccb5c15d432ab36bf220f580d5aa8666ad75fa Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 20 Nov 2025 12:54:06 +0100 Subject: [PATCH 5/6] Improve test names and perform more test. --- .../workmanager/WorkerDataConverterTest.kt | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt index 2d6d51ecbe..6c6998cb35 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/workmanager/WorkerDataConverterTest.kt @@ -48,7 +48,7 @@ class WorkerDataConverterTest { } @Test - fun `serializing lots of data leads to several work data generated`() { + fun `serializing lots of data leads to several work data generated - one room - 100 events should be split in 5 chunks`() { val data = List(100) { NotificationEventRequest( sessionId = A_SESSION_ID, @@ -67,7 +67,7 @@ class WorkerDataConverterTest { } @Test - fun `serializing lots of data leads to several work data generated case 2`() { + fun `serializing lots of data leads to several work data generated - one room - 101 events should be split in 6 chunks`() { val data = List(101) { NotificationEventRequest( sessionId = A_SESSION_ID, @@ -86,7 +86,7 @@ class WorkerDataConverterTest { } @Test - fun `serializing lots of data leads to several work data generated case 3`() { + fun `serializing lots of data leads to several work data generated - 3 rooms - 25 events should be split in 2 chunks and room not mixed`() { val data1 = List(15) { NotificationEventRequest( sessionId = A_SESSION_ID, @@ -118,6 +118,18 @@ class WorkerDataConverterTest { // All the items are present val deserialized = serialized.getOrNull()?.flatMap { sut.deserialize(it)!! } assertThat(deserialized).containsExactlyElementsIn(data) + // Rooms are not mixed between the chunks + val setsOfRooms = serialized.getOrNull()!! + .map { workData -> sut.deserialize(workData)!! } + .map { + it.map { request -> request.roomId }.toSet() + } + // Ensure that all sets are distinct + assertThat(setsOfRooms.size).isEqualTo(2) + // 3 roomId are present + assertThat(setsOfRooms.flatten().toSet()).containsExactly(A_ROOM_ID, A_ROOM_ID_2, A_ROOM_ID_3) + // No intersection between sets + assertThat(setsOfRooms[0].intersect(setsOfRooms[1])).isEmpty() } private fun testIdentity(data: List) { From 3c53c4d96b59c3040742b6a79f9cd6ecb5cc8446 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 20 Nov 2025 14:52:53 +0100 Subject: [PATCH 6/6] it -> dataList --- .../impl/workmanager/SyncNotificationWorkManagerRequest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt index a529beba4b..b11b83d6e4 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/workmanager/SyncNotificationWorkManagerRequest.kt @@ -34,8 +34,8 @@ class SyncNotificationWorkManagerRequest( return Result.failure(InvalidParameterException("notificationEventRequests cannot be empty")) } Timber.d("Scheduling ${notificationEventRequests.size} notification requests with WorkManager for $sessionId") - return workerDataConverter.serialize(notificationEventRequests).map { - it.map { data -> + return workerDataConverter.serialize(notificationEventRequests).map { dataList -> + dataList.map { data -> OneTimeWorkRequestBuilder() .setInputData(data) .apply {