From 0089f49c4af7abb7634408e402096423a22d0462 Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Sat, 23 Apr 2022 14:20:35 +0200 Subject: [PATCH] fix expiration when syncing only new messages this was broken by commit de6dc699 - we now iterated only over far-side messages, which we don't necessarily load, unlike the near-side ones (which we need to do to know their current importance). fix by iterating over sync entries instead of messages, which basically restores the pre-19128f15 state in that regard. the minor catch here is that we now need an auxiliary array to sort the sync entries by UIDs. on the upside, we can also use it to avoid repeated calculation of the flags. --- src/run-tests.pl | 11 +++++ src/sync.c | 117 ++++++++++++++++++++++++----------------------- 2 files changed, 72 insertions(+), 56 deletions(-) diff --git a/src/run-tests.pl b/src/run-tests.pl index 796ad55..3cefa73 100755 --- a/src/run-tests.pl +++ b/src/run-tests.pl @@ -1235,6 +1235,17 @@ my @X33 = ( ); test("max messages + expire - full", \@x33, \@X33, \@O31); +my @O34 = ("", "", "Sync New\nMaxMessages 3\n"); +my @X34 = ( + I, F, I, + B, "", "+~", "+T", + E, "", "+~", "+T", + F, "", "+~", "+T", + H, "", "*", "*", + I, "", "*", "*", +); +test("max messages + expire - new", \@x33, \@X34, \@O34); + my @x38 = ( F, C, 0, A, "*FS", "*FS", "*S", diff --git a/src/sync.c b/src/sync.c index 05a6f14..40713d4 100644 --- a/src/sync.c +++ b/src/sync.c @@ -864,6 +864,21 @@ load_box( sync_vars_t *svars, int t, uint minwuid, uint_array_t mexcs ) svars->drv[t]->load_box( svars->ctx[t], minwuid, maxwuid, svars->finduid[t], pairuid, svars->maxuid[t], mexcs, box_loaded, AUX ); } +typedef struct { + sync_rec_t *srec; + uchar flags; +} alive_srec_t; + +static int +cmp_srec_far( const void *a, const void *b ) +{ + uint au = (*(const alive_srec_t *)a).srec->uid[F]; + uint bu = (*(const alive_srec_t *)b).srec->uid[F]; + assert( au && bu ); + assert( au != bu ); + return au > bu ? 1 : -1; // Can't subtract, the result might not fit into signed int. +} + typedef struct { void *aux; sync_rec_t *srec; @@ -883,7 +898,7 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux sync_rec_t *srec, **srecmap; message_t *tmsg; flag_vars_t *fv; - int no[2], del[2], alive, todel; + int no[2], del[2]; uchar sflags, nflags, aflags, dflags; uint hashsz, idx; @@ -1238,79 +1253,68 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux /* Expire excess messages. Important (flagged, unread, or unpropagated) messages * older than the first not expired message are not counted towards the total. */ debug( "preparing message expiration\n" ); - // Due to looping only over the far side, we completely ignore unpaired - // near-side messages. This is correct, as we cannot expire them without - // data loss anyway; consequently, we also don't count them. - // Note that we also ignore near-side messages we're currently propagating, - // which delays expiration of some messages by one cycle. Otherwise, we'd have - // to sequence flag propagation after message propagation to avoid a race - // with 3rd-party expunging, and that seems unreasonably expensive. - alive = 0; - for (tmsg = svars->msgs[F]; tmsg; tmsg = tmsg->next) { - if (tmsg->status & M_DEAD) + alive_srec_t *arecs = nfmalloc( sizeof(*arecs) * svars->nsrecs ); + int alive = 0; + for (srec = svars->srecs; srec; srec = srec->next) { + if (srec->status & S_DEAD) continue; - // We ignore unpaired far-side messages, as there is obviously nothing - // to expire in the first place. - if (!(srec = tmsg->srec)) - continue; - if (!(srec->status & S_PENDING)) { - if (!srec->msg[N]) - continue; // Already expired or skipped. - nflags = (srec->msg[N]->flags | srec->aflags[N]) & ~srec->dflags[N]; - } else { - nflags = tmsg->flags; - } - if (!(nflags & F_DELETED) || (srec->status & (S_EXPIRE | S_EXPIRED))) { - // The message is not deleted, or it is, but only due to being expired. - alive++; - } - } - todel = alive - svars->chan->max_messages; - debug( "%d alive messages, %d excess - expiring\n", alive, todel ); - alive = 0; - for (tmsg = svars->msgs[F]; tmsg; tmsg = tmsg->next) { - if (tmsg->status & M_DEAD) - continue; - if (!(srec = tmsg->srec)) + // We completely ignore unpaired near-side messages, as we cannot expire + // them without data loss; consequently, we also don't count them. + // Note that we also ignore near-side messages we're currently propagating, + // which delays expiration of some messages by one cycle. Otherwise, we'd + // have to sequence flag updating after message propagation to avoid a race + // with external expunging, and that seems unreasonably expensive. + if (!srec->uid[F]) continue; if (!(srec->status & S_PENDING)) { + // We ignore unpaired far-side messages, as there is obviously nothing + // to expire in the first place. if (!srec->msg[N]) continue; nflags = (srec->msg[N]->flags | srec->aflags[N]) & ~srec->dflags[N]; } else { - nflags = tmsg->flags; + nflags = srec->msg[F]->flags; } - if (!(nflags & F_DELETED) || (srec->status & (S_EXPIRE|S_EXPIRED))) { - if ((nflags & F_FLAGGED) || - !((nflags & F_SEEN) || ((void)(todel > 0 && alive++), svars->chan->expire_unread > 0))) { - // Important messages are always fetched/kept. - debug( " pair(%u,%u) is important\n", srec->uid[F], srec->uid[N] ); - todel--; - } else if (todel > 0 || - ((srec->status & (S_EXPIRE|S_EXPIRED)) == (S_EXPIRE|S_EXPIRED)) || - ((srec->status & (S_EXPIRE|S_EXPIRED)) && (srec->msg[N]->flags & F_DELETED))) { - /* The message is excess or was already (being) expired. */ - srec->status |= S_NEXPIRE; - debug( " expiring pair(%u,%u)\n", srec->uid[F], srec->uid[N] ); - todel--; - } + if (!(nflags & F_DELETED) || (srec->status & (S_EXPIRE | S_EXPIRED))) { + // The message is not deleted, or it is, but only due to being expired. + arecs[alive++] = (alive_srec_t){ srec, nflags }; + } + } + // Sort such that the messages which have been in the + // complete store longest expire first. + qsort( arecs, alive, sizeof(*arecs), cmp_srec_far ); + int todel = alive - svars->chan->max_messages; + debug( "%d alive messages, %d excess - expiring\n", alive, todel ); + int unseen = 0; + for (int sri = 0; sri < alive; sri++) { + srec = arecs[sri].srec; + nflags = arecs[sri].flags; + if ((nflags & F_FLAGGED) || + !((nflags & F_SEEN) || ((void)(todel > 0 && unseen++), svars->chan->expire_unread > 0))) { + // Important messages are always fetched/kept. + debug( " pair(%u,%u) is important\n", srec->uid[F], srec->uid[N] ); + todel--; + } else if (todel > 0 || + ((srec->status & (S_EXPIRE | S_EXPIRED)) == (S_EXPIRE | S_EXPIRED)) || + ((srec->status & (S_EXPIRE | S_EXPIRED)) && (srec->msg[N]->flags & F_DELETED))) { + /* The message is excess or was already (being) expired. */ + srec->status |= S_NEXPIRE; + debug( " expiring pair(%u,%u)\n", srec->uid[F], srec->uid[N] ); + todel--; } } debug( "%d excess messages remain\n", todel ); - if (svars->chan->expire_unread < 0 && alive * 2 > svars->chan->max_messages) { + if (svars->chan->expire_unread < 0 && unseen * 2 > svars->chan->max_messages) { error( "%s: %d unread messages in excess of MaxMessages (%d).\n" "Please set ExpireUnread to decide outcome. Skipping mailbox.\n", - svars->orig_name[N], alive, svars->chan->max_messages ); + svars->orig_name[N], unseen, svars->chan->max_messages ); svars->ret |= SYNC_FAIL; cancel_sync( svars ); return; } - for (srec = svars->srecs; srec; srec = srec->next) { - if (srec->status & S_DEAD) - continue; + for (int sri = 0; sri < alive; sri++) { + srec = arecs[sri].srec; if (!(srec->status & S_PENDING)) { - if (!srec->msg[N]) - continue; uchar nex = (srec->status / S_NEXPIRE) & 1; if (nex != ((srec->status / S_EXPIRED) & 1)) { /* The record needs a state change ... */ @@ -1340,6 +1344,7 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux } } } + free( arecs ); } sync_ref( svars );