From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.2 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id B3D411F54E for ; Wed, 20 Jul 2022 07:27:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=80x24.org; s=selector1; t=1658302068; bh=3Qz8o68v1hmJ1nnvW63c7tbNh5ass2MCY9fIcrT5LQE=; h=Date:From:To:Subject:References:In-Reply-To:From; b=Uwmo1FFVf3tVh/U3Kai/ZKQqRldnmaRjdkEbYjYD6uS7j6BE3up5HkWgmT2Fs6nYT /gG8J9otJq1GA/6wXTB0TJvrlWBMU+QfsUyIbFUA6DreXmKNaYjzi4HpaU9ClPB6Uh ua8NOl/GRBXS/SYB7qUP++ENgVsnfdc9l3hoxek8= Date: Wed, 20 Jul 2022 07:27:48 +0000 From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 3/2] pop3: fix numerous bugs in delete handling Message-ID: <20220720072748.GA10401@dcvr> References: <20220719024950.1831808-1-e@80x24.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220719024950.1831808-1-e@80x24.org> List-Id: Eric Wong wrote: > The on-disk storage aspect still has me a little nervous :x I think this needs to be squashed into [PATCH 1/2] to avoid migration costs (probably nobody but me is running this, yet): ------8<------- Subject: [PATCH] pop3: fix numerous bugs in delete handling This requires a DB change to simplify our Perl code, but there's no migration for it presently. We were also incorrectly relying on array offsets instead of sequence numbers for "EXPIRE 0" handling. --- lib/PublicInbox/POP3.pm | 4 ++-- lib/PublicInbox/POP3D.pm | 2 +- t/pop3d.t | 37 +++++++++++++++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lib/PublicInbox/POP3.pm b/lib/PublicInbox/POP3.pm index 5cffc556..51c2b71a 100644 --- a/lib/PublicInbox/POP3.pm +++ b/lib/PublicInbox/POP3.pm @@ -218,7 +218,7 @@ sub need_txn ($) { sub _stat_cache ($) { my ($self) = @_; - my ($beg, $end) = ($self->{uid_dele} // 0, $self->{uid_max}); + my ($beg, $end) = (($self->{uid_dele} // -1) + 1, $self->{uid_max}); PublicInbox::IMAP::uid_clamp($self, \$beg, \$end); my $opt = { limit => PublicInbox::IMAP::UID_SLICE }; my $m = $self->{ibx}->over(1)->do_get(<<'', $opt, $beg, $end); @@ -305,7 +305,7 @@ sub retr_cb { # called by git->cat_async via ibx_async_cat $$bref .= substr($$bref, -2, 2) eq "\r\n" ? ".\r\n" : "\r\n.\r\n"; $self->msg_more("+OK message follows\r\n"); $self->write($bref); - $self->{expire} .= pack('S', $off) if exists $self->{expire}; + $self->{expire} .= pack('S', $off + 1) if exists $self->{expire}; $self->requeue; } diff --git a/lib/PublicInbox/POP3D.pm b/lib/PublicInbox/POP3D.pm index 65997f6d..c7bf1755 100644 --- a/lib/PublicInbox/POP3D.pm +++ b/lib/PublicInbox/POP3D.pm @@ -84,7 +84,7 @@ CREATE TABLE IF NOT EXISTS deletes ( txn_id INTEGER PRIMARY KEY NOT NULL, /* -1 == txn lock offset */ user_id INTEGER NOT NULL REFERENCES users, mailbox_id INTEGER NOT NULL REFERENCES mailboxes, - uid_dele INTEGER NULL, /* IMAP UID, NNTP article number */ + uid_dele INTEGER NOT NULL DEFAULT -1, /* IMAP UID, NNTP article */ UNIQUE(user_id, mailbox_id) ) } diff --git a/t/pop3d.t b/t/pop3d.t index e781987e..d5ccb0d8 100644 --- a/t/pop3d.t +++ b/t/pop3d.t @@ -54,10 +54,10 @@ unless (-r $key && -r $cert) { my $old = start_script(['-pop3d', '-W0', "--stdout=$tmpdir/plain.out", "--stderr=$olderr" ], $env, { 3 => $plain }); -my $oldc = Net::POP3->new($plain->sockhost, Port => $plain->sockport); +my @old_args = ($plain->sockhost, Port => $plain->sockport); +my $oldc = Net::POP3->new(@old_args); my $locked_mb = ('e'x32)."\@$group"; ok($oldc->apop("$locked_mb.0", 'anonymous'), 'APOP to old'); -my @old_args = ($plain->sockhost, Port => $plain->sockport); { # locking within the same process my $x = Net::POP3->new(@old_args); @@ -238,6 +238,39 @@ EOF { my $capa = $oldc->capa; ok(defined($capa->{PIPELINING}), 'pipelining supported by CAPA'); + is($capa->{EXPIRE}, 0, 'EXPIRE 0 set'); + + # clients which see "EXPIRE 0" can elide DELE requests + my $list = $oldc->list; + ok($oldc->get($_), "RETR $_") for keys %$list; + ok($oldc->quit, 'QUIT after RETR'); + + $oldc = Net::POP3->new(@old_args); + ok($oldc->apop("$locked_mb.0", 'anonymous'), 'APOP reconnect'); + my $cont = $oldc->list; + is_deeply($cont, {}, 'no messages after implicit DELE from EXPIRE 0'); + ok($oldc->quit, 'QUIT on noop'); + + # test w/o checking CAPA to trigger EXPIRE 0 + $oldc = Net::POP3->new(@old_args); + ok($oldc->apop($locked_mb, 'anonymous'), 'APOP on latest slice'); + my $l2 = $oldc->list; + is_deeply($l2, $list, 'different mailbox, different deletes'); + ok($oldc->get($_), "RETR $_") for keys %$list; + ok($oldc->quit, 'QUIT w/o EXPIRE nor DELE'); + + $oldc = Net::POP3->new(@old_args); + ok($oldc->apop($locked_mb, 'anonymous'), 'APOP again on latest'); + $l2 = $oldc->list; + is_deeply($l2, $list, 'no DELE nor EXPIRE preserves messages'); + ok($oldc->delete(2), 'explicit DELE on latest'); + ok($oldc->quit, 'QUIT w/ highest DELE'); + + # this is non-standard behavior, but necessary if we expect hundreds + # of thousands of users on cheap HW + $oldc = Net::POP3->new(@old_args); + ok($oldc->apop($locked_mb, 'anonymous'), 'APOP yet again on latest'); + is_deeply($oldc->list, {}, 'highest DELE deletes older messages, too'); } # TODO: more tests, but mpop was really helpful in helping me