Don't delete conversations that arrive while a list sync is in flight

[AndyScherzinger]

A conversation list sync reads the locally known conversations after the server has answered, and treats every one the response does not mention as a conversation the user left. A conversation that reached the database while the request was in flight — a single-room fetch, a room joined on another device, one the user just created — was never in that response and could not have been, so it was deleted, taking its cached messages and chat blocks with it through the foreign key cascade.

What it does

Takes the set of known conversation ids before the request goes out, and reconciles removals only against those. A conversation that appeared afterwards is left alone until the next full sync, which is the first one whose response could actually speak about it.

Why not simply read the conversations earlier

The same read feeds preservePendingLocalState, which needs the freshest local state to protect an in-flight favourite or read marker. Moving it earlier would trade this bug for that one. Only the removal reconcile wants the older snapshot, so only it gets one — as an id-only query, so the extra read does not load every conversation row.

Note for reviewers

detekt is red on this branch. It is red on master too, at the same count: 111 weighted issues against a maxIssues of 110, measured on master with this branch stashed. This change adds none.

🚧 TODO

• ...

🏁 Checklist

• ⛑️ Tests (unit and/or integration) are included or not needed
• 🔖 Capability is checked or not needed
• 🔙 Backport requests are created or not needed: /backport to stable-xx.x
• 📅 Milestone is set
• 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI

[github-actions]

📱 QA build

Download	app-qa-debug.apk
QR code	Open the QR code for this download
Commit	9422f58
Version	6749
Available until	7 days after this build

The QA build installs alongside a released Nextcloud app, so you can keep
using your existing install while testing.

Downloading the file requires a GitHub account, so open this link on the
device you want to test on, or transfer the APK to it.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: 934bc5d
• Latest commit, not reviewed: 9422f58

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e6a03840-d8f3-400a-b04b-e1698383f7a1

📥 Commits

Reviewing files that changed from the base of the PR and between ca5baa0 and 934bc5d.

📒 Files selected for processing (4)

• app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt
• app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt
• app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt
• app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The repository now captures an account’s conversation IDs before requesting rooms from the server. It limits sync deletion candidates to conversations in that snapshot. ConversationsDao adds an account-scoped ID query, and the preview DAO returns an empty list. Repository tests cover a conversation that appears in a later row read during the request.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 934bc

The sync protects conversations that arrive during a request, and the regression test exercises deletion filtering with a non-empty server response. The earlier test-failure concern is resolved; no material merge risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 934bc

The change narrowly reduces accidental cache deletion while preserving account isolation and atomic database updates. No introduced security weakness was identified. Remaining uncertainty concerns existing synchronization ordering and server-response completeness assumptions.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed destructive operation remains bounded to the requesting account's conversation IDs under the existing identity invariant. The filter reduces deletion eligibility; retaining a post-snapshot conversation changes local cache reconciliation, not server authorization.

Trust Boundaries and Controls

• observed — The server request still uses the existing user and base URL. Response conversations cross into local persistence through the same account-qualified mapping and transactional DAO consumer; no new authority transition appears in the changed path.

Resilience and Maintainability Implications

• inferred — Retries precede database mutation, deletion and upsert share a transaction, and cursor persistence follows successful reconciliation. Cancellation is rethrown. Interruption after database commit but before cursor persistence can cause replay rather than a partially applied deletion/upsert transaction.
• inferred — Per-sync atomicity does not establish ordering between overlapping requests or completeness of non-empty full responses. These limitations predate the PR and were not shown to become more exposed by its narrower deletion filter.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly and concisely describes the main change: preventing deletion of conversations added while a list sync is in flight.
Description check		The description explains the bug, the fix, the design rationale, testing, known detekt status, and checklist items. The template's screenshot section is omitted and the TODO retains its placeholder, b…

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f13dcaf9-a12c-479b-b7ad-5e14bac0aa90

📥 Commits

Reviewing files that changed from the base of the PR and between ab89e1d and 554f690.

📒 Files selected for processing (4)

• app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt
• app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt
• app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt
• app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[AndyScherzinger]

Note for review, since #6746 landed fix(conversations): run room list syncs of one account one at a time after this PR was opened: that commit does not make this one redundant, and it is worth being explicit about why, because the two look similar.

The mutex there serialises room list syncs of the same account against each other. The race it fixes is a delta response being written after a full sync has deleted a conversation, putting it back.

This PR fixes a different race: a room list sync against anything else that writes a conversation while its request is in flight — a single-room fetch, a room joined on another device, one the user just created. None of those take the room list lock, so serialising syncs leaves the window open. The response still cannot mention a conversation that did not exist when the server answered, and reconciling against the rows as they are after the answer still reads that as a conversation the user left.

One thing did improve from the merge, by accident rather than design. The id snapshot is taken inside syncRoomList, which now runs inside syncRoomListExclusively's lock, so a sync that waited for another one takes its snapshot after acquiring the lock rather than before waiting. A snapshot taken before the wait would have been stale by exactly the amount of time it waited.

No change needed here; rebased onto the merge and both of that commit's new tests pass alongside this one.

[github-actions]

Codacy

Lint

Type	master	PR
Warnings	138	138
Errors	15	15

SpotBugs

Category	Base	New
Bad practice	7	7
Correctness	8	8
Dodgy code	40	40
Internationalization	3	3
Malicious code vulnerability	3	3
Performance	8	8
Security	11	11
Total	80	80