Conversation
Text messages were sent from a plain viewModelScope coroutine tied to ChatActivity's ViewModel. If the network was down and the user left the chat before it recovered, the coroutine (and the only listener that retried on reconnect) was destroyed with it, leaving the message stuck locally with nothing left to retry it. SendMessageWorker now performs the send, chained per conversation via WorkManager so it keeps running independently of the chat screen and retries transient failures with backoff. The manual "resend" action now goes through the same worker. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
SendMessageWorker had no way of knowing a temporary message was deleted (via the message actions sheet) after it was enqueued but before connectivity allowed it to actually run, so it still posted the message to the server. Every attempt now re-checks that the temporary message still exists before sending, and skips the send if it's gone. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
… time Editing a still-queued (offline) message only updated its local temp message row (editTempChatMessage); SendMessageWorker kept sending the original text it had captured into WorkManager's input data at enqueue time, so the edit was silently lost once connectivity returned. The worker now reads the message text, and the other send parameters that are already persisted on that same row (account id, room token, display name, reply-to, silent flag), fresh at send time instead of from WorkManager's input data. This also removes the redundant copies of that data from both enqueue() call sites. addTemporaryMessage() now explicitly emits a success result so callers don't have to rely on "no emission means success"; MessageInputViewModel enqueues the worker only after that emission confirms the temporary message row actually exists, closing a race where the worker could run before the row was written and mistake that for a deleted message. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
sendChatMessage() launched an independent coroutine per call, each awaiting its own local temporary-message DB write before enqueueing SendMessageWorker. Those writes weren't guaranteed to finish in the order they were started, so messages sent in quick succession could reach SendMessageWorker's per-conversation queue - and therefore the server - out of order, until a later resync corrected it. A Mutex now serializes the insert-then-enqueue step per conversation, held across the whole step rather than just the enqueue call, so one send fully completes locally before the next queued send is allowed to start its own DB write. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Adds unit tests for the two OfflineFirstChatRepository behaviors this branch changed: addTemporaryMessage now emits Result.success on the happy path (it previously emitted nothing there), and markMessageForResend only resets a temp message's local status back to PENDING - it no longer sends anything itself, since that's SendMessageWorker's job now. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…early
addTemporaryMessage() wrapped its emit() calls in a single broad
catch (e: Exception) block:
try {
...
emit(Result.success(...))
} catch (e: Exception) {
emit(Result.failure(e))
}
Cancellation in coroutines works by throwing a CancellationException at
whatever line is currently suspended, and relies on every catch block
along the way letting it continue upward unchanged so the cancellation
actually reaches the top and the coroutine stops. A caller that stops
collecting after one value - e.g. Flow.first()/take(1) - does exactly
this: the moment it receives a value from emit(), it throws a
cancellation-style exception right back into that emit() call to signal
"got it, stop sending".
Because CancellationException is still a subtype of Exception on the
JVM, the catch block above did not distinguish "the collector is done
listening" from "something actually failed". It caught that
cancellation the same way as a real failure and tried to recover by
calling emit(Result.failure(e)) - except the collector had already
disconnected, so that second emit() is illegal and crashed with
"Flow exception transparency is violated: emission attempt ... after
a previous emit() has thrown".
This was not just a test artifact: the exact same crash could happen in
production if MessageInputViewModel's coroutine (in
sendChatMessage()) gets cancelled - e.g. ChatActivity/its ViewModel
being cleared - at the exact moment it is suspended on this emit()
call.
Fixed by catching CancellationException first and rethrowing it
unchanged, before the generic catch (e: Exception) gets a chance to
treat it as a failure:
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
emit(Result.failure(e))
}
The generic catch now only ever sees genuine failures (e.g. the DB
write itself throwing), and a collector disconnecting early is once
again just allowed to finish cancelling, the way structured concurrency
expects.
Added a regression test that collects addTemporaryMessage() with
first() directly, reproducing the exact crash scenario and confirming
it now returns the successful result cleanly instead of throwing.
Two other methods in this file (editChatMessage, editTempChatMessage)
have the identical emit()-inside-try/catch(Exception) shape and are
theoretically exposed to the same issue, but are pre-existing code
untouched by this branch - left alone for now, not fixed here.
Assisted-by: Claude Code:claude-sonnet-5
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
mahibi
marked this pull request as ready for review
September 18, 2026 11:35
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/35340361411/artifacts/10544343537 |
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.
What changed (4 commits on this branch):
the only reconnect-listener that retried sending) died with it, so the message never went out. Added SendMessageWorker, a CoroutineWorker enqueued via WorkManager
and chained per conversation (ExistingWorkPolicy.APPEND_OR_REPLACE), which survives leaving the chat, backgrounding, and process death, and retries with backoff.
Both the normal send path and the manual "resend failed message" action now go through it.
once connectivity returned. The worker now re-checks that the temporary message row still exists at the start of every attempt (including retries) and skips the
send if it's gone.
WorkManager's input data at enqueue time. The worker now reads the message text and other send parameters fresh from that same row at send time instead of from stale
captured data, so edits (and everything else already persisted on that row) are always picked up.
until the periodic resync corrected it), because each send's local DB write and worker-enqueue ran as an independent, unordered coroutine. A Mutex now serializes
that step per conversation so sends reach the server in the order they were actually sent.
Suggested manual test steps:
network back on. Reopen the chat — the message should now show as sent (not stuck pending/failed).
client) — it should not have been sent.
on the server has the edited text, not the original.
device vs. a second client (web or another device) — they should match with no reordering.
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)