Subject: Re: [imapext] I-D Action: draft-ietf-qresync-rfc5162bis-03.txt

From: Jan Kundrát <[email-masked]>
Date: Fri, 22 Nov 2013 17:32:58 +0100

Hi Alexey, thanks a lot for working on this. Here's a couple of comments 
from my point of view:

- The new version now requires VANISHED for all expunges, which is great. I 
don't understand why is it limited to mailboxes which do not announce 
NOMODSEQ, though. Surely the server already tracks UIDs for each and every 
message in a given mailbox *while the session is open*. But that's a very 
minor point, I guess -- upon NOMODSEQ, the general usefullness of this 
extension is substantially reduced anyway.

- The document now forces the server to either support the modseq tracking 
all the time, or not at all for a given mailbox. I believe that Dovecot at 
least used to enable this "lazily", i.e. it isn't active by default, but 
activated on demand when any client first issues a CONDSTORE-enabling 
command on a particular mailbox. My understanding is that after that 
happens, HIGHESTMODSEQ is properly updated for each session started 
afterwards. Timo, could you please confirm this? If that is the case, the 
new wording disables this.

- You probably wanted to make "RFC3501" a link in the ABNF grammar for the 
"status-att" nonterminal.

- The ABNF has a comment for the status-att-val nonterminal saying [...] 
"as described in Section 3.1.2 [[Check the ref]]". I don't think the last 
part is intentional.

- The ABNF allows for HIGHESTMODSEQ to be 0 and contains a comment 
clarifying that this is for situations where the server doesn't support 
MODSEQ tracking on this mailbox. That conflicts with the NOMODSEQ response 
code. My suggestion is to get rid of zero HIGHESTMODSEQ and make it clear 
that the alternatives are either NOMODSEQ or a non-zero HIGHESTMODSEQ; 
allowing two ways of conveying the same intention is not needed here, IMHO.

- Section 8 shall probably refer to both CONDSTORE and QRESYNC, not just 
QRESYNC.

- It appears that the interaction with the METADATA extension is not 
defined anywhere. Maybe at least mandate that changing any per-mailbox 
metadata MUST change the HIGHESTMODSEQ as well?

- The same for the ANNOTATE-EXPERIMENT-1 (or is it dead already?). Should 
changes to per-message metadata bump the message's MODSEQ?

- Finally, I still feel that there's a room for misinterpretation of the 
VANISHED. Suppose that the mailbox contains UIDs 2 and 4, the UIDNEXT is 6. 
Which of the following responses are valid and which demonstrate a broken 
server?

* VANISHED 1
* VANISHED 3
* VANISHED 5
* VANISHED 7

In my opinion, it is all of them. On the other hand, if it was about 
VANISHED EARLIER, only the VANISHED (EARLIER) 7 would be invalid and all of 
the others have to be allowed.

I therefore propose the following update, replacing this paragraphs:

   Note that a VANISHED response caused by EXPUNGE, UID EXPUNGE, or
   messages expunged in other connections MUST only contain UIDs for
   messages expunged since the last VANISHED/EXPUNGE response sent for
   the currently opened mailbox or since the mailbox was opened.  That
   is, servers MUST NOT send UIDs for previously expunged messages,
   unless explicitly requested to do so by the UID FETCH (VANISHED)
   command.  This is required to prevent a possible race condition where
   new arrivals for which the UID is not yet known by the client are
   expunged.

with something like this:

   A VANISHED response without the EARLIER tag MUST only refer to a UID
   which was visible to the session at the time the response is sent.
   In particular, this means that VANISHED response MUST NOT refer to a
   message whose removal was already communicated to the client, or to
   a message whose UID has not been present in the mailbox yet.  This is
   required to prevent a possible race condition where new arrivals for
   which the UID is not yet known by the client are expunged.

   The VANISHED EARLIER response MIGHT refer to messages whose removal
   was already known to the client.  However, it MUST NOT refer to a UID
   which has not been present in the mailbox yet (following the usual
   UIDVALIDITY semantics).

- I'm happy to see that the scenario of going out-of-sync upon new arrivals 
was addressed by the requirement to always include UID in unsolicited FETCH 
responses. Thanks!

Cheers,
Jan

-- 
Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 25 Nov 2013 19:25:46 +0000

On 22/11/2013 16:32, Jan Kundrát wrote:
> Hi Alexey, thanks a lot for working on this. Here's a couple of 
> comments from my point of view:
>
> - The new version now requires VANISHED for all expunges, which is 
> great. I don't understand why is it limited to mailboxes which do not 
> announce NOMODSEQ, though. Surely the server already tracks UIDs for 
> each and every message in a given mailbox *while the session is open*. 
> But that's a very minor point, I guess -- upon NOMODSEQ, the general 
> usefullness of this extension is substantially reduced anyway.
This was pretty much my conclusion. But I am happy to change that if 
other WG members agree with you.

> - The document now forces the server to either support the modseq 
> tracking all the time, or not at all for a given mailbox. I believe 
> that Dovecot at least used to enable this "lazily", i.e. it isn't 
> active by default, but activated on demand when any client first 
> issues a CONDSTORE-enabling command on a particular mailbox. My 
> understanding is that after that happens, HIGHESTMODSEQ is properly 
> updated for each session started afterwards. Timo, could you please 
> confirm this? If that is the case, the new wording disables this.
I am personally ambivalent on this (and would be happy to change that). 
Timo?
>
> - You probably wanted to make "RFC3501" a link in the ABNF grammar for 
> the "status-att" nonterminal.
Changed.
> - The ABNF has a comment for the status-att-val nonterminal saying 
> [...] "as described in Section 3.1.2 [[Check the ref]]". I don't think 
> the last part is intentional.
It was a reminder to me :-). Actually the section number was wrong, so I 
corrected it now.
> clarifying that this is for situations where the server doesn't 
> support MODSEQ tracking on this mailbox. That conflicts with the 
> NOMODSEQ response code. My suggestion is to get rid of zero 
> HIGHESTMODSEQ and make it clear that the alternatives are either 
> NOMODSEQ or a non-zero HIGHESTMODSEQ; allowing two ways of conveying 
> the same intention is not needed here, IMHO.
I think you are confused here: HIGHESTMODSEQ==0 is only used in STATUS 
responses. I don't think we need NOMODSEQ there.

> - Section 8 shall probably refer to both CONDSTORE and QRESYNC, not 
> just QRESYNC.
The second paragraph references both. I think the first one only applies 
to QRESYNC.

> metadata MUST change the HIGHESTMODSEQ as well?
Hmm. QRESYNC covers all message related changes. METADATA is about other 
mailbox state. I haven't thought about tracking that. I would like to 
hear other opinions on this!
> - The same for the ANNOTATE-EXPERIMENT-1 (or is it dead already?). 
> Should changes to per-message metadata bump the message's MODSEQ?
It already does. Metadata was defined to cover both flags and per 
message annotations. Do you think this is unclear?

I will reply to your last comment (omitted in this reply) in a separate 
message.

---

From: Alexey Melnikov <[email-masked]>
Date: Mon, 25 Nov 2013 19:29:03 +0000

Hi Jan,

On 22/11/2013 16:32, Jan Kundrát wrote:

> - Finally, I still feel that there's a room for misinterpretation of 
> the VANISHED. Suppose that the mailbox contains UIDs 2 and 4, the 
> UIDNEXT is 6. Which of the following responses are valid and which 
> demonstrate a broken server?
>
> * VANISHED 1
> * VANISHED 3
> * VANISHED 5
> * VANISHED 7
>
> VANISHED EARLIER, only the VANISHED (EARLIER) 7 would be invalid and 
> all of the others have to be allowed.
>
>   message whose removal was already communicated to the client,
I am agreeing with this part.
> or to
>   a message whose UID has not been present in the mailbox yet.
I think you meant what wasn't announced to the client yet.

However, I don't see a problem with this. Can you elaborate why sending 
VANISHED for UIDs the client hasn't seen yet is a problem?
> This is

Best Regards,
Alexey

---

From: Timo Sirainen <[email-masked]>
Date: Mon, 25 Nov 2013 20:51:15 +0100

On 25.11.2013, at 20.25, Alexey Melnikov <[email-masked]> wrote:

>> - The document now forces the server to either support the modseq tracking all the time, or not at all for a given mailbox. I believe that Dovecot at least used to enable this "lazily", i.e. it isn't active by default, but activated on demand when any client first issues a CONDSTORE-enabling command on a particular mailbox. My understanding is that after that happens, HIGHESTMODSEQ is properly updated for each session started afterwards. Timo, could you please confirm this? If that is the case, the new wording disables this.
> I am personally ambivalent on this (and would be happy to change that). Timo?

I’ve been thinking I should make it always track HIGHESTMODSEQ (but not per-message modseqs) to get rid of this issue, but haven’t really had time yet. But even if I do, there are quite a lot of old Dovecot installations left for many more years. So I think it would be good to say something to highly encourage clients to issue ENABLE CONDSTORE/QRESYNC before SELECT or issue CONDSTORE/QRESYNC-enabling SELECT command.

>> - It appears that the interaction with the METADATA extension is not defined anywhere. Maybe at least mandate that changing any per-mailbox metadata MUST change the HIGHESTMODSEQ as well?
> Hmm. QRESYNC covers all message related changes. METADATA is about other mailbox state. I haven't thought about tracking that. I would like to hear other opinions on this!

I don’t know if this should be a MUST, but Dovecot does increase HIGHESTMODSEQ on METADATA changes because it’s useful for syncing to know when mailbox has changed.

---

From: Jan Kundrát <[email-masked]>
Date: Mon, 25 Nov 2013 23:54:31 +0100

> I think you meant what wasn't announced to the client yet.

Yes. However, "announced" refers to stuff like EXISTS responses, i.e. 
servers should really issue VANISHED even for messages whose UIDs are still 
unknown to the client, but which were announced by the EXISTS response.

(When a new message arrives, the server always have to send EXISTS at 
first, and only then has a chance of either issuing untagged FETCH or 
waiting for client to ask for the UIDs explicitly.)

> However, I don't see a problem with this. Can you elaborate why 
> sending VANISHED for UIDs the client hasn't seen yet is a 
> problem?

Safety. QRESYNC is a complex extension and there's quite a potential for 
both servers and clients to introduce subtle bugs which only manifest 
themselves in uncommon situations. When my client sees something which 
doesn't make any sense, it's much better to reject this explicitly and 
point out that either the server or the client are broken than to lose 
data.

One example:

(two messages exists, UIDs 1 and 2)
S: * 4 EXISTS
S: * VANISHED 5

What UIDs are present at this point? Are there two, three or four messages 
in there, i.e. did VANISHED actually remoev anything?

This is why I proposed replacing EXISTS with something like the UID-based 
ARRIVED [1]. This was rejected in an attempt at reducing the amount of 
changes needed in the existing software, which makes sense. However, I 
believe that these corner cases have to described in the spec.

Cheers,
Jan

[1] http://tools.ietf.org/html/draft-kundrat-qresync-arrived-00

---

From: Jan Kundrát <[email-masked]>
Date: Tue, 26 Nov 2013 00:20:22 +0100

On Monday, 25 November 2013 20:25:46 CEST, Alexey Melnikov wrote:
> I think you are confused here: HIGHESTMODSEQ==0 is only used in 
> STATUS responses. I don't think we need NOMODSEQ there.

Yup, you're right, I missed that this is just about STATUS. I agree that 
introducing another STATUS attribute is not backwards compatible and 
therefore cannot really be used here unless we want to change the 
capability string. Trojita would disconnect upon seeing an unrecognized 
item in the STATUS response.

>> - Section 8 shall probably refer to both CONDSTORE and 
>> QRESYNC, not just QRESYNC.
> The second paragraph references both. I think the first one 
> only applies to QRESYNC.

Section 8 is about the security impact. I believe that both of these 
paragraphs apply to CONDSTORE and QRESYNC in exactly the same manner, but 
it's a style issue at most. That said, my preference is changing the text 
so that it better fits the unification of the two older RFCs.

>> - The same for the ANNOTATE-EXPERIMENT-1 (or is it dead 
>> already?). Should changes to per-message metadata bump the 
>> message's MODSEQ?
> It already does. Metadata was defined to cover both flags and 
> per message annotations. Do you think this is unclear?

That's good to hear and it certainly matches my opinion on how things 
should work, but I'm having troubles finding a reference to this behavior 
in the specs. What I see is section 2 defining "metadata" to mean "any 
[...] keyword" with a possibility of future RFCs including other items. I 
don't see anything relevant in RFC 5257, nor in the current draft.

With kind regards,
Jan