[PATCH 00/15] use RENAME_NOREPLACE on Linux 3.15+

[PATCH 00/15] use RENAME_NOREPLACE on Linux 3.15+

[Eric Wong]

What started off as a trivial, "something-to-do-before-bedtime"
change several days ago (15/15) ended up forcing me to fix
various hard-to-reproduce race conditions around Maildirs.

Eventually, `schedtool -a 0x1 -e ...' helped me reproduce
some races more easily (but still not with 100% reliability).

t/lei-{watch,auto-watch,export-kw}.t should all be more
reliable, now.

There's also several other minor fixes I found along the way...

Eric Wong (15):
  t/lei-{auto-watch,export-kw}: extra diagnostics on failure
  t/lei-import-maildir: rename fix (SR -> RS)
  t/lei-p2q: extra diagnostics
  lei/store: check for any unexpected process death
  lei note-event: drop unnecessary eval guard
  lei note-event: wq_io_do => wq_do
  lei_search: try harder to associate "lei index"-ed messages
  watch: check for {quit} before IDLE
  watch: remove redundant signal mask manipulation
  doc: lei-overview: add CAVEATS section
  lei note-event: clear_src on ENOENT
  dir_idle: treat IN_MOVED_FROM as a gone event
  lei: no Perl FileHandle for `undef' w/ ECONNRESET
  lei_mail_sync: mv_src: use transaction, check UNIQUE
  lei: use RENAME_NOREPLACE on Linux 3.15+

 Documentation/lei-overview.pod  |  5 ++++
 MANIFEST                        |  1 +
 devel/syscall-list              |  8 +++++-
 lib/PublicInbox/DirIdle.pm      |  3 +-
 lib/PublicInbox/FakeInotify.pm  |  3 ++
 lib/PublicInbox/LEI.pm          |  3 +-
 lib/PublicInbox/LeiExportKw.pm  | 19 ++++---------
 lib/PublicInbox/LeiMailSync.pm  |  8 ++++--
 lib/PublicInbox/LeiNoteEvent.pm | 13 +++++----
 lib/PublicInbox/LeiSearch.pm    | 13 ++++++++-
 lib/PublicInbox/LeiStore.pm     | 16 +++++++----
 lib/PublicInbox/LeiToMail.pm    |  7 ++---
 lib/PublicInbox/Syscall.pm      | 49 +++++++++++++++++++++++++++++++--
 lib/PublicInbox/Watch.pm        | 13 +++------
 t/lei-auto-watch.t              |  3 +-
 t/lei-export-kw.t               | 39 ++++++++++++++++++--------
 t/lei-import-maildir.t          |  2 +-
 t/lei-p2q.t                     |  2 +-
 t/rename_noreplace.t            | 26 +++++++++++++++++
 19 files changed, 173 insertions(+), 60 deletions(-)
 create mode 100644 t/rename_noreplace.t

[PATCH 01/15] t/lei-{auto-watch,export-kw}: extra diagnostics on failure

[Eric Wong]

Maybe these will help track down some failures and make
diagnosing bugs easier.  "lei export-kw" should also become
optional, even, so allow disabling it easily in the test.
---
 t/lei-auto-watch.t |  3 ++-
 t/lei-export-kw.t  | 39 ++++++++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/t/lei-auto-watch.t b/t/lei-auto-watch.t
index e5e132eb3bfd..d5661ae5d618 100644
--- a/t/lei-auto-watch.t
+++ b/t/lei-auto-watch.t
@@ -41,7 +41,8 @@ test_lei(sub {
 	$ins = json_utf8->decode($lei_out);
 	$exp = { "maildir:$x" => [ map { basename($_) } glob("$x/*/*") ],
 		"maildir:$y" => [ map { basename($_) } glob("$y/*/*") ] };
-	is_deeply($ins->{'mail-sync'}, $exp, 'mail_sync matches FS');
+	is_deeply($ins->{'mail-sync'}, $exp, 'mail_sync matches FS') or
+		diag explain($ins);
 });
 
 done_testing;
diff --git a/t/lei-export-kw.t b/t/lei-export-kw.t
index 1fe940bb6d89..55730e87c050 100644
--- a/t/lei-export-kw.t
+++ b/t/lei-export-kw.t
@@ -7,28 +7,45 @@ use File::Path qw(make_path);
 require_mods(qw(lei -imapd Mail::IMAPClient));
 my ($tmpdir, $for_destroy) = tmpdir;
 my $expect = eml_load('t/data/0001.patch');
+my $do_export_kw = 1;
+my $wait_for = sub {
+	my ($f) = @_;
+	lei_ok qw(export-kw --all=local) if $do_export_kw;
+	my $x = $f;
+	$x =~ s!\Q$tmpdir\E/!\$TMPDIR/!;
+	for (0..10) {
+		last if -f $f;
+		diag "tick #$_ $x";
+		tick(0.1);
+	}
+	ok(-f $f, "$x exists") or xbail;
+};
+
 test_lei({ tmpdir => $tmpdir }, sub {
 	my $home = $ENV{HOME};
 	my $md = "$home/md";
+	my $f;
 	make_path("$md/new", "$md/cur", "$md/tmp");
 	cp('t/data/0001.patch', "$md/new/y") or xbail "cp $md $!";
 	cp('t/data/message_embed.eml', "$md/cur/x:2,S") or xbail "cp $md $!";
-	lei_ok qw(index -q), $md;
+	lei_ok qw(index), $md;
 	lei_ok qw(tag t/data/0001.patch +kw:seen);
-	lei_ok qw(export-kw --all=local);
-	ok(!-e "$md/new/y", 'original gone');
-	is_deeply(eml_load("$md/cur/y:2,S"), $expect,
-		"`seen' kw exported");
+	$wait_for->($f = "$md/cur/y:2,S");
+	ok(!-e "$md/new/y", 'original gone') or
+		diag explain([glob("$md/*/*")]);
+	is_deeply(eml_load($f), $expect, "`seen' kw exported");
 
 	lei_ok qw(tag t/data/0001.patch +kw:answered);
-	lei_ok qw(export-kw --all=local);
-	ok(!-e "$md/cur/y:2,S", 'seen-only file gone');
-	is_deeply(eml_load("$md/cur/y:2,RS"), $expect, "`R' added");
+	$wait_for->($f = "$md/cur/y:2,RS");
+	ok(!-e "$md/cur/y:2,S", 'seen-only file gone') or
+		diag explain([glob("$md/*/*")]);
+	is_deeply(eml_load($f), $expect, "`R' added");
 
 	lei_ok qw(tag t/data/0001.patch -kw:answered -kw:seen);
-	lei_ok qw(export-kw --mode=set --all=local);
-	ok(!-e "$md/cur/y:2,RS", 'seen+answered file gone');
-	is_deeply(eml_load("$md/cur/y:2,"), $expect, 'no keywords left');
+	$wait_for->($f = "$md/cur/y:2,");
+	ok(!-e "$md/cur/y:2,RS", 'seen+answered file gone') or
+		diag explain([glob("$md/*/*")]);
+	is_deeply(eml_load($f), $expect, 'no keywords left');
 });
 
 done_testing;

[PATCH 02/15] t/lei-import-maildir: rename fix (SR -> RS)

[Eric Wong]

While it doesn't matter to us, the Maildir spec specifies
characters are to be sorted in alphabetical order.
---
 t/lei-import-maildir.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lei-import-maildir.t b/t/lei-import-maildir.t
index c81e7805fe7f..1e7eddd571d8 100644
--- a/t/lei-import-maildir.t
+++ b/t/lei-import-maildir.t
@@ -52,7 +52,7 @@ test_lei(sub {
 	my $r2 = json_utf8->decode($lei_out);
 	is_deeply($r2, $res, 'idempotent import')
 			or diag explain($imp_err, $res);
-	rename("$md/cur/x:2,S", "$md/cur/x:2,SR") or BAIL_OUT "rename: $!";
+	rename("$md/cur/x:2,S", "$md/cur/x:2,RS") or BAIL_OUT "rename: $!";
 	lei_ok('import', "maildir:$md", \'import Maildir after +answered');
 	lei_ok(qw(q -d none s:boolean), \'lei q after +answered');
 	$res = json_utf8->decode($lei_out);

[PATCH 03/15] t/lei-p2q: extra diagnostics

[Eric Wong]

I got one mysterious test failure here, once, and can't seem
to reproduce it...
---
 t/lei-p2q.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lei-p2q.t b/t/lei-p2q.t
index 495d81de78a5..bf40a43be466 100644
--- a/t/lei-p2q.t
+++ b/t/lei-p2q.t
@@ -7,7 +7,7 @@ require_mods(qw(json DBD::SQLite Search::Xapian));
 
 test_lei(sub {
 	ok(!lei(qw(p2q this-better-cause-format-patch-to-fail)),
-		'p2q fails on bogus arg');
+		'p2q fails on bogus arg') or diag $lei_err;
 	like($lei_err, qr/format-patch.*failed/, 'notes format-patch failure');
 	lei_ok(qw(p2q -w dfpost t/data/0001.patch));
 	is($lei_out, "dfpost:6e006fd73b1d\n", 'pathname') or diag $lei_err;

[PATCH 04/15] lei/store: check for any unexpected process death

[Eric Wong]

The lei/store process should only exit from EOF on the
socket, so make sure we note any unintended signals
---
 lib/PublicInbox/LeiStore.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm
index 821045701dfe..16e7d302dc2f 100644
--- a/lib/PublicInbox/LeiStore.pm
+++ b/lib/PublicInbox/LeiStore.pm
@@ -571,6 +571,12 @@ sub recv_and_run {
 	$self->SUPER::recv_and_run(@args);
 }
 
+sub _sto_atexit { # dwaitpid callback
+	my ($args, $pid) = @_;
+	my $self = $args->[0];
+	warn "lei/store PID:$pid died \$?=$?\n" if $?;
+}
+
 sub write_prepare {
 	my ($self, $lei) = @_;
 	$lei // die 'BUG: $lei not passed';
@@ -587,7 +593,7 @@ sub write_prepare {
 					-err_wr => $w,
 					to_close => [ $r ],
 				});
-		$self->wq_wait_async; # outlives $lei
+		$self->wq_wait_async(\&_sto_atexit); # outlives $lei
 		require PublicInbox::LeiStoreErr;
 		PublicInbox::LeiStoreErr->new($r, $lei);
 	}

[PATCH 05/15] lei note-event: drop unnecessary eval guard

[Eric Wong]

We don't want to lose the failure message in case note-event
fails.
---
 lib/PublicInbox/LeiNoteEvent.pm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LeiNoteEvent.pm b/lib/PublicInbox/LeiNoteEvent.pm
index 1749c98f8312..0709754f6d2a 100644
--- a/lib/PublicInbox/LeiNoteEvent.pm
+++ b/lib/PublicInbox/LeiNoteEvent.pm
@@ -72,8 +72,7 @@ sub lei_note_event {
 	my $lms = $lei->lms or return;
 	$lms->lms_write_prepare if $new_cur eq ''; # for ->clear_src below
 	$lei->{opt}->{quiet} = 1;
-	eval { $lms->arg2folder($lei, [ $folder ]) };
-	return if $@;
+	$lms->arg2folder($lei, [ $folder ]);
 	my $state = $cfg->get_1("watch.$folder.state") // 'tag-rw';
 	return if $state eq 'pause';
 	return $lms->clear_src($folder, \$bn) if $new_cur eq '';

[PATCH 06/15] lei note-event: wq_io_do => wq_do

[Eric Wong]

No need to pass extra arrayref args, here.
---
 lib/PublicInbox/LeiNoteEvent.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/LeiNoteEvent.pm b/lib/PublicInbox/LeiNoteEvent.pm
index 0709754f6d2a..3472e73070d6 100644
--- a/lib/PublicInbox/LeiNoteEvent.pm
+++ b/lib/PublicInbox/LeiNoteEvent.pm
@@ -97,7 +97,7 @@ sub lei_note_event {
 		return if index($fl, 'T') >= 0;
 		my $kw = PublicInbox::MdirReader::flags2kw($fl);
 		my $vmd = { kw => $kw, sync_info => [ $folder, \$bn ] };
-		$self->wq_io_do('maildir_event', [], $fn, $vmd, $state);
+		$self->wq_do('maildir_event', $fn, $vmd, $state);
 	} # else: TODO: imap
 }
 

[PATCH 07/15] lei_search: try harder to associate "lei index"-ed messages

[Eric Wong]

Allow checking for keyword changes if we have an known OID,
even if the blob isn't currently reachable.
---
 lib/PublicInbox/LeiSearch.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/LeiSearch.pm b/lib/PublicInbox/LeiSearch.pm
index 3e046b2146c6..1fb38da1d7aa 100644
--- a/lib/PublicInbox/LeiSearch.pm
+++ b/lib/PublicInbox/LeiSearch.pm
@@ -7,7 +7,7 @@ use strict;
 use v5.10.1;
 use parent qw(PublicInbox::ExtSearch); # PublicInbox::Search->reopen
 use PublicInbox::Search qw(xap_terms);
-use PublicInbox::ContentHash qw(content_digest content_hash);
+use PublicInbox::ContentHash qw(content_digest content_hash git_sha);
 use PublicInbox::MID qw(mids mids_for_index);
 use Carp qw(croak);
 
@@ -118,6 +118,13 @@ sub xoids_for {
 		}
 	}
 	$git->async_wait_all;
+
+	# it could be an 'lei index'-ed file that just got renamed
+	if (scalar(keys %$xoids) < ($min // 1) && defined($self->{topdir})) {
+		my $hex = git_sha(1, $eml)->hexdigest;
+		my @n = $overs[0]->blob_exists($hex);
+		for (@n) { $xoids->{$hex} //= $_ }
+	}
 	scalar(keys %$xoids) ? $xoids : undef;
 }
 
@@ -129,6 +136,10 @@ sub kw_changed {
 		my $xoids = xoids_for($self, $eml) // return;
 		$docids //= [];
 		@$docids = sort { $a <=> $b } values %$xoids;
+		if (!@$docids && $self->over) {
+			my $bin = git_sha(1, $eml)->digest;
+			@$docids = $self->over->oidbin_exists($bin);
+		}
 	}
 	for my $id (@$docids) {
 		$cur_kw = eval { msg_keywords($self, $id) } and last;

[PATCH 08/15] watch: check for {quit} before IDLE

[Eric Wong]

This may make it less likely for watch-dependent tests to get
stuck.  Unfortunately, due to the synchronous API of
Mail::IMAPClient, ->idle is still susceptible to missing
signals.
---
 lib/PublicInbox/Watch.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm
index b48d9cccc3e2..e80bbdec16fd 100644
--- a/lib/PublicInbox/Watch.pm
+++ b/lib/PublicInbox/Watch.pm
@@ -330,6 +330,7 @@ sub imap_idle_once ($$$$) {
 	my $end = now() + $intvl;
 	warn "I: $uri idling for ${intvl}s\n";
 	local $0 = "IDLE $0";
+	return if $self->{quit};
 	unless ($mic->idle) {
 		return if $self->{quit};
 		return "E: IDLE failed on $uri: $!";

[PATCH 09/15] watch: remove redundant signal mask manipulation

[Eric Wong]

The top-level daemon process already blocks all signals,
so there's no reason to block them around fork() calls.
---
 lib/PublicInbox/Watch.pm | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm
index e80bbdec16fd..3f6fe21b726f 100644
--- a/lib/PublicInbox/Watch.pm
+++ b/lib/PublicInbox/Watch.pm
@@ -391,11 +391,7 @@ sub watch_atfork_child ($) {
 	PublicInbox::DS::sig_setmask($self->{oldset});
 }
 
-sub watch_atfork_parent ($) {
-	my ($self) = @_;
-	_done_for_now($self);
-	PublicInbox::DS::block_signals();
-}
+sub watch_atfork_parent ($) { _done_for_now($_[0]) }
 
 sub imap_idle_requeue { # DS::add_timer callback
 	my ($self, $uri_intvl) = @_;
@@ -449,13 +445,12 @@ sub event_step {
 	return if $self->{quit};
 	my $idle_todo = $self->{idle_todo};
 	if ($idle_todo && @$idle_todo) {
-		my $oldset = watch_atfork_parent($self);
+		watch_atfork_parent($self);
 		eval {
 			while (my $uri_intvl = shift(@$idle_todo)) {
 				imap_idle_fork($self, $uri_intvl);
 			}
 		};
-		PublicInbox::DS::sig_setmask($oldset);
 		die $@ if $@;
 	}
 	fs_scan_step($self) if $self->{mdre};
@@ -492,7 +487,7 @@ sub poll_fetch_fork { # DS::add_timer callback
 	my ($self, $intvl, $uris) = @_;
 	return if $self->{quit};
 	pipe(my ($r, $w)) or die "pipe: $!";
-	my $oldset = watch_atfork_parent($self);
+	watch_atfork_parent($self);
 	my $seed = rand(0xffffffff);
 	my $pid = fork;
 	if (defined($pid) && $pid == 0) {
@@ -508,7 +503,6 @@ sub poll_fetch_fork { # DS::add_timer callback
 		close $w;
 		_exit(0);
 	}
-	PublicInbox::DS::sig_setmask($oldset);
 	die "fork: $!"  unless defined $pid;
 	$self->{poll_pids}->{$pid} = [ $intvl, $uris ];
 	PublicInbox::EOFpipe->new($r, \&reap, [$pid, \&poll_fetch_reap, $self]);

[PATCH 10/15] doc: lei-overview: add CAVEATS section

[Eric Wong]

IMAP and NNTP client performance absolutely sucks compared to what
the read-only daemons are capable of...
---
 Documentation/lei-overview.pod | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/lei-overview.pod b/Documentation/lei-overview.pod
index bb2fe50f7cd9..99fd6ef72174 100644
--- a/Documentation/lei-overview.pod
+++ b/Documentation/lei-overview.pod
@@ -137,6 +137,11 @@ Since lei runs as a daemon, L<lei-daemon-kill(1)> is required to kill
 the daemon so it can load new code.  It will be restarted with the
 next invocation of any lei command.
 
+=head1 CAVEATS
+
+IMAP and NNTP client performance is poor on high-latency connections.
+It will hopefully be fixed in 2022.
+
 =head1 CONTACT
 
 Feedback welcome via plain-text mail to L<mailto:meta@public-inbox.org>

[PATCH 11/15] lei note-event: clear_src on ENOENT

[Eric Wong]

When a file goes away, try to make sure we don't waste
time trying to access or store it.
---
 lib/PublicInbox/LeiNoteEvent.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LeiNoteEvent.pm b/lib/PublicInbox/LeiNoteEvent.pm
index 3472e73070d6..22d6ffac9feb 100644
--- a/lib/PublicInbox/LeiNoteEvent.pm
+++ b/lib/PublicInbox/LeiNoteEvent.pm
@@ -8,6 +8,7 @@ use strict;
 use v5.10.1;
 use parent qw(PublicInbox::IPC);
 use PublicInbox::DS;
+use Errno qw(ENOENT);
 
 our $to_flush; # { cfgpath => $lei }
 
@@ -59,8 +60,11 @@ sub eml_event ($$$$) {
 
 sub maildir_event { # via wq_io_do
 	my ($self, $fn, $vmd, $state) = @_;
-	my $eml = PublicInbox::InboxWritable::eml_from_path($fn) // return;
-	eml_event($self, $eml, $vmd, $state);
+	if (my $eml = PublicInbox::InboxWritable::eml_from_path($fn)) {
+		eml_event($self, $eml, $vmd, $state);
+	} elsif ($! == ENOENT) {
+		$self->{lms}->clear_src(@{$vmd->{sync_info}});
+	} # else: eml_from_path already warns
 }
 
 sub lei_note_event {

[PATCH 12/15] dir_idle: treat IN_MOVED_FROM as a gone event

[Eric Wong]

Whether an MUA uses rename(2) or link(2)+unlink(2) combination
should not matter to us.  We should be able to handle both
cases.
---
 lib/PublicInbox/DirIdle.pm     | 3 ++-
 lib/PublicInbox/FakeInotify.pm | 3 +++
 lib/PublicInbox/LEI.pm         | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/DirIdle.pm b/lib/PublicInbox/DirIdle.pm
index 270d3829bc3e..9206da9cb710 100644
--- a/lib/PublicInbox/DirIdle.pm
+++ b/lib/PublicInbox/DirIdle.pm
@@ -14,7 +14,8 @@ if ($^O eq 'linux' && eval { require Linux::Inotify2; 1 }) {
 		Linux::Inotify2::IN_CREATE();
 	$MAIL_GONE = Linux::Inotify2::IN_DELETE() |
 			Linux::Inotify2::IN_DELETE_SELF() |
-			Linux::Inotify2::IN_MOVE_SELF();
+			Linux::Inotify2::IN_MOVE_SELF() |
+			Linux::Inotify2::IN_MOVED_FROM();
 	$ino_cls = 'Linux::Inotify2';
 # Perl 5.22+ is needed for fileno(DIRHANDLE) support:
 } elsif ($^V ge v5.22 && eval { require PublicInbox::KQNotify }) {
diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm
index 641bc5bdc7c6..6d2696019d83 100644
--- a/lib/PublicInbox/FakeInotify.pm
+++ b/lib/PublicInbox/FakeInotify.pm
@@ -10,6 +10,7 @@ use parent qw(Exporter);
 use Time::HiRes qw(stat);
 use PublicInbox::DS qw(add_timer);
 sub IN_MODIFY () { 0x02 } # match Linux inotify
+# my $IN_MOVED_FROM	 0x00000040	/* File was moved from X.  */
 # my $IN_MOVED_TO = 0x80;
 # my $IN_CREATE = 0x100;
 sub MOVED_TO_OR_CREATE () { 0x80 | 0x100 }
@@ -136,6 +137,7 @@ use strict;
 sub fullname { ${$_[0]} }
 
 sub IN_DELETE { 0 }
+sub IN_MOVED_FROM { 0 }
 sub IN_DELETE_SELF { 0 }
 
 package PublicInbox::FakeInotify::GoneEvent;
@@ -143,6 +145,7 @@ use strict;
 our @ISA = qw(PublicInbox::FakeInotify::Event);
 
 sub IN_DELETE { 1 }
+sub IN_MOVED_FROM { 0 }
 
 package PublicInbox::FakeInotify::SelfGoneEvent;
 use strict;
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index c1f28f7b31bf..b68e526bf365 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -1198,7 +1198,7 @@ sub dir_idle_handler ($) { # PublicInbox::DirIdle callback
 	my $fn = $ev->fullname;
 	if ($fn =~ m!\A(.+)/(new|cur)/([^/]+)\z!) { # Maildir file
 		my ($mdir, $nc, $bn) = ($1, $2, $3);
-		$nc = '' if $ev->IN_DELETE;
+		$nc = '' if $ev->IN_DELETE || $ev->IN_MOVED_FROM;
 		for my $f (keys %{$MDIR2CFGPATH->{$mdir} // {}}) {
 			my $cfg = $PATH2CFG{$f} // next;
 			eval {

[PATCH 13/15] lei: no Perl FileHandle for `undef' w/ ECONNRESET

[Eric Wong]

Error reporting for recv_cmd4 methods is a bit wonky.
---
 lib/PublicInbox/LEI.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index b68e526bf365..43baeeb3d51c 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -1129,6 +1129,7 @@ sub event_step {
 		if (scalar(@fds) == 1 && !defined($fds[0])) {
 			return if $! == EAGAIN;
 			die "recvmsg: $!" if $! != ECONNRESET;
+			@fds = (); # for open loop below:
 		}
 		for (@fds) { open my $rfh, '+<&=', $_ }
 		if ($buf eq '') {

[PATCH 14/15] lei_mail_sync: mv_src: use transaction, check UNIQUE

[Eric Wong]

We need a transaction across two SQL statements so readers
(which don't use flock) will see the result as atomic.

This may help against some occasional test failures I'm seeing
from t/lei-auto-watch.t and t/lei-watch.t, or make the problem
more apparent.
---
 lib/PublicInbox/LeiMailSync.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm
index e70cb5de2b6b..124eb96948c5 100644
--- a/lib/PublicInbox/LeiMailSync.pm
+++ b/lib/PublicInbox/LeiMailSync.pm
@@ -182,16 +182,20 @@ sub mv_src {
 	my ($self, $folder, $oidbin, $id, $newbn) = @_;
 	my $lk = $self->lock_for_scope;
 	my $fid = $self->{fmap}->{$folder} //= fid_for($self, $folder, 1);
+	$self->{dbh}->begin_work;
 	my $sth = $self->{dbh}->prepare_cached(<<'');
 UPDATE blob2name SET name = ? WHERE fid = ? AND oidbin = ? AND name = ?
 
-	my $nr = $sth->execute($newbn, $fid, $oidbin, $$id);
-	if ($nr == 0) { # may race with a clear_src, ensure new value exists
+	# eval since unique constraint may fail due to race
+	my $nr = eval { $sth->execute($newbn, $fid, $oidbin, $$id) };
+	if (!defined($nr) || $nr == 0) { # $nr may be `0E0'
+		# may race with a clear_src, ensure new value exists
 		$sth = $self->{dbh}->prepare_cached(<<'');
 INSERT OR IGNORE INTO blob2name (oidbin, fid, name) VALUES (?, ?, ?)
 
 		$sth->execute($oidbin, $fid, $newbn);
 	}
+	$self->{dbh}->commit;
 }
 
 # read-only, iterates every oidbin + UID or name for a given folder

[PATCH 15/15] lei: use RENAME_NOREPLACE on Linux 3.15+

[Eric Wong]

One syscall is better than two for atomicity in Maildirs.  This
means there's no window where another process can see both the
old and new file at the same time (link && unlink), nor a window
where we might inadvertantly clobber an existing file if we were
to do `stat && rename'.
---
 MANIFEST                       |  1 +
 devel/syscall-list             |  8 +++++-
 lib/PublicInbox/LeiExportKw.pm | 19 +++++--------
 lib/PublicInbox/LeiStore.pm    |  8 +++---
 lib/PublicInbox/LeiToMail.pm   |  7 +++--
 lib/PublicInbox/Syscall.pm     | 49 +++++++++++++++++++++++++++++++---
 t/rename_noreplace.t           | 26 ++++++++++++++++++
 7 files changed, 92 insertions(+), 26 deletions(-)
 create mode 100644 t/rename_noreplace.t

diff --git a/MANIFEST b/MANIFEST
index af1522d71bd1..9fd979ef02fb 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -528,6 +528,7 @@ t/psgi_v2.t
 t/purge.t
 t/qspawn.t
 t/reindex-time-range.t
+t/rename_noreplace.t
 t/replace.t
 t/reply.t
 t/run.perl
diff --git a/devel/syscall-list b/devel/syscall-list
index b33401d98ce4..3d55df1fc1d7 100755
--- a/devel/syscall-list
+++ b/devel/syscall-list
@@ -1,4 +1,4 @@
-# Copyright 2021 all contributors <meta@public-inbox.org>
+# Copyright all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <http://www.gnu.org/licenses/agpl-3.0.txt>
 # Dump syscall numbers under Linux and any other kernel which
 # promises stable syscall numbers.  This is to maintain
@@ -9,7 +9,10 @@
 eval 'exec perl -S $0 ${1+"$@"}' # no shebang
 	if 0; # running under some shell
 use strict;
+use v5.10.1;
 use File::Temp 0.19;
+use POSIX qw(uname);
+say '$machine='.(POSIX::uname())[-1];
 my $cc = $ENV{CC} // 'cc';
 my @cflags = split(/\s+/, $ENV{CFLAGS} // '-Wall');
 my $str = do { local $/; <DATA> };
@@ -43,6 +46,9 @@ int main(void)
 	D(SYS_inotify_add_watch);
 	D(SYS_inotify_rm_watch);
 	D(SYS_prctl);
+#ifdef SYS_renameat2
+	D(SYS_renameat2);
+#endif
 #endif /* Linux, any other OSes with stable syscalls? */
 	printf("size_t=%zu off_t=%zu\n", sizeof(size_t), sizeof(off_t));
 	return 0;
diff --git a/lib/PublicInbox/LeiExportKw.pm b/lib/PublicInbox/LeiExportKw.pm
index 0b65c2762633..ceeef7f21d54 100644
--- a/lib/PublicInbox/LeiExportKw.pm
+++ b/lib/PublicInbox/LeiExportKw.pm
@@ -7,6 +7,7 @@ use strict;
 use v5.10.1;
 use parent qw(PublicInbox::IPC PublicInbox::LeiInput);
 use Errno qw(EEXIST ENOENT);
+use PublicInbox::Syscall qw(rename_noreplace);
 
 sub export_kw_md { # LeiMailSync->each_src callback
 	my ($oidbin, $id, $self, $mdir) = @_;
@@ -30,30 +31,22 @@ sub export_kw_md { # LeiMailSync->each_src callback
 	my $lei = $self->{lei};
 	for my $d (@try) {
 		my $src = "$mdir/$d/$$id";
-
-		# we use link(2) + unlink(2) since rename(2) may
-		# inadvertently clobber if the "uniquefilename" part wasn't
-		# actually unique.
-		if (link($src, $dst)) { # success
-			# unlink(2) may ENOENT from parallel invocation,
-			# ignore it, but not other serious errors
-			if (!unlink($src) and $! != ENOENT) {
-				$lei->child_error(1, "E: unlink($src): $!");
-			}
+		if (rename_noreplace($src, $dst)) { # success
 			$self->{lms}->mv_src("maildir:$mdir",
 						$oidbin, $id, $bn);
-			return; # success anyways if link(2) worked
+			return; # success
 		} elsif ($! == EEXIST) { # lost race with lei/store?
 			return;
 		} elsif ($! != ENOENT) {
-			$lei->child_error(1, "E: link($src -> $dst): $!");
+			$lei->child_error(1,
+				"E: rename_noreplace($src -> $dst): $!");
 		} # else loop @try
 	}
 	my $e = $!;
 	# both tries failed
 	my $oidhex = unpack('H*', $oidbin);
 	my $src = "$mdir/{".join(',', @try)."}/$$id";
-	$lei->child_error(1, "link($src -> $dst) ($oidhex): $e");
+	$lei->child_error(1, "rename_noreplace($src -> $dst) ($oidhex): $e");
 	for (@try) { return if -e "$mdir/$_/$$id" }
 	$self->{lms}->clear_src("maildir:$mdir", $id);
 }
diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm
index 16e7d302dc2f..f1316229bb32 100644
--- a/lib/PublicInbox/LeiStore.pm
+++ b/lib/PublicInbox/LeiStore.pm
@@ -32,6 +32,7 @@ use POSIX ();
 use IO::Handle (); # ->autoflush
 use Sys::Syslog qw(syslog openlog);
 use Errno qw(EEXIST ENOENT);
+use PublicInbox::Syscall qw(rename_noreplace);
 
 sub new {
 	my (undef, $dir, $opt) = @_;
@@ -185,10 +186,7 @@ sub export1_kw_md ($$$$$) {
 	my $dst = "$mdir/cur/$bn";
 	for my $d (@try) {
 		my $src = "$mdir/$d/$orig";
-		if (link($src, $dst)) {
-			if (!unlink($src) and $! != ENOENT) {
-				syslog('warning', "unlink($src): $!");
-			}
+		if (rename_noreplace($src, $dst)) {
 			# TODO: verify oidbin?
 			$self->{lms}->mv_src("maildir:$mdir",
 					$oidbin, \$orig, $bn);
@@ -196,7 +194,7 @@ sub export1_kw_md ($$$$$) {
 		} elsif ($! == EEXIST) { # lost race with "lei export-kw"?
 			return;
 		} elsif ($! != ENOENT) {
-			syslog('warning', "link($src -> $dst): $!");
+			syslog('warning', "rename_noreplace($src -> $dst): $!");
 		}
 	}
 	for (@try) { return if -e "$mdir/$_/$orig" };
diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm
index ca4e92de48b7..d33d27aec006 100644
--- a/lib/PublicInbox/LeiToMail.pm
+++ b/lib/PublicInbox/LeiToMail.pm
@@ -12,6 +12,7 @@ use PublicInbox::Spawn qw(spawn);
 use Symbol qw(gensym);
 use IO::Handle; # ->autoflush
 use Fcntl qw(SEEK_SET SEEK_END O_CREAT O_EXCL O_WRONLY);
+use PublicInbox::Syscall qw(rename_noreplace);
 
 my %kw2char = ( # Maildir characters
 	draft => 'D',
@@ -262,10 +263,8 @@ sub _buf2maildir ($$$$) {
 		$rand = '';
 		do {
 			$base = $rand.$common.':2,'.kw2suffix($kw);
-		} while (!($ok = link($tmp, $dst.$base)) && $!{EEXIST} &&
-			($rand = _rand.','));
-		die "link($tmp, $dst$base): $!" unless $ok;
-		unlink($tmp) or warn "W: failed to unlink $tmp: $!\n";
+		} while (!($ok = rename_noreplace($tmp, $dst.$base)) &&
+			$!{EEXIST} && ($rand = _rand.','));
 		\$base;
 	} else {
 		my $err = "Error writing $smsg->{blob} to $dst: $!\n";
diff --git a/lib/PublicInbox/Syscall.pm b/lib/PublicInbox/Syscall.pm
index 7ab4291119ea..c00385b94db8 100644
--- a/lib/PublicInbox/Syscall.pm
+++ b/lib/PublicInbox/Syscall.pm
@@ -13,8 +13,9 @@
 # License or the Artistic License, as specified in the Perl README file.
 package PublicInbox::Syscall;
 use strict;
+use v5.10.1;
 use parent qw(Exporter);
-use POSIX qw(ENOSYS O_NONBLOCK);
+use POSIX qw(ENOENT EEXIST ENOSYS O_NONBLOCK);
 use Config;
 
 # $VERSION = '0.25'; # Sys::Syscall version
@@ -22,7 +23,7 @@ our @EXPORT_OK = qw(epoll_ctl epoll_create epoll_wait
                   EPOLLIN EPOLLOUT EPOLLET
                   EPOLL_CTL_ADD EPOLL_CTL_DEL EPOLL_CTL_MOD
                   EPOLLONESHOT EPOLLEXCLUSIVE
-                  signalfd);
+                  signalfd rename_noreplace);
 our %EXPORT_TAGS = (epoll => [qw(epoll_ctl epoll_create epoll_wait
                              EPOLLIN EPOLLOUT
                              EPOLL_CTL_ADD EPOLL_CTL_DEL EPOLL_CTL_MOD
@@ -64,13 +65,16 @@ our (
      $SYS_epoll_ctl,
      $SYS_epoll_wait,
      $SYS_signalfd4,
+     $SYS_renameat2,
      );
 
 my $SFD_CLOEXEC = 02000000; # Perl does not expose O_CLOEXEC
 our $no_deprecated = 0;
 
 if ($^O eq "linux") {
-    my $machine = (POSIX::uname())[-1];
+    my (undef, undef, $release, undef, $machine) = POSIX::uname();
+    my ($maj, $min) = ($release =~ /\A([0-9]+)\.([0-9]+)/);
+    $SYS_renameat2 = 0 if "$maj.$min" < 3.15;
     # whether the machine requires 64-bit numbers to be on 8-byte
     # boundaries.
     my $u64_mod_8 = 0;
@@ -91,22 +95,26 @@ if ($^O eq "linux") {
         $SYS_epoll_ctl    = 255;
         $SYS_epoll_wait   = 256;
         $SYS_signalfd4 = 327;
+        $SYS_renameat2 //= 353;
     } elsif ($machine eq "x86_64") {
         $SYS_epoll_create = 213;
         $SYS_epoll_ctl    = 233;
         $SYS_epoll_wait   = 232;
         $SYS_signalfd4 = 289;
+	$SYS_renameat2 //= 316;
     } elsif ($machine eq 'x32') {
         $SYS_epoll_create = 1073742037;
         $SYS_epoll_ctl = 1073742057;
         $SYS_epoll_wait = 1073742056;
         $SYS_signalfd4 = 1073742113;
+	$SYS_renameat2 //= 0x40000000 + 316;
     } elsif ($machine eq 'sparc64') {
 	$SYS_epoll_create = 193;
 	$SYS_epoll_ctl = 194;
 	$SYS_epoll_wait = 195;
 	$u64_mod_8 = 1;
 	$SYS_signalfd4 = 317;
+	$SYS_renameat2 //= 345;
 	$SFD_CLOEXEC = 020000000;
     } elsif ($machine =~ m/^parisc/) {
         $SYS_epoll_create = 224;
@@ -120,18 +128,21 @@ if ($^O eq "linux") {
         $SYS_epoll_wait   = 238;
         $u64_mod_8        = 1;
         $SYS_signalfd4 = 313;
+	$SYS_renameat2 //= 357;
     } elsif ($machine eq "ppc") {
         $SYS_epoll_create = 236;
         $SYS_epoll_ctl    = 237;
         $SYS_epoll_wait   = 238;
         $u64_mod_8        = 1;
         $SYS_signalfd4 = 313;
+	$SYS_renameat2 //= 357;
     } elsif ($machine =~ m/^s390/) {
         $SYS_epoll_create = 249;
         $SYS_epoll_ctl    = 250;
         $SYS_epoll_wait   = 251;
         $u64_mod_8        = 1;
         $SYS_signalfd4 = 322;
+	$SYS_renameat2 //= 347;
     } elsif ($machine eq "ia64") {
         $SYS_epoll_create = 1243;
         $SYS_epoll_ctl    = 1244;
@@ -153,6 +164,7 @@ if ($^O eq "linux") {
         $u64_mod_8        = 1;
         $no_deprecated    = 1;
         $SYS_signalfd4 = 74;
+	$SYS_renameat2 //= 276;
     } elsif ($machine =~ m/arm(v\d+)?.*l/) {
         # ARM OABI
         $SYS_epoll_create = 250;
@@ -160,18 +172,21 @@ if ($^O eq "linux") {
         $SYS_epoll_wait   = 252;
         $u64_mod_8        = 1;
         $SYS_signalfd4 = 355;
+	$SYS_renameat2 //= 382;
     } elsif ($machine =~ m/^mips64/) {
         $SYS_epoll_create = 5207;
         $SYS_epoll_ctl    = 5208;
         $SYS_epoll_wait   = 5209;
         $u64_mod_8        = 1;
         $SYS_signalfd4 = 5283;
+	$SYS_renameat2 //= 5311;
     } elsif ($machine =~ m/^mips/) {
         $SYS_epoll_create = 4248;
         $SYS_epoll_ctl    = 4249;
         $SYS_epoll_wait   = 4250;
         $u64_mod_8        = 1;
         $SYS_signalfd4 = 4324;
+	$SYS_renameat2 //= 4351;
     } else {
         # as a last resort, try using the *.ph files which may not
         # exist or may be wrong
@@ -280,6 +295,34 @@ sub signalfd ($$) {
 	}
 }
 
+sub _rename_noreplace_racy ($$) {
+	my ($old, $new) = @_;
+	if (link($old, $new)) {
+		warn "unlink $old: $!\n" if !unlink($old) && $! != ENOENT;
+		1
+	} else {
+		undef;
+	}
+}
+
+# TODO: support FD args?
+sub rename_noreplace ($$) {
+	my ($old, $new) = @_;
+	if ($SYS_renameat2) { # RENAME_NOREPLACE = 1, AT_FDCWD = -100
+		my $ret = syscall($SYS_renameat2, -100, $old, -100, $new, 1);
+		if ($ret == 0) {
+			1; # like rename() perlop
+		} elsif ($! == ENOSYS) {
+			undef $SYS_renameat2;
+			_rename_noreplace_racy($old, $new);
+		} else {
+			undef
+		}
+	} else {
+		_rename_noreplace_racy($old, $new);
+	}
+}
+
 1;
 
 =head1 WARRANTY
diff --git a/t/rename_noreplace.t b/t/rename_noreplace.t
new file mode 100644
index 000000000000..bd1c4e9236a7
--- /dev/null
+++ b/t/rename_noreplace.t
@@ -0,0 +1,26 @@
+#!perl -w
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+use strict;
+use v5.10.1;
+use PublicInbox::TestCommon;
+use_ok 'PublicInbox::Syscall', 'rename_noreplace';
+my ($tmpdir, $for_destroy) = tmpdir;
+
+open my $fh, '>', "$tmpdir/a" or xbail $!;
+my @sa = stat($fh);
+is(rename_noreplace("$tmpdir/a", "$tmpdir/b"), 1, 'rename_noreplace');
+my @sb = stat("$tmpdir/b");
+ok(scalar(@sb), 'new file exists');
+ok(!-e "$tmpdir/a", 'original gone');
+is("@sa[0,1]", "@sb[0,1]", 'same st_dev + st_ino');
+
+is(rename_noreplace("$tmpdir/a", "$tmpdir/c"), undef, 'undef on ENOENT');
+ok($!{ENOENT}, 'ENOENT set when missing');
+
+open $fh, '>', "$tmpdir/a" or xbail $!;
+is(rename_noreplace("$tmpdir/a", "$tmpdir/b"), undef, 'undef on EEXIST');
+ok($!{EEXIST}, 'EEXIST set when missing');
+is_deeply([stat("$tmpdir/b")], \@sb, 'target unchanged on EEXIST');
+
+done_testing;

Re: [PATCH 03/15] t/lei-p2q: extra diagnostics

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> I got one mysterious test failure here, once, and can't seem
> to reproduce it...

nearly 2 years later: I've hit it again, but once again cannot
reproduce it...

> +++ b/t/lei-p2q.t
> @@ -7,7 +7,7 @@ require_mods(qw(json DBD::SQLite Search::Xapian));
>  
>  test_lei(sub {
>  	ok(!lei(qw(p2q this-better-cause-format-patch-to-fail)),
> -		'p2q fails on bogus arg');
> +		'p2q fails on bogus arg') or diag $lei_err;
>  	like($lei_err, qr/format-patch.*failed/, 'notes format-patch failure');

$? does get printed (32768, so (>> 8) means 128), as it should be.
So I wonder if there's a place we drop the socket prematurely or
something else is amiss...

On a side note, lei's internal IPC could probably be simplified
a bit w/o sacrificing parallelism.