[PATCH 3/2] pop3: fix numerous bugs in delete handling

[Eric Wong <e@80x24.org>]

Eric Wong <e@80x24.org> 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