Skip to content

Comments

feat: Use IMAP APPEND command to upload sync messages (#5845)#5901

Merged
iequidoo merged 2 commits intomainfrom
iequidoo/sync-msgs-via-imap
Sep 20, 2024
Merged

feat: Use IMAP APPEND command to upload sync messages (#5845)#5901
iequidoo merged 2 commits intomainfrom
iequidoo/sync-msgs-via-imap

Conversation

@iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Aug 20, 2024

Close #5845

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch 6 times, most recently from 8ec208b to 0f6f3f1 Compare August 23, 2024 01:51
@iequidoo

This comment was marked as resolved.

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch 2 times, most recently from 065ed07 to 54cd51a Compare August 23, 2024 23:31
@iequidoo

This comment was marked as outdated.

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 54cd51a to 4b6908f Compare August 24, 2024 01:34
@iequidoo iequidoo marked this pull request as ready for review August 24, 2024 01:45
@iequidoo iequidoo requested review from link2xt and r10s August 24, 2024 02:24
@link2xt
Copy link
Collaborator

link2xt commented Aug 29, 2024

Adding optional arguments for APPEND here: chatmail/async-imap#106

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 4b6908f to 81d26b8 Compare August 30, 2024 21:56
@iequidoo iequidoo marked this pull request as draft August 30, 2024 22:47
@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 81d26b8 to 9b17493 Compare August 30, 2024 22:54
@iequidoo iequidoo marked this pull request as ready for review August 31, 2024 00:31
@iequidoo iequidoo requested a review from link2xt August 31, 2024 00:32
@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 9b17493 to 60dcf71 Compare September 1, 2024 22:48
@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 60dcf71 to 55d4c76 Compare September 5, 2024 02:29
@iequidoo
Copy link
Collaborator Author

iequidoo commented Sep 5, 2024

I made a dumb merge, but this actually breaks what was fixed in 418dfbf because groups are promoted by SMTP, but QR code tokens are synced by IMAP. Need to think how to fix this.

EDIT: True, JSON-RPC tests are broken now: https://github.com/deltachat/deltachat-core-rust/actions/runs/10712806799/job/29704001842?pr=5901

@iequidoo iequidoo marked this pull request as draft September 5, 2024 02:34
@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 55d4c76 to d294ac3 Compare September 5, 2024 02:44
@link2xt
Copy link
Collaborator

link2xt commented Sep 5, 2024

This race condition between getting a group promoted and receiving a sync message for its QR code is already a problem even without this PR if sync messages are moved to DeltaChat but group messages are not because these folders are fetched in arbitrary order. If second device fetches DeltaChat and gets sync message before fetching INBOX and receiving first group message, it is going to drop the QR code.

Maybe QR code token should be saved into tokens regardless of whether the group exists, but if we don't know about this group (yet) and cannot accept join requests, we should ignore join requests and let the first device handle them.

In the tests we need to artificially wait until QR code and group is synced before scanning the QR code.

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from d294ac3 to a39b719 Compare September 6, 2024 16:23
@iequidoo
Copy link
Collaborator Author

iequidoo commented Sep 6, 2024

What is the reason to introduce "IMAP APPEND" at all? We typically try to use IMAP only in a minimal way.

I forgot to add the motivation to the commit message, done now:

  • With IMAP APPEND we can upload messages directly to the DeltaChat folder (for non-chatmail
    accounts).
  • We can set the \Seen flag immediately so that if the user has other MUA, it doesn't alert about
    a new message if it's just a sync message (there were several such reports on the support
    forum). Though this also isn't useful for chatmail.
  • We don't need SMTP envelope and overall remove some overhead on processing sync messages.

Are there any server implementations not supporting APPEND? I think this may be the only blocker

@link2xt
Copy link
Collaborator

link2xt commented Sep 6, 2024

What is the reason to introduce "IMAP APPEND" at all? We typically try to use IMAP only in a minimal way.

Primary reason is avoiding sync messages appearing in the inbox before being moved into DeltaChat folder because user complain that they appear in the web interface:
https://support.delta.chat/t/receiving-blank-emails-with-encrypted-attachments-whenever-an-email-blocked-deleted-or-accepted/3206
Gmail seems to keep the message displayed even some time after moving, so placing it directly into DeltaChat avoids this.

@link2xt
Copy link
Collaborator

link2xt commented Sep 6, 2024

Are there any server implementations not supporting APPEND?

Unlikely, most clients save sent messages into Sent folder using APPEND.

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from a39b719 to b329ada Compare September 7, 2024 00:40
@iequidoo iequidoo changed the base branch from main to iequidoo/async-qr-code-tokens September 7, 2024 01:28
@iequidoo iequidoo changed the base branch from iequidoo/async-qr-code-tokens to main September 7, 2024 01:35
@link2xt
Copy link
Collaborator

link2xt commented Sep 8, 2024

I opened an issue regarding APPEND response parsing in async-imap, going to look into it: chatmail/async-imap#108

Edit: made a PR chatmail/async-imap#109

Edit2: published async-imap 0.10.1

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch 2 times, most recently from 3e832ca to dd410d1 Compare September 18, 2024 02:07
@iequidoo iequidoo marked this pull request as ready for review September 18, 2024 02:19
@iequidoo iequidoo requested a review from link2xt September 18, 2024 02:21
src/imap.rs Outdated
// does not do so, the client MAY issue a NOOP command (or failing
// that, a CHECK command) after one or more APPEND commands.
//
// So deselect the folder just in case if the server doesn't send EXISTS. Then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually happen that server does not send EXISTS? Which server it is?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a first device that APPENDs a message and second device which IDLEs in the folder and should receive a sync message. If the server does not send EXISTS, second device will not notice a new message. Closing the folder on the first device does not help with this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually happen that server does not send EXISTS? Which server it is?

No, at least with chatmail servers.

IMU this RFC paragraph isn't about EXISTS related to APPEND itself, but about unsolicited EXISTS responses while APPEND is in progress. As for second device, i think any reasonable server implementation should notify it for appended messages too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think RFC warns about some misbehaving servers that first flush unsolicited EXISTS messages and then append the message, so EXISTS for the just appended message is only flushed on the next command. This is why NOOP is useful if you want to get EXISTS for a just added message, e.g. to learn its sequence number. But we don't care about the sequence number as we only use UIDs.

If the server does not notify with unsolicited EXISTS about previous messages that were added before, then I guess it is so broken that it will also not notify after NOOP and during IDLE (and probably does not support IDLE). There is nothing special about APPEND that justifies closing and reopening the folder to check UIDNEXT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a similar paragraph for the CHECK command:

      There is no guarantee that an EXISTS untagged response will happen
      as a result of CHECK.  NOOP, not CHECK, SHOULD be used for new
      message polling.

But i don't see such warnings for other commands. Also see https://www.rfc-editor.org/rfc/rfc3501#section-7:

   An example of unilateral untagged server data occurs when the IMAP
   connection is in the selected state.  In the selected state, the
   server checks the mailbox for new messages as part of command
   execution.  **Normally**, this is part of the execution of every command;
   hence, a NOOP command suffices to check for new messages.  If new
   messages are found, the server sends untagged EXISTS and RECENT
   responses reflecting the new size of the mailbox.

So i'm afraid this is about some strange implementations that don't send untagged EXISTS in all commands and we shouldn't rely on that. There's nothing too bad with closing the folder before APPEND, anyway the next step is store_seen_flags_on_imap() which may select another folder, so maybe it's not worth optimising here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CHECK description is just a bad wording in an attempt to fix previous bad wording in IMAP 2 specification.
IMAP 2 specification https://datatracker.ietf.org/doc/html/rfc1176 said:
"The CHECK command forces a check for new messages and a rescan of the mailbox for internal change for those implementations that allow multiple simultaneous read/write access to the same mailbox. It is recommend that periodic implicit checks for new mail be done by servers as well. The server should send unsolicited EXISTS and RECENT responses with the current status before returning an OK to the client."
This means that unsolicited EXISTS should be sent if there is a pending unsolicited EXISTS, not always.

https://datatracker.ietf.org/doc/html/rfc1732 attempted to clarify:
"Poor wording in the description of the CHECK command in earlier specifications implied that a CHECK command is the way to get the current number of messages in the mailbox. This is incorrect. A CHECK command does not necessarily result in an EXISTS response. Clients must remember the most recent EXISTS value sent from the server, and should not generate unnecessary CHECK commands."

This was shortened to "There is no guarantee that an EXISTS untagged response will happen as a result of CHECK." in IMAP4 spec, but is impossible to understand out of context.

"NOOP, not CHECK, SHOULD be used for new message polling." is another unrelated clarification that I think means that for polling clients should not use CHECK because this forces the server to poll the disk and it should do this periodically anyway ("It is recommend that periodic implicit checks for new mail be done by servers as well."). This is from the time when inotify and IDLE did not exist so IMAP servers polled the disk and IMAP clients polled the server.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i'm afraid this is about some strange implementations that don't send untagged EXISTS in all commands and we shouldn't rely on that.

Overall, CHECK description just says that there may be no EXISTS if there is no pending unsolicited EXISTS (as someone may think if they read poorly written IMAP 2 spec and wanted something like STATUS instead which did not exist in IMAP 2) and APPEND description says that unsolicited EXISTS for just appended message may be flushed on the next command by some old servers.

I don't see any indication that there are buggy servers out there that for some reason flush unsolicited responses for NOOP but not for APPEND. If the servers are that buggy, then we will miss message notification anyway when we don't have anything to APPEND. Most of the time we don't have outgoing sync messages so will not CLOSE before APPEND anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw IMAP4rev2 even removed CHECK completely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for CHECK I looked into how this APPEND note was added. Tracked it to this diff:
https://author-tools.ietf.org/iddiff?url1=draft-crispin-imap-base-04&url2=draft-crispin-imap-base-05&difftype=--html

There is a related thread around the same date:
https://mailarchive.ietf.org/arch/msg/imap/jRMqDuN50oThnBkZVqcn0Se1DLQ/

Relevant quote:

4) When does APPEND complete?

When a client does an APPEND, it doesn't necessairly "see" the new 
messages (via EXISTS) immediately.  It was agreed that if a client wishes 
to see the messages it should issue a CHECK after a series of APPEND 
operations.  This works with all tested server implementations (NOOP doesn't 
work with all of them).  Here's some proposed text:

    If the destination mailbox for APPEND is the currently selected 
    mailbox, the server does not need to notify the client immediately 
    via a "* EXISTS" response.  Clients wishing to view the appended 
    messages should issue a CHECK command after a series of APPEND 
    commands.

This reflects current practice, and actually gives a use for the CHECK 
command.

Then there is a whole thread discussing implementation details of some "Innosoft mail server" that only checks when CHECK is issued and some server developed by Mark Crispin (UW IMAP probably) that only checked on NOOP but not on CHECK due to a bug. Here is the source code of the IMAP server in question:
https://github.com/uw-imap/imap/blob/master/src/imapd/imapd.c

The thread is discussing quircs of the servers that existed in 1996 and were considered bugs back then already. Servers polled the disk as there was no inotify or similar mechanisms. The other half of the thread is discussing semantics of UIDs which were just introduced.

First version of Dovecot was released only in 2002.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed closing the folder before APPEND, but left uploading sync messages only from the inbox loop because if the mvbox for some reason can't be created and thus the mvbox loop doesn't exists, sync messages must be uploaded from the inbox loop anyway. And in this case they will be uploaded to Inbox.

@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from dd410d1 to 73002ec Compare September 19, 2024 20:37
Why:
- With IMAP APPEND we can upload messages directly to the DeltaChat folder (for non-chatmail
  accounts).
- We can set the `\Seen` flag immediately so that if the user has other MUA, it doesn't alert about
  a new message if it's just a sync message (there were several such reports on the support
  forum). Though this also isn't useful for chatmail.
- We don't need SMTP envelope and overall remove some overhead on processing sync messages.
@iequidoo iequidoo force-pushed the iequidoo/sync-msgs-via-imap branch from 73002ec to 13d7502 Compare September 19, 2024 21:01
@iequidoo iequidoo requested a review from link2xt September 19, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use IMAP APPEND command to upload sync messages with \Seen flag

3 participants