missing messages in thread overview

missing messages in thread overview

[Kyle Meyer]

I noticed a thread today that is missing messages in the thread
overview.  In the top-level inbox, all the messages are there:

  [bug#45000] [PATCH 0/9] Build git-annex with assistant webapp enabled
   2020-12-02 11:17 UTC  (11+ messages) - mbox.gz / Atom
  ` [bug#45000] [PATCH 1/9] gnu: Add ghc-yesod-test
    ` [bug#45000] [PATCH 2/9] gnu: Add ghc-wai-app-static
    ` [bug#45000] [PATCH 3/9] gnu: Add ghc-yesod-static
    ` [bug#45000] [PATCH 4/9] gnu: Add ghc-cryptonite-conduit
    ` [bug#45000] [PATCH 5/9] gnu: ghc-yesod-core: Update to 1.6.18.6
    ` [bug#45000] [PATCH 6/9] gnu: Add ghc-hjsmin
    ` [bug#45000] [PATCH 7/9] gnu: Add ghc-template-haskell
    ` [bug#45000] [PATCH 8/9] gnu: Add ghc-boot-th
    ` [bug#45000] [PATCH 9/9] gnu: git-annex: Build with git-annex assistant webapp enabled

However, when I click through, the thread overview is missing five
messages:

  Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
  -- links below jump to the message on this page --
  2020-12-02  4:53 [bug#45000] [PATCH 0/9] Build git-annex with assistant webapp enabled Jonathan Frederickson
  2020-12-02  4:55 ` [bug#45000] [PATCH 1/9] gnu: Add ghc-yesod-test Jonathan Frederickson
  2020-12-02  4:55   ` [bug#45000] [PATCH 2/9] gnu: Add ghc-wai-app-static Jonathan Frederickson
  2020-12-02  4:55   ` [bug#45000] [PATCH 7/9] gnu: Add ghc-template-haskell Jonathan Frederickson
  2020-12-02  4:55   ` [bug#45000] [PATCH 9/9] gnu: git-annex: Build with git-annex assistant webapp enabled Jonathan Frederickson
  2020-12-02 11:17 ` [bug#45000] [PATCH 0/9] Build git-annex with " zimoun

https://yhetil.org/guix-patches/86sg8o1mou.fsf@gmail.com/T/#t

The same subset of messages are in the thread's mbox:

  $ curl -fSsL https://yhetil.org/guix-patches/86sg8o1mou.fsf@gmail.com/t.mbox.gz \
    | gzip -d | grep -i ^subject
  Subject: [bug#45000] [PATCH 0/9] Build git-annex with assistant webapp enabled
  Subject: [bug#45000] [PATCH 1/9] gnu: Add ghc-yesod-test.
  Subject: [bug#45000] [PATCH 7/9] gnu: Add ghc-template-haskell.
  Subject: [bug#45000] [PATCH 9/9] gnu: git-annex: Build with git-annex
  Subject: [bug#45000] [PATCH 2/9] gnu: Add ghc-wai-app-static.
  Subject: [bug#45000] [PATCH 0/9] Build git-annex with assistant webapp enabled

I collected the complete thread (currently eleven messages) and used
scripts/import_vger_from_mbox to create a separate inbox at
<https://yhetil.org/debug-gap/>.  As before, the top-level listing shows
all the patches, and the thread overview is still missing some, though
different ones:

  2020-12-02  4:53 [bug#45000] [PATCH 0/9] Build git-annex with assistant webapp enabled Jonathan Frederickson
  2020-12-02 11:17 ` zimoun
  2020-12-02  4:55 [bug#45000] [PATCH 1/9] gnu: Add ghc-yesod-test Jonathan Frederickson
  2020-12-02  4:55 ` [bug#45000] [PATCH 3/9] gnu: Add ghc-yesod-static Jonathan Frederickson
  2020-12-02  4:55 ` [bug#45000] [PATCH 4/9] gnu: Add ghc-cryptonite-conduit Jonathan Frederickson
  2020-12-02  4:55 ` [bug#45000] [PATCH 5/9] gnu: ghc-yesod-core: Update to 1.6.18.6 Jonathan Frederickson
  2020-12-02  4:55 ` [bug#45000] [PATCH 6/9] gnu: Add ghc-hjsmin Jonathan Frederickson
  2020-12-02  4:55 ` [bug#45000] [PATCH 8/9] gnu: Add ghc-boot-th Jonathan Frederickson

On rethreading with `public-inbox-index -c --reindex --rethread', the
thread overview looks like this:

  2020-12-02  4:53 [bug#45000] [PATCH 0/9] Build git-annex with assistant webapp enabled Jonathan Frederickson
  2020-12-02 11:17 ` zimoun

All of the above is with a recent commit on master,
v1.6.0-149-g4cc4ad59.

Any ideas what might be going on here?

Re: missing messages in thread overview

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> <https://yhetil.org/debug-gap/>.

Thanks, I'm able to reproduce it.

<snip>

> Any ideas what might be going on here?

It seems like the presence of two ghosts in the histories fails
to get merged together; so it's order-dependency bug in
OverIdx.pm rethread seems to be exacerbating the condition (less
sure about rethread, though).

Basically, there should be only one distinct `tid' column
in the `over' table, but there's two tids and that's
causing over->get_thread to only see one tid or the other.

The top-level inbox view doesn't care about `tid' column, it
just fetches the most recent messages; so it's able to get
a complete view to render all the messages in the thread.

Will try to work on it more in a bit (struggling to decide
between starvation or possible COVID-19 exposure... :<)

[WIP] over: ensure old, merged {tid} is really gone.

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> It seems like the presence of two ghosts in the histories fails
> to get merged together; so it's order-dependency bug in
> OverIdx.pm rethread seems to be exacerbating the condition (less
> sure about rethread, though).

Yes, the fix is quite small (I think the below test case can be
made smaller).

--rethread seems to be a separate bug, will fix when more awake.

-----------------8<--------------
Subject: [PATCH] over: ensure old, merged {tid} is really gone.

We must use the result of link_refs() since it can trigger
merge_threads() and invalidate $old_tid.  In case
merge_threads() isn't triggered,  link_refs() will return
$old_tid anyways.
---
 MANIFEST                   |  1 +
 lib/PublicInbox/OverIdx.pm |  2 +-
 t/thread-index-gap.t       | 94 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 1 deletion(-)
 create mode 100644 t/thread-index-gap.t

diff --git a/MANIFEST b/MANIFEST
index 544ec5f9..946e4b8a 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -366,6 +366,7 @@ t/solver_git.t
 t/spamcheck_spamc.t
 t/spawn.t
 t/thread-cycle.t
+t/thread-index-gap.t
 t/time.t
 t/uri_imap.t
 t/utf8.eml
diff --git a/lib/PublicInbox/OverIdx.pm b/lib/PublicInbox/OverIdx.pm
index 07cca4e5..ac53518c 100644
--- a/lib/PublicInbox/OverIdx.pm
+++ b/lib/PublicInbox/OverIdx.pm
@@ -298,7 +298,7 @@ sub _add_over {
 		}
 	} elsif ($n < 0) { # ghost
 		$$old_tid //= $cur_valid ? $cur_tid : next_tid($self);
-		link_refs($self, $refs, $$old_tid);
+		$$old_tid = link_refs($self, $refs, $$old_tid);
 		delete_by_num($self, $n);
 		$$v++;
 	}
diff --git a/t/thread-index-gap.t b/t/thread-index-gap.t
new file mode 100644
index 00000000..1772ce22
--- /dev/null
+++ b/t/thread-index-gap.t
@@ -0,0 +1,94 @@
+#!perl -w
+# Copyright (C) 2020 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 Test::More;
+use PublicInbox::TestCommon;
+use PublicInbox::Eml;
+use PublicInbox::InboxWritable;
+use PublicInbox::Config;
+require_mods(qw(DBD::SQLite));
+require_git(2.6);
+my ($home, $for_destroy) = tmpdir();
+local $ENV{HOME} = $home;
+ok(run_script([qw(-init -V2 index-gap), "$home/index-gap",
+	qw(http://example.com/v2test index-gap@example.com)]), 'init');
+my $pi_cfg = PublicInbox::Config->new;
+my $ibx = $pi_cfg->lookup_name('index-gap');
+PublicInbox::InboxWritable->new($ibx);
+
+chomp(my @msgs = reverse(split(/\n\n/, <<'EOF')));
+Subject: [bug#45000] [PATCH 2/9]
+Message-Id: <20201202045540.31248-2-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 9/9]
+Message-Id: <20201202045540.31248-9-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 7/9]
+Message-Id: <20201202045540.31248-7-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 1/9]
+References: <20201202045335.31096-1-j@example.com>
+In-Reply-To: <20201202045335.31096-1-j@example.com>
+Message-Id: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 0/9]
+Message-Id: <20201202045335.31096-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 0/9]
+In-Reply-To: <20201202045335.31096-1-j@example.com>
+References: <20201202045335.31096-1-j@example.com>
+Message-ID: <86sg8o1mou.fsf@example.com>
+
+Subject: [bug#45000] [PATCH 8/9]
+Message-Id: <20201202045540.31248-8-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 6/9]
+Message-Id: <20201202045540.31248-6-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 5/9]
+Message-Id: <20201202045540.31248-5-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 4/9]
+Message-Id: <20201202045540.31248-4-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 3/9]
+Message-Id: <20201202045540.31248-3-j@example.com>
+In-Reply-To: <20201202045540.31248-1-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+EOF
+
+my $im = $ibx->importer(0);
+for my $msg (@msgs) {
+	$im->add(PublicInbox::Eml->new("$msg\nFrom: x\@example.com\n\n"));
+}
+$im->done;
+
+my @tid = $ibx->over->dbh->selectall_array('SELECT DISTINCT(tid) FROM over');
+is(scalar(@tid), 1, 'only one thread');
+
+if (0 && 'FIXME') {
+	$ibx->over->dbh_close;
+	ok(run_script([qw(-index --reindex --rethread -v), $ibx->{inboxdir}]),
+		'rethread');
+	@tid = $ibx->over->dbh->selectall_array(
+				'SELECT DISTINCT(tid) FROM over');
+	is(scalar(@tid), 1, 'only one thread after rethread');
+}
+
+done_testing;

Re: [WIP] over: ensure old, merged {tid} is really gone.

[Kyle Meyer]

Eric Wong writes:

> Yes, the fix is quite small (I think the below test case can be
> made smaller).
>
> --rethread seems to be a separate bug, will fix when more awake.

Great, thank you for the explanation and fix.

By the way, I've been enjoying playing with the extindex feature.  I
haven't been doing anything fancy and it's just a few inboxes, but it's
been _really_ nice to be able to group the inboxes for a project.
Anyway, off topic for this thread, but thanks for that too!

[PATCH] over: ensure old, merged {tid} is really gone

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> Eric Wong writes:
> 
> > Yes, the fix is quite small (I think the below test case can be
> > made smaller).
> >
> > --rethread seems to be a separate bug, will fix when more awake.
> 
> Great, thank you for the explanation and fix.

No problem, thanks for the excellent bug report.  Below patch
fixes the rethread case, too, which has the same conceptual
problem in it was leaving a stale {tid} in the DB.

Also made the test data smaller and a shuffle test to ensure
it's truly order-independent.

> By the way, I've been enjoying playing with the extindex feature.  I
> haven't been doing anything fancy and it's just a few inboxes, but it's
> been _really_ nice to be able to group the inboxes for a project.
> Anyway, off topic for this thread, but thanks for that too!

Great to know.  Working on some other stuff in that area to
allow selecting inbox subsets and (hopefully) reduce SSD wear.

-------------------8<---------------

Subject: [PATCH] over: ensure old, merged {tid} is really gone

We must use the result of link_refs() since it can trigger
merge_threads() and invalidate $old_tid.  In case
merge_threads() isn't triggered, link_refs() will return
$old_tid anyways.

When rethreading and allocating new {tid}, we also must update
the row where the now-expired {tid} came from to ensure only the
new {tid} is seen when reindexing subsequent messages in
history.  Otherwise, every subsequently reindexed+rethreaded
message could end up getting a new {tid}.

Reported-by: Kyle Meyer <kyle@kyleam.com>
Link: https://public-inbox.org/meta/87360nlc44.fsf@kyleam.com/
---
 MANIFEST                   |  1 +
 lib/PublicInbox/OverIdx.pm | 12 ++++++--
 t/thread-index-gap.t       | 57 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 3 deletions(-)
 create mode 100644 t/thread-index-gap.t

diff --git a/MANIFEST b/MANIFEST
index 544ec5f9..946e4b8a 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -366,6 +366,7 @@ t/solver_git.t
 t/spamcheck_spamc.t
 t/spawn.t
 t/thread-cycle.t
+t/thread-index-gap.t
 t/time.t
 t/uri_imap.t
 t/utf8.eml
diff --git a/lib/PublicInbox/OverIdx.pm b/lib/PublicInbox/OverIdx.pm
index 07cca4e5..88daa64f 100644
--- a/lib/PublicInbox/OverIdx.pm
+++ b/lib/PublicInbox/OverIdx.pm
@@ -170,8 +170,14 @@ sub _resolve_mid_to_tid {
 		$$tid = $cur_tid;
 	} else { # rethreading, queue up dead ghosts
 		$$tid = next_tid($self);
-		my $num = $smsg->{num};
-		push(@{$self->{-ghosts_to_delete}}, $num) if $num < 0;
+		my $n = $smsg->{num};
+		if ($n > 0) {
+			$self->{dbh}->prepare_cached(<<'')->execute($$tid, $n);
+UPDATE over SET tid = ? WHERE num = ?
+
+		} elsif ($n < 0) {
+			push(@{$self->{-ghosts_to_delete}}, $n);
+		}
 	}
 	1;
 }
@@ -298,7 +304,7 @@ sub _add_over {
 		}
 	} elsif ($n < 0) { # ghost
 		$$old_tid //= $cur_valid ? $cur_tid : next_tid($self);
-		link_refs($self, $refs, $$old_tid);
+		$$old_tid = link_refs($self, $refs, $$old_tid);
 		delete_by_num($self, $n);
 		$$v++;
 	}
diff --git a/t/thread-index-gap.t b/t/thread-index-gap.t
new file mode 100644
index 00000000..49f254e9
--- /dev/null
+++ b/t/thread-index-gap.t
@@ -0,0 +1,57 @@
+#!perl -w
+# Copyright (C) 2020 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 Test::More;
+use PublicInbox::TestCommon;
+use PublicInbox::Eml;
+use PublicInbox::InboxWritable;
+use PublicInbox::Config;
+use List::Util qw(shuffle);
+require_mods(qw(DBD::SQLite));
+require_git(2.6);
+
+chomp(my @msgs = split(/\n\n/, <<'EOF')); # "git log" order
+Subject: [bug#45000] [PATCH 1/9]
+References: <20201202045335.31096-1-j@example.com>
+Message-Id: <20201202045540.31248-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 0/9]
+Message-Id: <20201202045335.31096-1-j@example.com>
+
+Subject: [bug#45000] [PATCH 0/9]
+References: <20201202045335.31096-1-j@example.com>
+Message-ID: <86sg8o1mou.fsf@example.com>
+
+Subject: [bug#45000] [PATCH 8/9]
+Message-Id: <20201202045540.31248-8-j@example.com>
+References: <20201202045540.31248-1-j@example.com>
+
+EOF
+
+my ($home, $for_destroy) = tmpdir();
+local $ENV{HOME} = $home;
+for my $msgs (['orig', reverse @msgs], ['shuffle', shuffle(@msgs)]) {
+	my $desc = shift @$msgs;
+	my $n = "index-cap-$desc";
+	run_script([qw(-init -L basic -V2), $n, "$home/$n",
+		"http://example.com/$n", "$n\@example.com"]) or
+		BAIL_OUT 'init';
+	my $ibx = PublicInbox::Config->new->lookup_name($n);
+	my $im = PublicInbox::InboxWritable->new($ibx)->importer(0);
+	for my $m (@$msgs) {
+		$im->add(PublicInbox::Eml->new("$m\nFrom: x\@example.com\n\n"));
+	}
+	$im->done;
+	my $over = $ibx->over;
+	my @tid = $over->dbh->selectall_array('SELECT DISTINCT(tid) FROM over');
+	is(scalar(@tid), 1, "only one thread initially ($desc)");
+	$over->dbh_close;
+	run_script([qw(-index --reindex --rethread), $ibx->{inboxdir}]) or
+		BAIL_OUT 'rethread';
+	@tid = $over->dbh->selectall_array('SELECT DISTINCT(tid) FROM over');
+	is(scalar(@tid), 1, "only one thread after rethread ($desc)");
+}
+
+done_testing;