Fixing issue #3240 Seen/Unseen Flag not working correctly if sharedseen of a shared mailbox is not set to true

[tpayen]

Following the issue #3240, when using \Seen flag with shared seen disable, modseq isn't updated so store command never return the fetch flags. So we force the update of modseq value by calling index_rewrite_record function

This is probably unoptimized patch as we can unify the call of index_rewrite_record with others functions

[elliefm]

Have confirmed by testing that this patch does seem to fix the problem, and doesn't seem to introduce any new ones (at least: not that our tests find), but I'm keeping the full discussion in the linked issue #3240 for now.

[elliefm]

This looks okay, and my tests (here) confirm that it a) fixes the problem, and b) doesn't break anything else that we have a test for. It also cherry picks cleanly onto the later branches, and a) and b) still apply for each.

But I don't know the msgrecord or index APIs in enough depth to tell whether this is the right way to do it, so can we get some experienced eyes on this please?

[ksmurchison]

This also looks ok to me, but I'll defer to @brong to determine if there is a better way to bump the modseq.

[brong]

This was fixed slightly differently in later code. This code will bounce the modseq twice, while moving the "get_index_record" and "rewrite_record" after the msgrecord_save() like the current code does will be slightly more efficient.

[tpayen]

This was fixed slightly differently in later code. This code will bounce the modseq twice, while moving the "get_index_record" and "rewrite_record" after the msgrecord_save() like the current code does will be slightly more efficient.

Thanks for the review. As the end of the loop in index_store function already call index_rewrite_record I'm not sure to understand why the modseq isn't bounce in this case. Unless we have "nothing to do" after the index_storeflag call and it use the goto doneloop ? I clearly understand that we're doing things twice with this patch but I don't really know how to do it better for now.

[elliefm]

@brong

This was fixed slightly differently in later code.

Looks like it wasn't fully, though! The new Flags.seen_sharedmb_nosharedseen test in cyrusimap/cassandane#142 (not merged yet) fails on current Cyrus master.

It fails at this line -- the \Seen flag is stored okay, but we expect an untagged response and don't get one:
https://github.com/cyrusimap/cassandane/pull/142/files#diff-2d74fb1901913e00a6db529589e7c4665a613b273867e331d1c814ad7e94df9eR299

I reckon this might be a case of "it's already fixed for user/ mailboxes, but not shared ones specifically" -- Cyrus master passes all the other Flags tests, even with the updates to aggressively check for the expected untagged responses, suggesting that it is shared mailboxes specifically that are the problem here.

I'm not sure whether this means the modseq is not being bumped, or if it's just the untagged response that's missing. I think I thought the two were inherently related but I don't really remember. What do you think?

I guess we might have two things here, and one is already fixed on master, the other not. Locally merging this PR on top of master allows the new test to pass, but we don't wanna bump the modseq twice... guess the tests should also check for that!

[tpayen]

Hi @elliefm @brong is it possible to find a way to move forward on this PR?
We are currently using this patch on production on our 10 servers and 100K users since 1 year now and it's fixing well the issue. And we haven't seen any performance problem since.
As the issue is stil open, I think this is a major problem for people using sharedmailboxes with Thunderbird. So it could be nice to find a way to fix it with this patch or an other.

[elliefm]

Thanks for the ping, I've put this onto the agenda to discuss next time we meet. Usually that would be next week, but I'm not currently certain if next week's call is going ahead, it might not be until the week after.

[brong]

Since this fixes things reasonably in 3.2, I'm going to just go ahead and merge it with apologies for taking so long.