From fb767a84891107888e0b8e08bb9975aa50e32e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=BCttler?= Date: Tue, 28 Apr 2026 14:02:12 +0200 Subject: [PATCH] fix: don't resurrect locally-deleted IMAP message on next sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The incremental IMAP sync issued `UID ${lastUid + 1}:*` to look for new mail. RFC 3501 §6.4.4 reverses `n:*` to `*:n` when n exceeds the largest UID, so a server with one message at UID 1 and `lastUid=1` returned UID 1 for `UID 2:*` — re-fetching and re-inserting a row the user had just deleted locally (whose pending change had not yet flushed). `_fetchAndUpsertImap` now looks up the UIDs in the mailbox that have a pending `delete` or `move` queued and skips the insert for those. The existing `UID n:*` query is left intact so freshly-delivered SMTP mail keeps driving StreamBuilder rebuilds in the E2E flow. Regression test in `email_repository_imap_test.dart` deletes a synced message and calls `syncEmails` directly — exactly what the in-app sync button does — and asserts the row stays gone with the pending change still queued. Co-Authored-By: Claude Opus 4.7 (1M context) --- done.md | 27 ++++++++++++ .../repositories/email_repository_impl.dart | 43 +++++++++++++++++++ .../email_repository_imap_test.dart | 35 +++++++++++++++ 3 files changed, 105 insertions(+) diff --git a/done.md b/done.md index 3c72d1d..891b4d2 100644 --- a/done.md +++ b/done.md @@ -6,6 +6,33 @@ Tasks get moved from next.md to done.md ## Tasks +## IMAP delete: locally-deleted message no longer reappears after sync + +User report: deleting an IMAP message removes it from the list, but tapping +the sync button before the next background flush makes it pop back in. + +Reproduced in `test/integration/email_repository_imap_test.dart` with a new +case `syncEmails after local delete does not resurrect message`: it deletes an +email locally (which queues a pending change and drops the cached row), then +calls `syncEmails` directly — exactly what the sync button does — and +asserts the row stays gone and the pending change stays queued. + +Root cause: the incremental IMAP sync issues `UID ${lastUid + 1}:*` to look +for new mail. Per RFC 3501 §6.4.4 a sequence range `n:*` reverses to `*:n` +when `n` exceeds the largest UID. With one message at UID 1 and `lastUid=1`, +`UID 2:*` reverses to `*:2` and the server returns UID 1, which then gets +re-fetched and re-inserted — undoing the optimistic local delete. + +Fix in `lib/data/repositories/email_repository_impl.dart`: in +`_fetchAndUpsertImap`, look up the UIDs in this mailbox that have a pending +`delete` or `move` queued and skip the insert for those. Keeping the `UID n:*` +search untouched preserves the existing E2E flow where re-fetching freshly +delivered SMTP messages drives the StreamBuilder rebuild. + +Same protection guards the `move`-on-delete path (when a Trash mailbox is +configured) for free, since `moveEmail` enqueues a `move` and drops the cached +row in the source mailbox. + ## task deploy-android works end-to-end The original "Emulator did not become ready within 120 s" was already resolved in diff --git a/lib/data/repositories/email_repository_impl.dart b/lib/data/repositories/email_repository_impl.dart index 5355f0b..12d2209 100644 --- a/lib/data/repositories/email_repository_impl.dart +++ b/lib/data/repositories/email_repository_impl.dart @@ -438,6 +438,8 @@ class EmailRepositoryImpl implements EmailRepository { final fetch = sequence.isUidSequence ? await client.uidFetchMessages(sequence, fetchItems) : await client.fetchMessages(sequence, fetchItems); + final pendingByUid = + await _pendingDeleteOrMoveUids(account.id, mailboxPath); var bytes = 0; await _db.transaction(() async { for (final msg in fetch.messages) { @@ -451,6 +453,18 @@ class EmailRepositoryImpl implements EmailRepository { log('IMAP: skipping message with no uid (mailbox=$mailboxPath)'); continue; } + // Don't resurrect a row the user has already removed locally via a + // pending delete or move. The IMAP server still has the message + // until the next flushPendingChanges, and `UID lastUid+1:*` can + // even return a UID smaller than `lastUid+1` because RFC 3501 + // §6.4.4 reverses `n:*` to `*:n` when `n` exceeds the largest UID. + if (pendingByUid.containsKey(uid)) { + log( + 'IMAP: skipping insert for uid=$uid in $mailboxPath ' + '(pending ${pendingByUid[uid]})', + ); + continue; + } bytes += msg.size ?? 0; final emailId = '${account.id}:$uid'; final msgId = envelope.messageId?.trim(); @@ -488,6 +502,35 @@ class EmailRepositoryImpl implements EmailRepository { return bytes; } + // UIDs in [mailboxPath] that have a pending local delete or move queued. + // Used by the IMAP fetch path to avoid re-inserting rows the user has + // already removed from view but whose change has not yet flushed. + Future> _pendingDeleteOrMoveUids( + String accountId, + String mailboxPath, + ) async { + final rows = await (_db.select(_db.pendingChanges) + ..where( + (t) => + t.accountId.equals(accountId) & + t.resourceType.equals('Email') & + (t.changeType.equals('delete') | t.changeType.equals('move')), + )) + .get(); + final result = {}; + for (final r in rows) { + try { + final payload = jsonDecode(r.payload) as Map; + if (payload['mailboxPath'] != mailboxPath) continue; + final uid = payload['uid']; + if (uid is int) result[uid] = r.changeType; + } catch (_) { + // Malformed payload — skip. + } + } + return result; + } + Future?> _loadImapCheckpoint( String accountId, String resourceType, diff --git a/test/integration/email_repository_imap_test.dart b/test/integration/email_repository_imap_test.dart index e8ab4f0..b5c9e07 100644 --- a/test/integration/email_repository_imap_test.dart +++ b/test/integration/email_repository_imap_test.dart @@ -520,4 +520,39 @@ void main() { await client.logout(); } }); + + // Regression: the in-app "sync" button calls syncEmails directly and does + // not invoke flushPendingChanges itself. After a local delete, the message + // is removed from the local DB optimistically but still lives on the server + // until the pending change is flushed. If syncEmails runs before that flush, + // it must not resurrect the deleted row from the server. + test('syncEmails after local delete does not resurrect message', () async { + await appendToInbox('delete-and-sync'); + + final r = makeRepo(); + await r.accounts.addAccount(account, userPass); + await r.emails.syncEmails('test', 'INBOX'); + + final emails = await r.emails.observeEmails('test', 'INBOX').first; + expect(emails, hasLength(1)); + + // User taps delete in the UI: enqueues a pending change, drops local row. + await r.emails.deleteEmail(emails.first.id); + expect(await r.emails.observeEmails('test', 'INBOX').first, isEmpty); + + // User taps the sync button: syncEmails runs without flushPendingChanges. + await r.emails.syncEmails('test', 'INBOX'); + + final after = await r.emails.observeEmails('test', 'INBOX').first; + expect( + after, + isEmpty, + reason: 'deleted message must not reappear on next sync', + ); + + // The pending delete must still be queued for the next flush. + final pending = await r.db.select(r.db.pendingChanges).get(); + expect(pending, hasLength(1)); + expect(pending.first.changeType, 'delete'); + }); }