Another pile of things found while working on better synchronization. Eric Wong (10): lei_mail_sync: forget_folder: simplify code lei prune-mail-sync: handle --all (no args) lei_mail_sync: simplify group2folders lei_mail_sync: make rename_folder more robust t/lei-watch: avoid race between glob + readlink lei note-event: always flush changes on daemon exit lei: refresh watches before MUA spawn for Maildir lei_mail_sync: set_src uses binary OIDs lei: fix error reporting from lei/store -> lei-daemon lei/store: correctly delete entries from over lib/PublicInbox/LEI.pm | 4 ++++ lib/PublicInbox/LeiMailSync.pm | 36 ++++++++++++++++++++++------------ lib/PublicInbox/LeiStore.pm | 8 +++++--- lib/PublicInbox/LeiStoreErr.pm | 14 +++++++++++-- t/lei-watch.t | 2 +- t/lei_mail_sync.t | 15 +++++++------- 6 files changed, 53 insertions(+), 26 deletions(-)
No need to bump refcounts of {dbh} nor declare extra variables for a rarely-called function. --- lib/PublicInbox/LeiMailSync.pm | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm index b63f8dea..57b56b3c 100644 --- a/lib/PublicInbox/LeiMailSync.pm +++ b/lib/PublicInbox/LeiMailSync.pm @@ -403,13 +403,11 @@ EOF sub forget_folder { my ($self, $folder) = @_; - my ($fid, $sth); - $fid = delete($self->{fmap}->{$folder}) // + my $fid = delete($self->{fmap}->{$folder}) // fid_for($self, $folder) // return; - my $dbh = $self->{dbh}; - $dbh->do('DELETE FROM blob2name WHERE fid = ?', undef, $fid); - $dbh->do('DELETE FROM blob2num WHERE fid = ?', undef, $fid); - $dbh->do('DELETE FROM folders WHERE fid = ?', undef, $fid); + for my $t (qw(blob2name blob2num folders)) { + $self->{dbh}->do("DELETE FROM $t WHERE fid = ?", undef, $fid); + } } # only used for changing canonicalization errors
This still needs tests, but I noticed "--all" w/o "local" or "remote" was not working correctly since split() returned an empty array. --- lib/PublicInbox/LeiMailSync.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm index 57b56b3c..bf8fb7de 100644 --- a/lib/PublicInbox/LeiMailSync.pm +++ b/lib/PublicInbox/LeiMailSync.pm @@ -330,6 +330,7 @@ sub group2folders { EOM my %x = map { $_ => $_ } split(/,/, $all); my @ok = grep(defined, delete(@x{qw(local remote), ''})); + push(@ok, '') if $all eq ''; my @no = keys %x; if (@no) { @no = (join(',', @no));
No need to loop when we can rely on grep. --- lib/PublicInbox/LeiMailSync.pm | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm index bf8fb7de..56468c78 100644 --- a/lib/PublicInbox/LeiMailSync.pm +++ b/lib/PublicInbox/LeiMailSync.pm @@ -350,9 +350,7 @@ EOM } else { @inc = @all; } - for (@inc) { - push(@$folders, $_) unless $seen{$_}++; - } + push(@$folders, (grep { !$seen{$_}++ } @inc)); } scalar(@$folders) || $lei->fail(<<EOM); no --mail-sync folders known to lei
We need to account for past canonicalization errors and deal with cases which violate uniqueness constraints in mail_sync.sqlite3 --- lib/PublicInbox/LeiMailSync.pm | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm index 56468c78..275e0cc4 100644 --- a/lib/PublicInbox/LeiMailSync.pm +++ b/lib/PublicInbox/LeiMailSync.pm @@ -412,11 +412,24 @@ sub forget_folder { # only used for changing canonicalization errors sub rename_folder { my ($self, $old, $new) = @_; - my $fid = delete($self->{fmap}->{$old}) // + my $ofid = delete($self->{fmap}->{$old}) // fid_for($self, $old) // return; - $self->{dbh}->do(<<EOM, undef, $new, $fid); + eval { + $self->{dbh}->do(<<EOM, undef, $new, $ofid); UPDATE folders SET loc = ? WHERE fid = ? EOM + }; + if ($@ =~ /\bunique\b/i) { + my $nfid = $self->{fmap}->{$new} // fid_for($self, $new); + for my $t (qw(blob2name blob2num)) { + $self->{dbh}->do(<<EOM, undef, $nfid, $ofid); +UPDATE OR REPLACE $t SET fid = ? WHERE fid = ? +EOM + } + $self->{dbh}->do(<<EOM, undef, $ofid); +DELETE FROM folders WHERE fid = ? +EOM + } } sub imap_oidbin ($$$) {
Open file handles in lei-daemon may be unstable so we need to account for readlink() returning undef. --- t/lei-watch.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/lei-watch.t b/t/lei-watch.t index 86fa6649..a881fbb9 100644 --- a/t/lei-watch.t +++ b/t/lei-watch.t @@ -25,7 +25,7 @@ test_lei(sub { lei_ok 'daemon-pid'; chomp(my $pid = $lei_out); skip 'missing /proc/$PID/fd', 1 if !-d "/proc/$pid/fd"; my @ino = grep { - readlink($_) =~ /\binotify\b/ + (readlink($_) // '') =~ /\binotify\b/ } glob("/proc/$pid/fd/*"); is(scalar(@ino), 1, 'only one inotify FD'); my $ino_fd = (split('/', $ino[0]))[-1];
Because the timer may not fire in time before daemon shutdown. --- lib/PublicInbox/LEI.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index 28fe0c83..520fb519 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -1254,6 +1254,7 @@ sub lazy_start { my (undef, $eof_p) = PublicInbox::PktOp->pair; sub { $exit_code //= shift; + eval 'PublicInbox::LeiNoteEvent::flush_task()'; my $lis = $pil or exit($exit_code); # closing eof_p triggers \&noop wakeup $listener = $eof_p = $pil = $path = undef;
If we possibly just wrote or created a Maildir, ensure it's monitored by the lei watch mechanism. --- lib/PublicInbox/LEI.pm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index 520fb519..41e811ca 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -951,6 +951,9 @@ sub exec_buf ($$) { sub start_mua { my ($self) = @_; + if ($self->{ovv}->{fmt} =~ /\A(?:maildir)\z/) { # TODO: IMAP + refresh_watches($self); + } my $mua = $self->{opt}->{mua} // return; my $mfolder = $self->{ovv}->{dst}; my (@cmd, $replaced);
Another step towards moving more of our internals to use binary OIDs to avoid needless conversions before hitting disk. --- lib/PublicInbox/LeiMailSync.pm | 4 ++-- lib/PublicInbox/LeiStore.pm | 2 +- t/lei_mail_sync.t | 15 ++++++++------- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm index 275e0cc4..f8834a27 100644 --- a/lib/PublicInbox/LeiMailSync.pm +++ b/lib/PublicInbox/LeiMailSync.pm @@ -118,7 +118,7 @@ EOM } sub set_src { - my ($self, $oidhex, $folder, $id) = @_; + my ($self, $oidbin, $folder, $id) = @_; my $fid = $self->{fmap}->{$folder} //= fid_for($self, $folder, 1); my $sth; if (ref($id)) { # scalar name @@ -131,7 +131,7 @@ INSERT OR IGNORE INTO blob2name (oidbin, fid, name) VALUES (?, ?, ?) INSERT OR IGNORE INTO blob2num (oidbin, fid, uid) VALUES (?, ?, ?) } - $sth->execute(pack('H*', $oidhex), $fid, $id); + $sth->execute($oidbin, $fid, $id); } sub clear_src { diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm index 28e36e89..5a48c064 100644 --- a/lib/PublicInbox/LeiStore.pm +++ b/lib/PublicInbox/LeiStore.pm @@ -229,7 +229,7 @@ sub lms_rename_folder { sub set_sync_info { my ($self, $oidhex, $folder, $id) = @_; - _lms_rw($self)->set_src($oidhex, $folder, $id); + _lms_rw($self)->set_src(pack('H*', $oidhex), $folder, $id); } sub _remove_if_local { # git->cat_async arg diff --git a/t/lei_mail_sync.t b/t/lei_mail_sync.t index a5e5f5d3..5daa49cd 100644 --- a/t/lei_mail_sync.t +++ b/t/lei_mail_sync.t @@ -16,24 +16,25 @@ is_deeply([$ro->folders], [], 'no folders, yet'); my $imap = 'imaps://bob@[::1]/INBOX;UIDVALIDITY=9'; $lms->lms_begin; -is($lms->set_src('deadbeef', $imap, 1), 1, 'set IMAP once'); -ok($lms->set_src('deadbeef', $imap, 1) == 0, 'set IMAP idempotently'); +my $deadbeef = "\xde\xad\xbe\xef"; +is($lms->set_src($deadbeef, $imap, 1), 1, 'set IMAP once'); +ok($lms->set_src($deadbeef, $imap, 1) == 0, 'set IMAP idempotently'); $lms->lms_commit; is_deeply([$ro->folders], [$imap], 'IMAP folder added'); is_deeply([$ro->folders($imap)], [$imap], 'IMAP folder with full GLOB'); is_deeply([$ro->folders('imaps://bob@[::1]/INBOX')], [$imap], 'IMAP folder with partial GLOB'); -is_deeply($ro->locations_for("\xde\xad\xbe\xef"), +is_deeply($ro->locations_for($deadbeef), { $imap => [ 1 ] }, 'locations_for w/ imap'); my $maildir = 'maildir:/home/user/md'; my $fname = 'foo:2,S'; $lms->lms_begin; -ok($lms->set_src('deadbeef', $maildir, \$fname), 'set Maildir once'); -ok($lms->set_src('deadbeef', $maildir, \$fname) == 0, 'set Maildir again'); +ok($lms->set_src($deadbeef, $maildir, \$fname), 'set Maildir once'); +ok($lms->set_src($deadbeef, $maildir, \$fname) == 0, 'set Maildir again'); $lms->lms_commit; -is_deeply($ro->locations_for("\xde\xad\xbe\xef"), +is_deeply($ro->locations_for($deadbeef), { $imap => [ 1 ], $maildir => [ $fname ] }, 'locations_for w/ maildir + imap'); @@ -45,7 +46,7 @@ if ('mess things up pretend old bug') { $lms->lms_commit; $lms->lms_begin; - ok($lms->set_src('deadbeef', $maildir, \$fname), 'set Maildir once'); + ok($lms->set_src($deadbeef, $maildir, \$fname), 'set Maildir once'); $lms->lms_commit; };
We must set autoflush to ensure timely notification of clients; and lei-daemon must not block when waiting on reads in case of spurious wakeups. Finally, if no clients are connected to lei-daemon, write to syslog to ensure the error is visible. --- lib/PublicInbox/LeiStore.pm | 2 ++ lib/PublicInbox/LeiStoreErr.pm | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm index 5a48c064..0652137e 100644 --- a/lib/PublicInbox/LeiStore.pm +++ b/lib/PublicInbox/LeiStore.pm @@ -28,6 +28,7 @@ use PublicInbox::Spawn qw(spawn); use List::Util qw(max); use File::Temp (); use POSIX (); +use IO::Handle (); # ->autoflush sub new { my (undef, $dir, $opt) = @_; @@ -514,6 +515,7 @@ sub write_prepare { $self->ipc_lock_init("$dir/ipc.lock"); substr($dir, -length('/lei/store'), 10, ''); pipe(my ($r, $w)) or die "pipe: $!"; + $w->autoflush(1); # Mail we import into lei are private, so headers filtered out # by -mda for public mail are not appropriate local @PublicInbox::MDA::BAD_HEADERS = (); diff --git a/lib/PublicInbox/LeiStoreErr.pm b/lib/PublicInbox/LeiStoreErr.pm index 68ce96d6..5f9ba24d 100644 --- a/lib/PublicInbox/LeiStoreErr.pm +++ b/lib/PublicInbox/LeiStoreErr.pm @@ -8,22 +8,32 @@ use strict; use v5.10.1; use parent qw(PublicInbox::DS); use PublicInbox::Syscall qw(EPOLLIN EPOLLONESHOT); +use Sys::Syslog qw(openlog syslog closelog); +use IO::Handle (); # ->blocking sub new { my ($cls, $rd, $lei) = @_; my $self = bless { sock => $rd, store_path => $lei->store_path }, $cls; + $rd->blocking(0); $self->SUPER::new($rd, EPOLLIN | EPOLLONESHOT); } sub event_step { my ($self) = @_; - $self->do_read(\(my $rbuf), 4096) or return; + my $rbuf = $self->{rbuf} // \(my $x = ''); + $self->do_read($rbuf, 8192, length($$rbuf)) or return; my $cb; + my $printed; for my $lei (values %PublicInbox::DS::DescriptorMap) { $cb = $lei->can('store_path') // next; next if $cb->($lei) ne $self->{store_path}; my $err = $lei->{2} // next; - print $err $rbuf; + print $err $$rbuf and $printed = 1; + } + if (!$printed) { + openlog('lei-store', 'pid,nowait,nofatal,ndelay', 'user'); + for my $l (split(/\n/, $$rbuf)) { syslog('warning', '%s', $l) } + closelog(); # don't share across fork } }
Some of these errors were inadvertantly lost due to delayed error reporting in the past. --- lib/PublicInbox/LeiStore.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm index 0652137e..ab39043e 100644 --- a/lib/PublicInbox/LeiStore.pm +++ b/lib/PublicInbox/LeiStore.pm @@ -243,8 +243,8 @@ sub remove_docids ($;@) { my $eidx = eidx_init($self); for my $docid (@docids) { $eidx->idx_shard($docid)->ipc_do('xdb_remove', $docid); - $self->{oidx}->delete_by_num($docid); - $self->{oidx}->{dbh}->do(<<EOF, undef, $docid); + $eidx->{oidx}->delete_by_num($docid); + $eidx->{oidx}->{dbh}->do(<<EOF, undef, $docid); DELETE FROM xref3 WHERE docid = ? EOF }