Hi, this issue affects Cyrus-Imapd 3.2.x

@elliefm: hi, did you see this issue ?

I've just tried reproducing the issue with the following Cassandane test: cyrusimap/cassandane@master...elliefm:v32/3240-sharedseen-on-shared-mailbox

But the test passes okay on both 3.2 and master, so I can't see an obvious problem here...

User subscribed to a shared mailbox

It would be useful to know whether this "shared mailbox" is a mailbox at the top level namespace (not belonging to any particular user), or whether it is a mailbox belonging to a second user that they have given the first user access to.

User has acl all on shared mailbox

It would be useful to know exactly what the ACL is set to. all is not a sensible ACL string (it means "admin and lookup only", and it might complain about l being set twice). So I assume they mean "all the settings are enabled", and am testing with lrswipkxtecdan. But it would help to know exactly what their ACL is, in case the difference is the problem.

Hi, it seems we encounter a similar problem with version 3.2 on Debian. We have recently updated 2 of our cyrusimap servers from 2.5 to 3.2.6 and our Thunderbird clients are losing Seen/Unseen status when using shared mailboxes.

We have activated imap debug log to try to figure out the problem and it seems to come from the UID STORE command which is acting like SILENT option is always enable when using shared mailboxes.

For example in Cyrus 2.5, STORE command result was :

SendData: 30 uid store 492 -Flags (\Seen)
CreateNewLineFromSocket: * 1 FETCH (FLAGS () UID 492)
CreateNewLineFromSocket: 30 OK Completed

And with Cyrus 3.2.6, the result is :

SendData: 17 uid store 1 +Flags (\Seen)
CreateNewLineFromSocket: 17 OK Completed

As Thunderbird didn't receive the expected result, its cache is losing its head and only a stop/start of TB can recover correct operation. This situation only produce when we're using shared mailboxes as others mailboxes work well in TB.
We're using lrswipkxtecdan acl and the problem seems to only exist with the \Seen flag, for example STORE command with \Flagged flag produces the expected result

PS: A precision, as I checked the cassandane test, the problem isn't in writing the flag, as the flag is correctly positioned on the message by the server. The problem is only in the result of the STORE command which doesn't FETCH the flags.

PPS: RFC 3501 (https://datatracker.ietf.org/doc/html/rfc3501#section-6.4.6) says

  The STORE command alters data associated with a message in the
  mailbox.  Normally, STORE will return the updated value of the
  data with an untagged FETCH response.  A suffix of ".SILENT" in
  the data item name prevents the untagged FETCH, and the server
  SHOULD assume that the client has determined the updated value
  itself or does not care about the updated value.

We're available for more tests or info if necessary

After a search in the code, here is the observation we made :
In the index_tellchanges function (https://github.com/cyrusimap/cyrus-imapd/blob/cyrus-imapd-3.2/imap/index.c#L3756) we have :

        /* report if it's changed since last told */
        if (im->modseq > im->told_modseq)
            index_printflags(state, msgno, printuid, printmodseq);

But for the \Seen flag, when "sharedseen" is disabled, the change of the flag doesn't update the value of im->modseq. And this probably come from the index_storeflag function. In 3.2 we have : https://github.com/cyrusimap/cyrus-imapd/blob/cyrus-imapd-3.2/imap/index.c#L4868

    r = msgrecord_set_systemflags(msgrec, system_flags);
    if (r) return r;
    r = msgrecord_set_internalflags(msgrec, internal_flags);
    if (r) return r;
    r = msgrecord_set_userflags(msgrec, user_flags);
    if (r) return r;

And in 2.5 : https://github.com/cyrusimap/cyrus-imapd/blob/cyrus-imapd-2.5/imap/index.c#L3576

    r = index_rewrite_record(state, msgno, record);
    if (r) return r;

As index_rewrite_record function seems to do a lot more, like updating the modseq value of im and \Seen in this case isn't a system, internal or user flag. We've made a first patch adding a call to index_rewrite_record at the end of the index_storeflag function which seems to solve the issue. This is probably unoptimized but I will start a PR based on this first patch

For example in Cyrus 2.5, STORE command result was :

SendData: 30 uid store 492 -Flags (\Seen)
CreateNewLineFromSocket: * 1 FETCH (FLAGS () UID 492)
CreateNewLineFromSocket: 30 OK Completed

And with Cyrus 3.2.6, the result is :

SendData: 17 uid store 1 +Flags (\Seen)
CreateNewLineFromSocket: 17 OK Completed

Oh that's good intel, thanks. I thought the problem was that the flag was either not saved or lost, not that the response confused the client. I think my test case from a few weeks ago was only looking for the OK status, but not the untagged response. I'll update it and see if I can finally reproduce this!

Thanks to @tpayen's info I've been able to update my test branch to reproduce the problem. It affects 3.4 and master too, not just 3.2. I've also run it with @tpayen's patch in #3577 and it does seem to fix it -- I'll comment briefly there too, but I'm trying to keep the discussion in one place.

I'm about to add similar "check the untagged responses" logic to the rest of our flags tests, to see whether the regression is isolated to the \Seen flag on shared mailboxes without sharedseen, or if it goes deeper than that.

A while ago I mentioned having tests for this, but the link stopped working when we stopped keeping cassandane on its own repo. Those tests are here now: master...elliefm:cyrus-imapd:v35/3240-sharedseen-on-shared-mailbox