[PATCH 0/2] watch: some more bugfixes

[PATCH 0/2] watch: some more bugfixes

[Eric Wong]

Some things I noticed and fixed last week while reconfiguring
some inboxes to use the `basic' indexlevel to save disk space.

Eric Wong (2):
  watch: use detect_indexlevel for unconfigured inboxes
  watch: close inotify FD on ->quit

 lib/PublicInbox/Admin.pm         | 27 ---------------------------
 lib/PublicInbox/InboxWritable.pm | 27 +++++++++++++++++++++++++++
 lib/PublicInbox/SearchIdx.pm     |  2 +-
 lib/PublicInbox/Watch.pm         |  7 +++++++
 script/public-inbox-convert      |  2 +-
 script/public-inbox-index        |  6 +++---
 t/index-git-times.t              |  5 +++--
 t/indexlevels-mirror.t           |  7 ++++---
 t/init.t                         |  5 +++--
 9 files changed, 49 insertions(+), 39 deletions(-)

[PATCH 1/2] watch: use detect_indexlevel for unconfigured inboxes

[Eric Wong]

I favor leaving the publicinbox.<name>.indexlevel parameter
out of config files to make it easier to alter and reduce
sources of truth.  It worked well in most cases, but
public-inbox-watch also needs to detect the indexlevel.

Moving the sub to InboxWritable (from Admin) probably makes
sense since it's a per-inbox attribute and allows -watch
to reuse it.
---
 lib/PublicInbox/Admin.pm         | 27 ---------------------------
 lib/PublicInbox/InboxWritable.pm | 27 +++++++++++++++++++++++++++
 lib/PublicInbox/SearchIdx.pm     |  2 +-
 lib/PublicInbox/Watch.pm         |  2 ++
 script/public-inbox-convert      |  2 +-
 script/public-inbox-index        |  6 +++---
 t/index-git-times.t              |  5 +++--
 t/indexlevels-mirror.t           |  7 ++++---
 t/init.t                         |  5 +++--
 9 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/lib/PublicInbox/Admin.pm b/lib/PublicInbox/Admin.pm
index abfcbb9c..da34a3bd 100644
--- a/lib/PublicInbox/Admin.pm
+++ b/lib/PublicInbox/Admin.pm
@@ -87,33 +87,6 @@ sub resolve_git_dir {
 	$dir;
 }
 
-# for unconfigured inboxes
-sub detect_indexlevel ($) {
-	my ($ibx) = @_;
-
-	my $over = $ibx->over;
-	my $srch = $ibx->search;
-	delete @$ibx{qw(over search)}; # don't leave open FDs lying around
-
-	# brand new or never before indexed inboxes default to full
-	return 'full' unless $over;
-	my $l = 'basic';
-	return $l unless $srch;
-	if (my $xdb = $srch->xdb) {
-		$l = 'full';
-		my $m = $xdb->get_metadata('indexlevel');
-		if ($m eq 'medium') {
-			$l = $m;
-		} elsif ($m ne '') {
-			warn <<"";
-$ibx->{inboxdir} has unexpected indexlevel in Xapian: $m
-
-		}
-		$ibx->{-skip_docdata} = 1 if $xdb->get_metadata('skip_docdata');
-	}
-	$l;
-}
-
 sub unconfigured_ibx ($$) {
 	my ($dir, $i) = @_;
 	my $name = "unconfigured-$i";
diff --git a/lib/PublicInbox/InboxWritable.pm b/lib/PublicInbox/InboxWritable.pm
index 17dfbe18..022e2a69 100644
--- a/lib/PublicInbox/InboxWritable.pm
+++ b/lib/PublicInbox/InboxWritable.pm
@@ -245,4 +245,31 @@ sub git_dir_latest {
 		"$self->{inboxdir}/git/$$max.git" : undef;
 }
 
+# for unconfigured inboxes
+sub detect_indexlevel ($) {
+	my ($ibx) = @_;
+
+	my $over = $ibx->over;
+	my $srch = $ibx->search;
+	delete @$ibx{qw(over search)}; # don't leave open FDs lying around
+
+	# brand new or never before indexed inboxes default to full
+	return 'full' unless $over;
+	my $l = 'basic';
+	return $l unless $srch;
+	if (my $xdb = $srch->xdb) {
+		$l = 'full';
+		my $m = $xdb->get_metadata('indexlevel');
+		if ($m eq 'medium') {
+			$l = $m;
+		} elsif ($m ne '') {
+			warn <<"";
+$ibx->{inboxdir} has unexpected indexlevel in Xapian: $m
+
+		}
+		$ibx->{-skip_docdata} = 1 if $xdb->get_metadata('skip_docdata');
+	}
+	$l;
+}
+
 1;
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index b907772e..f36c8f97 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -1123,7 +1123,7 @@ sub begin_txn_lazy {
 }
 
 # store 'indexlevel=medium' in v2 shard=0 and v1 (only one shard)
-# This metadata is read by Admin::detect_indexlevel:
+# This metadata is read by InboxWritable->detect_indexlevel:
 sub set_metadata_once {
 	my ($self) = @_;
 
diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm
index 693c7181..b87abafe 100644
--- a/lib/PublicInbox/Watch.pm
+++ b/lib/PublicInbox/Watch.pm
@@ -79,6 +79,8 @@ sub new {
 		my $ibx = $_[0] = PublicInbox::InboxWritable->new($_[0]);
 
 		my $watches = $ibx->{watch} or return;
+
+		$ibx->{indexlevel} //= $ibx->detect_indexlevel;
 		$watches = PublicInbox::Config::_array($watches);
 		for my $watch (@$watches) {
 			my $uri;
diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index 5f4f2020..750adca4 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -63,7 +63,7 @@ if (delete $old->{-unconfigured}) {
 }
 die "Only conversion from v1 inboxes is supported\n" if $old->version >= 2;
 
-my $detected = PublicInbox::Admin::detect_indexlevel($old);
+my $detected = $old->detect_indexlevel;
 $old->{indexlevel} //= $detected;
 my $env;
 if ($opt->{'index'}) {
diff --git a/script/public-inbox-index b/script/public-inbox-index
index a04be9fc..f29e7c3c 100755
--- a/script/public-inbox-index
+++ b/script/public-inbox-index
@@ -66,6 +66,7 @@ $opt->{-use_cwd} = 1;
 my @ibxs = PublicInbox::Admin::resolve_inboxes(\@ARGV, $opt, $cfg);
 PublicInbox::Admin::require_or_die('-index');
 unless (@ibxs) { print STDERR $help; exit 1 }
+require PublicInbox::InboxWritable;
 
 my (@eidx, %eidx_seen);
 my $update_extindex = $opt->{'update-extindex'};
@@ -96,8 +97,9 @@ for my $ei_name (@$update_extindex) {
 my $mods = {};
 my @eidx_unconfigured;
 foreach my $ibx (@ibxs) {
+	$ibx = PublicInbox::InboxWritable->new($ibx);
 	# detect_indexlevel may also set $ibx->{-skip_docdata}
-	my $detected = PublicInbox::Admin::detect_indexlevel($ibx);
+	my $detected = $ibx->detect_indexlevel;
 	# XXX: users can shoot themselves in the foot, with opt->{indexlevel}
 	$ibx->{indexlevel} //= $opt->{indexlevel} // ($opt->{xapian_only} ?
 			'full' : $detected);
@@ -117,11 +119,9 @@ $opt->{compact} = 0 if !$mods->{'Search::Xapian'};
 PublicInbox::Admin::require_or_die(keys %$mods);
 my $env = PublicInbox::Admin::index_prepare($opt, $cfg);
 local %ENV = (%ENV, %$env) if $env;
-require PublicInbox::InboxWritable;
 PublicInbox::Xapcmd::check_compact() if $opt->{compact};
 PublicInbox::Admin::progress_prepare($opt);
 for my $ibx (@ibxs) {
-	$ibx = PublicInbox::InboxWritable->new($ibx);
 	if ($opt->{compact} >= 2) {
 		PublicInbox::Xapcmd::run($ibx, 'compact', $opt->{compact_opt});
 	}
diff --git a/t/index-git-times.t b/t/index-git-times.t
index ffe9223c..96886c5e 100644
--- a/t/index-git-times.t
+++ b/t/index-git-times.t
@@ -8,6 +8,7 @@ use PublicInbox::Config;
 use PublicInbox::Admin;
 use PublicInbox::Import;
 use File::Path qw(remove_tree);
+require PublicInbox::InboxWritable;
 
 require_mods(qw(DBD::SQLite Search::Xapian));
 use_ok 'PublicInbox::Over';
@@ -57,7 +58,7 @@ my $smsg;
 {
 	my $cfg = PublicInbox::Config->new;
 	my $ibx = $cfg->lookup($addr);
-	my $lvl = PublicInbox::Admin::detect_indexlevel($ibx);
+	my $lvl = PublicInbox::InboxWritable::detect_indexlevel($ibx);
 	is($lvl, 'medium', 'indexlevel detected');
 	is($ibx->{-skip_docdata}, 1, '--skip-docdata flag set on -index');
 	$smsg = $ibx->over->get_art(1);
@@ -80,7 +81,7 @@ SKIP: {
 	my $check_v2 = sub {
 		my $ibx = PublicInbox::Inbox->new({inboxdir => $v2dir,
 				address => $addr});
-		my $lvl = PublicInbox::Admin::detect_indexlevel($ibx);
+		my $lvl = PublicInbox::InboxWritable::detect_indexlevel($ibx);
 		is($lvl, 'medium', 'indexlevel detected after convert');
 		is($ibx->{-skip_docdata}, 1,
 			'--skip-docdata preserved after convert');
diff --git a/t/indexlevels-mirror.t b/t/indexlevels-mirror.t
index 463b35be..62411671 100644
--- a/t/indexlevels-mirror.t
+++ b/t/indexlevels-mirror.t
@@ -5,7 +5,7 @@ use strict;
 use v5.10.1;
 use PublicInbox::TestCommon;
 use PublicInbox::Eml;
-use PublicInbox::Inbox;
+use PublicInbox::InboxWritable;
 require PublicInbox::Admin;
 my $PI_TEST_VERSION = $ENV{PI_TEST_VERSION} || 2;
 require_git('2.6') if $PI_TEST_VERSION == 2;
@@ -110,7 +110,8 @@ my $import_index_incremental = sub {
 
 	if ($level ne 'basic') {
 		ok(run_script(['-xcpdb', '-q', $mirror]), "v$v xcpdb OK");
-		is(PublicInbox::Admin::detect_indexlevel($ro_mirror), $level,
+		is(PublicInbox::InboxWritable::detect_indexlevel($ro_mirror),
+			$level,
 		   'indexlevel detectable by Admin after xcpdb v' .$v.$level);
 		delete $ro_mirror->{$_} for (qw(over search));
 		my $mset = $ro_mirror->search->mset('m:m@2');
@@ -152,7 +153,7 @@ my $import_index_incremental = sub {
 	is_deeply(\@rw_nums, \@expect, "v$v master has expected NNTP articles");
 	is_deeply(\@ro_nums, \@expect, "v$v mirror matches master articles");
 
-	is(PublicInbox::Admin::detect_indexlevel($ro_mirror), $level,
+	is(PublicInbox::InboxWritable::detect_indexlevel($ro_mirror), $level,
 	   'indexlevel detectable by Admin '.$v.$level);
 
 	SKIP: {
diff --git a/t/init.t b/t/init.t
index 46258e45..0096ca30 100644
--- a/t/init.t
+++ b/t/init.t
@@ -6,6 +6,7 @@ use v5.10.1;
 use PublicInbox::Config;
 use PublicInbox::TestCommon;
 use PublicInbox::Admin;
+use PublicInbox::InboxWritable;
 my ($tmpdir, $for_destroy) = tmpdir();
 sub quiet_fail {
 	my ($cmd, $msg) = @_;
@@ -147,7 +148,7 @@ SKIP: {
 		ok(run_script($cmd), "-init -L $lvl");
 		is(read_indexlevel("v2$lvl"), $lvl, "indexlevel set to '$lvl'");
 		my $ibx = PublicInbox::Inbox->new({ inboxdir => $dir });
-		is(PublicInbox::Admin::detect_indexlevel($ibx), $lvl,
+		is(PublicInbox::InboxWritable::detect_indexlevel($ibx), $lvl,
 			'detected expected level w/o config');
 		ok(!$ibx->{-skip_docdata}, 'docdata written by default');
 	}
@@ -159,7 +160,7 @@ SKIP: {
 			"$name\@example.com" ];
 		ok(run_script($cmd), "-init -V$v --skip-docdata");
 		my $ibx = PublicInbox::Inbox->new({ inboxdir => $dir });
-		is(PublicInbox::Admin::detect_indexlevel($ibx), 'full',
+		is(PublicInbox::InboxWritable::detect_indexlevel($ibx), 'full',
 			"detected default indexlevel -V$v");
 		ok($ibx->{-skip_docdata}, "docdata skip set -V$v");
 		ok($ibx->search->has_threadid, 'has_threadid flag set on new inbox');

[PATCH 2/2] watch: close inotify FD on ->quit

[Eric Wong]

For simplicity, we quit and recreate an entire watch instance
on SIGHUP.  However, inotify (and signalfd) FDs are tied to
the DS event loop and stay pinned to existence that way.
Thus we explicitly close the FD in Watch->quit to prevent
leakage on SIGHUP.
---
 lib/PublicInbox/Watch.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm
index b87abafe..c7acda14 100644
--- a/lib/PublicInbox/Watch.pm
+++ b/lib/PublicInbox/Watch.pm
@@ -256,6 +256,10 @@ sub quit {
 	%{$self->{opendirs}} = ();
 	_done_for_now($self);
 	quit_done($self);
+	if (defined(my $fd = delete $self->{dir_idle_fd})) {
+		my $di = $PublicInbox::DS::DescriptorMap{$fd};
+		$di->close if $di && $di->can('add_watches');
+	}
 	if (my $idle_mic = delete $self->{idle_mic}) {
 		return unless $idle_mic->IsConnected && $idle_mic->Socket;
 		eval { $idle_mic->done };
@@ -280,6 +284,7 @@ sub watch_fs_init ($) {
 	require PublicInbox::DirIdle;
 	# inotify_create + EPOLL_CTL_ADD
 	my $dir_idle = PublicInbox::DirIdle->new($cb);
+	$self->{dir_idle_fd} = fileno($dir_idle->{sock}) if $dir_idle->{sock};
 	$dir_idle->add_watches([keys %{$self->{mdmap}}]);
 }