notmuch insert --keep fails in parallel w/ notmuch new

notmuch insert --keep fails in parallel w/ notmuch new

[Maarten Aertsen]

Hello list,

I'd like to report a bug against notmuch 0.21, packaged for Debian 
jessie-backports as 0.21-3~bpo8+1.

# Steps to reproduce:
  1. start a notmuch new mail w/ lots of stuff to process (e.g. a 
     migration)
  2. try notmuch insert --keep

# Expected result:
Given the current text in the manpage, I'd expect notmuch insert --keep 
to save the message to the Maildir even if no db interaction is possible:

"--keep Keep the message file if indexing fails, and keep the message 
	indexed if applying tags or maildir flag synchronization fails.  
	Ignore these  errors and return exit status 0 to indicate 
        succesful mail delivery."

That way, I can cron notmuch new to pick up the occasional message that 
can't get indexed right away.

# Actual result:
notmuch insert --keep fails with exit code 1 due to the inability to get 
db access.

# Context:
I've switched to a setup with postfix, where notmuch insert --keep is 
called by /local/, postfix's local delivery daemon, via a pipe in 
.forward. Local treats a non-zero exit code as a 5.3.x status code and 
sends a bounce to the sender.

I had a short chat with j4ni on #notmuch, where he said:
< j4ni> Sagi: oh bummer, we try to open the db first, and bail out 
early. we should change the order of that (at least with --keep).

Thanks for developing notmuch.

best regards, Maarten Aertsen

Re: notmuch insert --keep fails in parallel w/ notmuch new

[David Bremner]

Maarten Aertsen <sagi-notmuch@rtsn.nl> writes:

>
> I had a short chat with j4ni on #notmuch, where he said:
> < j4ni> Sagi: oh bummer, we try to open the db first, and bail out 
> early. we should change the order of that (at least with --keep).
>

FWIW, I agree it's a bug, and it should be not too hard to fix by moving
the open after the call to maildir write new.

d

[PATCH] cli: avoid non-zero exits in notmuch insert --keep

[Maarten Aertsen]

Re-ordered code that touches the database to try and deliver e-mail to
at least try to deliver to the Maildir (which, with --keep should return
success).

In the case of any failure, we now return EX_TEMPFAIL (a sendmail
convention, defined in sysexits.h) to signal to the LDA that it should
retry later. This prevents a direct reject or bounce of e-mail.
---
 notmuch-client.h |  1 +
 notmuch-insert.c | 42 +++++++++++++++++++++++-------------------
 notmuch.c        | 17 +++++++++++++----
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 18e6c60..e3d6a46 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -466,6 +466,7 @@ print_status_query (const char *loc,
 
 extern char *notmuch_requested_db_uuid;
 extern const notmuch_opt_desc_t  notmuch_shared_options [];
+notmuch_bool_t notmuch_has_unmatched_db_uuid (notmuch_database_t *notmuch);
 void notmuch_exit_if_unmatched_db_uuid (notmuch_database_t *notmuch);
 
 void notmuch_process_shared_options (const char* subcommand_name);
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5205c17..35b6779 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -28,6 +28,8 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+#include <sysexits.h>
+
 static volatile sig_atomic_t interrupted;
 
 static void
@@ -532,31 +534,33 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     action.sa_flags = 0;
     sigaction (SIGINT, &action, NULL);
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-	return EXIT_FAILURE;
-
-    notmuch_exit_if_unmatched_db_uuid (notmuch);
-
     /* Write the message to the Maildir new directory. */
     newpath = maildir_write_new (config, STDIN_FILENO, maildir);
     if (! newpath) {
-	notmuch_database_destroy (notmuch);
 	return EXIT_FAILURE;
     }
 
-    /* Index the message. */
-    status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep);
-
-    /* Commit changes. */
-    close_status = notmuch_database_destroy (notmuch);
-    if (close_status) {
-	/* Hold on to the first error, if any. */
-	if (! status)
-	    status = close_status;
-	fprintf (stderr, "%s: failed to commit database changes: %s\n",
-		 keep ? "Warning" : "Error",
-		 notmuch_status_to_string (close_status));
+    status = notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    if (! status) {
+	/* with keep, send EX_TEMPFAIL per sysexits.h to invite the caller to
+	 * retry at some later point and avoid permanent failure */
+	if (notmuch_has_unmatched_db_uuid(notmuch))
+            exit (keep ? EX_TEMPFAIL : EXIT_FAILURE);
+
+        /* Index the message. */
+        status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep);
+
+        /* Commit changes. */
+        close_status = notmuch_database_destroy (notmuch);
+        if (close_status) {
+            /* Hold on to the first error, if any. */
+            if (! status)
+                status = close_status;
+            fprintf (stderr, "%s: failed to commit database changes: %s\n",
+                keep ? "Warning" : "Error",
+                notmuch_status_to_string (close_status));
+	}
     }
 
     if (status) {
diff --git a/notmuch.c b/notmuch.c
index ce6c575..783bb2a 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -220,20 +220,29 @@ be supported in the future.\n", notmuch_format_version);
     }
 }
 
-void
-notmuch_exit_if_unmatched_db_uuid (notmuch_database_t *notmuch)
+notmuch_bool_t
+notmuch_has_unmatched_db_uuid (notmuch_database_t *notmuch)
 {
     const char *uuid = NULL;
 
     if (!notmuch_requested_db_uuid)
-	return;
+	return FALSE;
     IGNORE_RESULT (notmuch_database_get_revision (notmuch, &uuid));
 
     if (strcmp (notmuch_requested_db_uuid, uuid) != 0){
 	fprintf (stderr, "Error: requested database revision %s does not match %s\n",
 		 notmuch_requested_db_uuid, uuid);
-	exit (1);
+	return TRUE;
     }
+
+    return FALSE;
+}
+
+void
+notmuch_exit_if_unmatched_db_uuid (notmuch_database_t *notmuch)
+{
+    if (notmuch_has_unmatched_db_uuid(notmuch))
+	exit (1);
 }
 
 static void
-- 
2.7.0

Re: [PATCH] cli: avoid non-zero exits in notmuch insert --keep

[David Bremner]

Maarten Aertsen <sagi-notmuch@rtsn.nl> writes:

> In the case of any failure, we now return EX_TEMPFAIL (a sendmail
> convention, defined in sysexits.h) to signal to the LDA that it should
> retry later. This prevents a direct reject or bounce of e-mail.

We talked a bit on IRC about the portability of sysexits.h. Afaik, it's
available in GNU systems, on *BSD systems, and on OS/X. We can also
borrow the gnulib copy if we need a compatability version.

> +notmuch_bool_t notmuch_has_unmatched_db_uuid (notmuch_database_t *notmuch);
>  void notmuch_exit_if_unmatched_db_uuid (notmuch_database_t *notmuch);

It's a bit unfortunate to end up with two such functions, but I guess we
could migrate everything to has_unmatched_db_uuid. The double negative
is a bit confusing too, it seems like it would be more natural to have a
_has_matching_db_uuid
> +    status = notmuch_database_open (notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
> +    if (! status) {
> +	/* with keep, send EX_TEMPFAIL per sysexits.h to invite the caller to
> +	 * retry at some later point and avoid permanent failure */
> +	if (notmuch_has_unmatched_db_uuid(notmuch))
> +            exit (keep ? EX_TEMPFAIL : EXIT_FAILURE);

We're a bit fussy about spaces; see devel/STYLE

I'd invite brainstorming about whether mismatched UUID is really a
temporary failure. Unlike a lock, I don't see it going away without
user/admin intervention.  Actually, someone even specifying a UUID when
calling notmuch insert would be a bit surprising.

I suspect some  of the tests in T070-insert.sh could/should be updated
for changed exit values.

d

[PATCH 1/2] cli/insert: delay database open until after writing mail file

[David Bremner]

The idea is to get the mail written to disk, even if we can't open the
database (e.g. because some other process has a write lock, and notmuch
is compiled for non-blocking opens).
---
 notmuch-insert.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 131f09e..862da88 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -532,19 +532,19 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     action.sa_flags = 0;
     sigaction (SIGINT, &action, NULL);
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-	return EXIT_FAILURE;
-
-    notmuch_exit_if_unmatched_db_uuid (notmuch);
-
     /* Write the message to the Maildir new directory. */
     newpath = maildir_write_new (config, STDIN_FILENO, maildir);
     if (! newpath) {
-	notmuch_database_destroy (notmuch);
 	return EXIT_FAILURE;
     }
 
+   if (notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+	return EXIT_FAILURE;
+
+    notmuch_exit_if_unmatched_db_uuid (notmuch);
+
+
     /* Index the message. */
     status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep);
 
-- 
2.10.2

[PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors

[David Bremner]

Attempt to distinguish between errors indicating misconfiguration or
programmer error, which we consider "permanant", in the sense that
automatic retries are unlikely to be useful, and those indicating
transient error conditions. We consider XAPIAN_EXCEPTION transient
because it covers the important special case of locking failure.
---
 notmuch-client.h    |  3 +++
 notmuch-insert.c    |  9 +++++----
 status.c            | 16 ++++++++++++++++
 test/T070-insert.sh | 24 ++++++++++++++----------
 4 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 793f32e..d026e60 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -489,6 +489,9 @@ print_status_database (const char *loc,
 		       const notmuch_database_t *database,
 		       notmuch_status_t status);
 
+int
+status_to_exit (notmuch_status_t status);
+
 #include "command-line-arguments.h"
 
 extern char *notmuch_requested_db_uuid;
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 862da88..a152f15 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -538,9 +538,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-   if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-	return EXIT_FAILURE;
+    status = notmuch_database_open (notmuch_config_get_database_path (config),
+				    NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    if (status)
+	return status_to_exit(status);
 
     notmuch_exit_if_unmatched_db_uuid (notmuch);
 
@@ -577,5 +578,5 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	notmuch_run_hook (db_path, "post-insert");
     }
 
-    return status ? EXIT_FAILURE : EXIT_SUCCESS;
+    return status ? status_to_exit(status) : EXIT_SUCCESS;
 }
diff --git a/status.c b/status.c
index 45d3fb4..8bc2fe4 100644
--- a/status.c
+++ b/status.c
@@ -36,3 +36,19 @@ print_status_database (const char *loc,
     }
     return status;
 }
+
+int
+status_to_exit (notmuch_status_t status)
+{
+    switch (status) {
+    case NOTMUCH_STATUS_SUCCESS:
+	return EXIT_SUCCESS;
+    case NOTMUCH_STATUS_OUT_OF_MEMORY:
+    case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
+    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
+    case NOTMUCH_STATUS_FILE_ERROR:
+	return EX_TEMPFAIL;
+    default:
+	return EXIT_FAILURE;
+    }
+}
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index c2485bb..fd620e5 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -189,7 +189,6 @@ notmuch config set new.tags $OLDCONFIG
 
 for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
     READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
-gen_insert_msg
 cat <<EOF > index-file-$code.gdb
 set breakpoint pending on
 set logging file index-file-$code.log
@@ -201,15 +200,20 @@ continue
 end
 run
 EOF
-test_begin_subtest "error exit when add_message returns $code"
-gdb --batch-silent --return-child-result -x index-file-$code.gdb \
-    --args notmuch insert  < $gen_msg_filename
-test_expect_equal $? 1
-
-test_begin_subtest "success exit with --keep when add_message returns $code"
-gdb --batch-silent --return-child-result -x index-file-$code.gdb \
-    --args notmuch insert --keep  < $gen_msg_filename
-test_expect_equal $? 0
+done
+
+gen_insert_msg
+
+for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
+    test_expect_code 1 "EXIT_FAILURE when add_message returns $code" \
+	"gdb --batch-silent --return-child-result -x index-file-$code.gdb \
+	     --args notmuch insert  < $gen_msg_filename"
+done
+
+for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
+    test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \
+	"gdb --batch-silent --return-child-result -x index-file-$code.gdb \
+	     --args notmuch insert  < $gen_msg_filename"
 done
 
 test_done
-- 
2.10.2

Re: [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors

[David Bremner]

David Bremner <david@tethera.net> writes:

> Attempt to distinguish between errors indicating misconfiguration or
> programmer error, which we consider "permanant", in the sense that
> automatic retries are unlikely to be useful, and those indicating
> transient error conditions. We consider XAPIAN_EXCEPTION transient
> because it covers the important special case of locking failure.
> ---
>  notmuch-client.h    |  3 +++
>  notmuch-insert.c    |  9 +++++----
>  status.c            | 16 ++++++++++++++++
>  test/T070-insert.sh | 24 ++++++++++++++----------
>  4 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 793f32e..d026e60 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -489,6 +489,9 @@ print_status_database (const char *loc,
>  		       const notmuch_database_t *database,
>  		       notmuch_status_t status);
>  
> +int
> +status_to_exit (notmuch_status_t status);
> +
>  #include "command-line-arguments.h"
>  
>  extern char *notmuch_requested_db_uuid;
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 862da88..a152f15 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -538,9 +538,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>  	return EXIT_FAILURE;
>      }
>  
> -   if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> -	return EXIT_FAILURE;
> +    status = notmuch_database_open (notmuch_config_get_database_path (config),
> +				    NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
> +    if (status)
> +	return status_to_exit(status);

I guess this should return NOTMUCH_STATUS_SUCCESS if --keep is passed.

>      notmuch_exit_if_unmatched_db_uuid (notmuch);
>  
> @@ -577,5 +578,5 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>  	notmuch_run_hook (db_path, "post-insert");
>      }
>  
> -    return status ? EXIT_FAILURE : EXIT_SUCCESS;
> +    return status ? status_to_exit(status) : EXIT_SUCCESS;

This can be simplified to just "return status_to_exit (status)".

And whitespace fixes for both calls to status_to_exit.


> +int
> +status_to_exit (notmuch_status_t status)
> +{
> +    switch (status) {
> +    case NOTMUCH_STATUS_SUCCESS:
> +	return EXIT_SUCCESS;
> +    case NOTMUCH_STATUS_OUT_OF_MEMORY:
> +    case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
> +    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
> +    case NOTMUCH_STATUS_FILE_ERROR:
> +	return EX_TEMPFAIL;
> +    default:
> +	return EXIT_FAILURE;
> +    }

This classification is pretty arbitrary. The main goal is have locking
errors, which are currently NOTMUCH_STATUS_XAPIAN_EXCEPTION treated as
TEMPFAIL.

d

Re: [PATCH 1/2] cli/insert: delay database open until after writing mail file

[David Bremner]

David Bremner <david@tethera.net> writes:

> The idea is to get the mail written to disk, even if we can't open the
> database (e.g. because some other process has a write lock, and notmuch
> is compiled for non-blocking opens).
> ---
>  notmuch-insert.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 131f09e..862da88 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -532,19 +532,19 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      action.sa_flags = 0;
>      sigaction (SIGINT, &action, NULL);
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> -	return EXIT_FAILURE;
> -
> -    notmuch_exit_if_unmatched_db_uuid (notmuch);
> -
>      /* Write the message to the Maildir new directory. */
>      newpath = maildir_write_new (config, STDIN_FILENO, maildir);
>      if (! newpath) {
> -	notmuch_database_destroy (notmuch);
>  	return EXIT_FAILURE;
>      }
>  
> +   if (notmuch_database_open (notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +	return EXIT_FAILURE;
> +
> +    notmuch_exit_if_unmatched_db_uuid (notmuch);
> +
> +

One extra newline here.

v2 of insert tempfail series

[David Bremner]

This incorporates Tomi's patch of 

     id:1480367228-22183-1-git-send-email-tomi.ollila@iki.fi

verbatim, to sort out conflicts.

It fixes the issues I alread sent mail about, and puts back the --keep
tests for various error codes.

The interdiff follows; most of it is Tomi's fault :).

diff --git a/notmuch-insert.c b/notmuch-insert.c
index a152f15..bc96af0 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -541,7 +541,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     status = notmuch_database_open (notmuch_config_get_database_path (config),
 				    NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
     if (status)
-	return status_to_exit(status);
+	return keep ? NOTMUCH_STATUS_SUCCESS : status_to_exit (status);
 
     notmuch_exit_if_unmatched_db_uuid (notmuch);
 
@@ -578,5 +578,5 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	notmuch_run_hook (db_path, "post-insert");
     }
 
-    return status ? status_to_exit(status) : EXIT_SUCCESS;
+    return status_to_exit (status);
 }
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index fd620e5..3e7d582 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -206,14 +206,24 @@ gen_insert_msg
 
 for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
     test_expect_code 1 "EXIT_FAILURE when add_message returns $code" \
-	"gdb --batch-silent --return-child-result -x index-file-$code.gdb \
-	     --args notmuch insert  < $gen_msg_filename"
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "success exit with --keep when add_message returns $code" \
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert --keep < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
 done
 
 for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
     test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \
-	"gdb --batch-silent --return-child-result -x index-file-$code.gdb \
-	     --args notmuch insert  < $gen_msg_filename"
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "success exit with --keep when add_message returns $code" \
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert --keep < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
 done
 
 test_done

[PATCH 1/3] test: gdb insert: redirect input inside gdb script

[David Bremner]

From: Tomi Ollila <tomi.ollila@iki.fi>

Running `gdb command < input` is not as reliable way to give input
to the command (some installations of gdb consume it). Use "set args"
gdb command to have input redirected at gdb 'run' time.
---
 test/T070-insert.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index c2485bb..43b3abc 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -202,13 +202,15 @@ end
 run
 EOF
 test_begin_subtest "error exit when add_message returns $code"
-gdb --batch-silent --return-child-result -x index-file-$code.gdb \
-    --args notmuch insert  < $gen_msg_filename
+gdb --batch-silent --return-child-result \
+    -ex "set args insert < $gen_msg_filename" \
+    -x index-file-$code.gdb notmuch
 test_expect_equal $? 1
 
 test_begin_subtest "success exit with --keep when add_message returns $code"
-gdb --batch-silent --return-child-result -x index-file-$code.gdb \
-    --args notmuch insert --keep  < $gen_msg_filename
+gdb --batch-silent --return-child-result \
+    -ex "set args insert --keep < $gen_msg_filename" \
+    -x index-file-$code.gdb notmuch
 test_expect_equal $? 0
 done
 
-- 
2.10.2

[PATCH 2/3] cli/insert: delay database open until after writing mail file

[David Bremner]

The idea is to get the mail written to disk, even if we can't open the
database (e.g. because some other process has a write lock, and notmuch
is compiled for non-blocking opens).
---
 notmuch-insert.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 131f09e..862da88 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -532,19 +532,19 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     action.sa_flags = 0;
     sigaction (SIGINT, &action, NULL);
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-	return EXIT_FAILURE;
-
-    notmuch_exit_if_unmatched_db_uuid (notmuch);
-
     /* Write the message to the Maildir new directory. */
     newpath = maildir_write_new (config, STDIN_FILENO, maildir);
     if (! newpath) {
-	notmuch_database_destroy (notmuch);
 	return EXIT_FAILURE;
     }
 
+   if (notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+	return EXIT_FAILURE;
+
+    notmuch_exit_if_unmatched_db_uuid (notmuch);
+
+
     /* Index the message. */
     status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep);
 
-- 
2.10.2

[PATCH 3/3] cli/insert: return EX_TEMPFAIL for some errors

[David Bremner]

Attempt to distinguish between errors indicating misconfiguration or
programmer error, which we consider "permanent", in the sense that
automatic retries are unlikely to be useful, and those indicating
transient error conditions. We consider XAPIAN_EXCEPTION transient
because it covers the important special case of locking failure.
---
 notmuch-client.h    |  3 +++
 notmuch-insert.c    |  9 +++++----
 status.c            | 16 ++++++++++++++++
 test/T070-insert.sh | 36 ++++++++++++++++++++++++------------
 4 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 793f32e..d026e60 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -489,6 +489,9 @@ print_status_database (const char *loc,
 		       const notmuch_database_t *database,
 		       notmuch_status_t status);
 
+int
+status_to_exit (notmuch_status_t status);
+
 #include "command-line-arguments.h"
 
 extern char *notmuch_requested_db_uuid;
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 862da88..bc96af0 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -538,9 +538,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-   if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-	return EXIT_FAILURE;
+    status = notmuch_database_open (notmuch_config_get_database_path (config),
+				    NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    if (status)
+	return keep ? NOTMUCH_STATUS_SUCCESS : status_to_exit (status);
 
     notmuch_exit_if_unmatched_db_uuid (notmuch);
 
@@ -577,5 +578,5 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	notmuch_run_hook (db_path, "post-insert");
     }
 
-    return status ? EXIT_FAILURE : EXIT_SUCCESS;
+    return status_to_exit (status);
 }
diff --git a/status.c b/status.c
index 45d3fb4..8bc2fe4 100644
--- a/status.c
+++ b/status.c
@@ -36,3 +36,19 @@ print_status_database (const char *loc,
     }
     return status;
 }
+
+int
+status_to_exit (notmuch_status_t status)
+{
+    switch (status) {
+    case NOTMUCH_STATUS_SUCCESS:
+	return EXIT_SUCCESS;
+    case NOTMUCH_STATUS_OUT_OF_MEMORY:
+    case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
+    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
+    case NOTMUCH_STATUS_FILE_ERROR:
+	return EX_TEMPFAIL;
+    default:
+	return EXIT_FAILURE;
+    }
+}
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 43b3abc..3e7d582 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -189,7 +189,6 @@ notmuch config set new.tags $OLDCONFIG
 
 for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
     READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
-gen_insert_msg
 cat <<EOF > index-file-$code.gdb
 set breakpoint pending on
 set logging file index-file-$code.log
@@ -201,17 +200,30 @@ continue
 end
 run
 EOF
-test_begin_subtest "error exit when add_message returns $code"
-gdb --batch-silent --return-child-result \
-    -ex "set args insert < $gen_msg_filename" \
-    -x index-file-$code.gdb notmuch
-test_expect_equal $? 1
-
-test_begin_subtest "success exit with --keep when add_message returns $code"
-gdb --batch-silent --return-child-result \
-    -ex "set args insert --keep < $gen_msg_filename" \
-    -x index-file-$code.gdb notmuch
-test_expect_equal $? 0
+done
+
+gen_insert_msg
+
+for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
+    test_expect_code 1 "EXIT_FAILURE when add_message returns $code" \
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "success exit with --keep when add_message returns $code" \
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert --keep < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
+done
+
+for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
+    test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
+    test_expect_code 0 "success exit with --keep when add_message returns $code" \
+         "gdb --batch-silent --return-child-result \
+	     -ex \"set args insert --keep < $gen_msg_filename\" \
+	     -x index-file-$code.gdb notmuch"
 done
 
 test_done
-- 
2.10.2

Re: v2 of insert tempfail series

[Tomi Ollila]

On Tue, Nov 29 2016, David Bremner <david@tethera.net> wrote:

> This incorporates Tomi's patch of 
>
>      id:1480367228-22183-1-git-send-email-tomi.ollila@iki.fi
>
> verbatim, to sort out conflicts.
>
> It fixes the issues I alread sent mail about, and puts back the --keep
> tests for various error codes.
>
> The interdiff follows; most of it is Tomi's fault :).

This series looks good to me -- also my part which I had a slight
afterthought when I looked what brew install gdb printed out -- it
suggested setting 'startup-with-shell off' which would have borken
my version (on macOS), but

http://apple.stackexchange.com/questions/246245/macos-sierra-gdb-not-codesigned

informed that setting startup-with-shell has so far not been necessary
(and it would be major pain everywhere if it were required; in addition to
everything else using `exec-wrapper` also requires shell...)

I.e. if it makes gdb use more robust in general that is good reason to use it.


Last (and least) (i.e. I could not resist the templation): the code block
below would be simpler in this format -- but perhaps what happends there is
less understandable -- although now the where the expansion is done is more
visible ;)

       +    test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \
       +         "gdb --batch-silent --return-child-result \
       +	     -ex 'set args insert < $gen_msg_filename' \
       +	     -x index-file-$code.gdb notmuch"

(all variable expansions are done here (in context of the outermost "gdb...")
and not in the `eval` that is done by test_expect_code -- unless any of the
variables expanded to yet another variable syntax ;)

Tomi

>
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index a152f15..bc96af0 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -541,7 +541,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      status = notmuch_database_open (notmuch_config_get_database_path (config),
>  				    NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
>      if (status)
> -	return status_to_exit(status);
> +	return keep ? NOTMUCH_STATUS_SUCCESS : status_to_exit (status);
>  
>      notmuch_exit_if_unmatched_db_uuid (notmuch);
>  
> @@ -578,5 +578,5 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>  	notmuch_run_hook (db_path, "post-insert");
>      }
>  
> -    return status ? status_to_exit(status) : EXIT_SUCCESS;
> +    return status_to_exit (status);
>  }
> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index fd620e5..3e7d582 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -206,14 +206,24 @@ gen_insert_msg
>  
>  for code in  FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
>      test_expect_code 1 "EXIT_FAILURE when add_message returns $code" \
> -	"gdb --batch-silent --return-child-result -x index-file-$code.gdb \
> -	     --args notmuch insert  < $gen_msg_filename"
> +         "gdb --batch-silent --return-child-result \
> +	     -ex \"set args insert < $gen_msg_filename\" \
> +	     -x index-file-$code.gdb notmuch"
> +    test_expect_code 0 "success exit with --keep when add_message returns $code" \
> +         "gdb --batch-silent --return-child-result \
> +	     -ex \"set args insert --keep < $gen_msg_filename\" \
> +	     -x index-file-$code.gdb notmuch"
>  done
>  
>  for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do
>      test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \
> -	"gdb --batch-silent --return-child-result -x index-file-$code.gdb \
> -	     --args notmuch insert  < $gen_msg_filename"
> +         "gdb --batch-silent --return-child-result \
> +	     -ex \"set args insert < $gen_msg_filename\" \
> +	     -x index-file-$code.gdb notmuch"
> +    test_expect_code 0 "success exit with --keep when add_message returns $code" \
> +         "gdb --batch-silent --return-child-result \
> +	     -ex \"set args insert --keep < $gen_msg_filename\" \
> +	     -x index-file-$code.gdb notmuch"
>  done
>  
>  test_done

NEWS/docs for insert tempfail changes

[David Bremner]

I went back and forth few times, but eventually decided to go with
Tomi's version of the test quoting, the better to share the blame.
The patches will be merged to release and master, in prep for another
bugfix release.  Here are some proposed news and manpage changes.

[PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL

[David Bremner]

---
 doc/man1/notmuch-insert.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
index 9e7afc3..9847e67 100644
--- a/doc/man1/notmuch-insert.rst
+++ b/doc/man1/notmuch-insert.rst
@@ -60,6 +60,13 @@ indexing to Notmuch database, changing tags, and synchronizing tags to
 maildir flags. The ``--keep`` option may be used to settle for
 successful message file delivery.
 
+This command supports the following special exit status code for
+errors most likely to be temporary in nature, e.g. failure to get a
+database write lock.
+
+``75 (EX_TEMPFAIL)``
+    A temporary failure occured; the user is invited to retry.
+
 The exit status of the **post-insert** hook does not affect the exit
 status of the **insert** command.
 
-- 
2.10.2

[PATCH 2/2] NEWS: news for notmuch-insert error handling

[David Bremner]

---
 NEWS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/NEWS b/NEWS
index 99ef277..c0aa479 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,15 @@
+Notmuch 0.23.4 (UNRELEASED)
+===========================
+
+Command Line Interface
+----------------------
+
+Improve error handling in notmuch insert
+
+  Database lock errors no longer prevent message file delivery to the
+  filesystem.  Certain errors during `notmuch insert` most likely to
+  be temporary return EX_TEMPFAIL.
+
 Notmuch 0.23.3 (2016-11-27)
 ===========================
 
-- 
2.10.2

Re: NEWS/docs for insert tempfail changes

[Tomi Ollila]

On Wed, Dec 07 2016, David Bremner <david@tethera.net> wrote:

> I went back and forth few times, but eventually decided to go with
> Tomi's version of the test quoting, the better to share the blame.
> The patches will be merged to release and master, in prep for another
> bugfix release.  Here are some proposed news and manpage changes.

From the (content and) formatting point of view these look good.

Tomi

Re: [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL

[David Bremner]

David Bremner <david@tethera.net> writes:

> ---
>  doc/man1/notmuch-insert.rst | 7 +++++++

series pushed to release and master