[PATCH v2 0/3] notmuch insert failure paths and post-insert hook

[PATCH v2 0/3] notmuch insert failure paths and post-insert hook

[Jani Nikula]

This is v2 of the final three patches of [1]. The failure paths have
been improved further still, with some new warning messages on any
cleanup errors as well. Man pages have been updated.

Tests are still lacking. The post-insert hook should be simple, but I
haven't had a sudden outburst of creativity in figuring out how to test
all the fail scenarios.

BR,
Jani.


[1] id:cover.1411379395.git.jani@nikula.org

Jani Nikula (3):
  cli/insert: add fail path to add_file_to_database
  cli/insert: require succesful message indexing for success status
  cli/insert: add post-insert hook

 doc/man1/notmuch-insert.rst |  26 ++++++--
 doc/man5/notmuch-hooks.rst  |  11 ++++
 notmuch-insert.c            | 140 +++++++++++++++++++++++++++++++-------------
 test/T070-insert.sh         |   2 +-
 4 files changed, 132 insertions(+), 47 deletions(-)

-- 
2.1.0

[PATCH v2 1/3] cli/insert: add fail path to add_file_to_database

[Jani Nikula]

Handle failures gracefully in add_file_to_database, renamed simply
add_file while at it. Add keep option to not remove the message from
database if tagging or tag syncing to maildir flags fails. Expand the
function documentation to cover the changes.
---
 notmuch-insert.c | 99 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 35 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5ef6e66d86e2..0ea438015dbe 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -364,50 +364,80 @@ FAIL:
     return NULL;
 }
 
-/* Add the specified message file to the notmuch database, applying tags.
- * The file is renamed to encode notmuch tags as maildir flags. */
-static void
-add_file_to_database (notmuch_database_t *notmuch, const char *path,
-		      tag_op_list_t *tag_ops, notmuch_bool_t synchronize_flags)
+/*
+ * Add the specified message file to the notmuch database, applying
+ * tags in tag_ops. If synchronize_flags is TRUE, the tags are
+ * synchronized to maildir flags (which may result in message file
+ * rename).
+ *
+ * Return NOTMUCH_STATUS_SUCCESS on success, errors otherwise. If keep
+ * is TRUE, errors in tag changes and flag syncing are ignored and
+ * success status is returned; otherwise such errors cause the message
+ * to be removed from the database. Failure to add the message to the
+ * database results in error status regardless of keep.
+ */
+static notmuch_status_t
+add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
+	  notmuch_bool_t synchronize_flags, notmuch_bool_t keep)
 {
     notmuch_message_t *message;
     notmuch_status_t status;
 
     status = notmuch_database_add_message (notmuch, path, &message);
-    switch (status) {
-    case NOTMUCH_STATUS_SUCCESS:
-    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-	break;
-    default:
-    case NOTMUCH_STATUS_FILE_NOT_EMAIL:
-    case NOTMUCH_STATUS_READ_ONLY_DATABASE:
-    case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
-    case NOTMUCH_STATUS_OUT_OF_MEMORY:
-    case NOTMUCH_STATUS_FILE_ERROR:
-    case NOTMUCH_STATUS_NULL_POINTER:
-    case NOTMUCH_STATUS_TAG_TOO_LONG:
-    case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
-    case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
-    case NOTMUCH_STATUS_LAST_STATUS:
-	fprintf (stderr, "Error: failed to add `%s' to notmuch database: %s\n",
-		 path, notmuch_status_to_string (status));
-	return;
-    }
-
-    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
-	/* Don't change tags of an existing message. */
-	if (synchronize_flags) {
-	    status = notmuch_message_tags_to_maildir_flags (message);
-	    if (status != NOTMUCH_STATUS_SUCCESS)
-		fprintf (stderr, "Error: failed to sync tags to maildir flags\n");
+    if (status == NOTMUCH_STATUS_SUCCESS) {
+	status = tag_op_list_apply (message, tag_ops, 0);
+	if (status) {
+	    fprintf (stderr, "%s: failed to apply tags to file '%s': %s\n",
+		     keep ? "Warning" : "Error",
+		     path, notmuch_status_to_string (status));
+	    goto DONE;
 	}
+    } else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
+	status = NOTMUCH_STATUS_SUCCESS;
+    } else if (status == NOTMUCH_STATUS_FILE_NOT_EMAIL) {
+	fprintf (stderr, "Error: delivery of non-mail file: '%s'\n", path);
+	goto FAIL;
     } else {
-	tag_op_flag_t flags = synchronize_flags ? TAG_FLAG_MAILDIR_SYNC : 0;
+	fprintf (stderr, "Error: failed to add '%s' to notmuch database: %s\n",
+		 path, notmuch_status_to_string (status));
+	goto FAIL;
+    }
 
-	tag_op_list_apply (message, tag_ops, flags);
+    if (synchronize_flags) {
+	status = notmuch_message_tags_to_maildir_flags (message);
+	if (status != NOTMUCH_STATUS_SUCCESS)
+	    fprintf (stderr, "%s: failed to sync tags to maildir flags for '%s': %s\n",
+		     keep ? "Warning" : "Error",
+		     path, notmuch_status_to_string (status));
+
+	/*
+	 * Note: Unfortunately a failed maildir flag sync might
+	 * already have renamed the file, in which case the cleanup
+	 * path may fail.
+	 */
     }
 
+  DONE:
     notmuch_message_destroy (message);
+
+    if (status) {
+	if (keep) {
+	    status = NOTMUCH_STATUS_SUCCESS;
+	} else {
+	    notmuch_status_t cleanup_status;
+
+	    cleanup_status = notmuch_database_remove_message (notmuch, path);
+	    if (cleanup_status != NOTMUCH_STATUS_SUCCESS &&
+		cleanup_status != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
+		fprintf (stderr, "Warning: failed to remove '%s' from database "
+			 "after errors: %s. Please run 'notmuch new' to fix.\n",
+			 path, notmuch_status_to_string (cleanup_status));
+	    }
+	}
+    }
+
+  FAIL:
+    return status;
 }
 
 int
@@ -508,8 +538,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     /* Add the message to the index.
      * Even if adding the message to the notmuch database fails,
      * the message is on disk and we consider the delivery completed. */
-    add_file_to_database (notmuch, newpath, tag_ops,
-				    synchronize_flags);
+    add_file (notmuch, newpath, tag_ops, synchronize_flags, TRUE);
 
     notmuch_database_destroy (notmuch);
     return EXIT_SUCCESS;
-- 
2.1.0

[PATCH v2 2/3] cli/insert: require succesful message indexing for success status

[Jani Nikula]

Add --keep option to keep any remaining stuff in index or file. We
could distinguish between failures to index and failures to apply tags
or maildir sync, but for simplicity just have one.
---
 doc/man1/notmuch-insert.rst | 19 ++++++++++++-------
 notmuch-insert.c            | 36 ++++++++++++++++++++++++++++++------
 test/T070-insert.sh         |  2 +-
 3 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
index 2be1a7b8841c..e396f6cf2279 100644
--- a/doc/man1/notmuch-insert.rst
+++ b/doc/man1/notmuch-insert.rst
@@ -38,16 +38,21 @@ Supported options for **insert** include
         does not exist. Otherwise the folder must already exist for mail
         delivery to succeed.
 
+    ``--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.
+
 EXIT STATUS
 ===========
 
-This command returns exit status 0 if the message was successfully added
-to the mail directory, even if the message could not be indexed and
-added to the notmuch database. In the latter case, a warning will be
-printed to standard error but the message file will be left on disk.
-
-If the message could not be written to disk then a non-zero exit status
-is returned.
+This command returns exit status 0 on succesful mail delivery,
+non-zero otherwise. The default is to indicate failed mail delivery on
+any errors, including message file delivery to the filesystem, message
+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.
 
 SEE ALSO
 ========
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 0ea438015dbe..7074077ca699 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -443,6 +443,7 @@ add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
 int
 notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 {
+    notmuch_status_t status, close_status;
     notmuch_database_t *notmuch;
     struct sigaction action;
     const char *db_path;
@@ -452,6 +453,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     char *query_string = NULL;
     const char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
+    notmuch_bool_t keep = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
@@ -461,6 +463,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -535,11 +538,32 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-    /* Add the message to the index.
-     * Even if adding the message to the notmuch database fails,
-     * the message is on disk and we consider the delivery completed. */
-    add_file (notmuch, newpath, tag_ops, synchronize_flags, TRUE);
+    /* 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) {
+	if (keep) {
+	    status = NOTMUCH_STATUS_SUCCESS;
+	} else {
+	    /* If maildir flag sync failed, this might fail. */
+	    if (unlink (newpath)) {
+		fprintf (stderr, "Warning: failed to remove '%s' from maildir "
+			 "after errors: %s. Please run 'notmuch new' to fix.\n",
+			 newpath, strerror (errno));
+	    }
+	}
+    }
 
-    notmuch_database_destroy (notmuch);
-    return EXIT_SUCCESS;
+    return status ? EXIT_FAILURE : EXIT_SUCCESS;
 }
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index ea9db07e2fa2..aacc643b7288 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -23,7 +23,7 @@ test_expect_code 1 "Insert zero-length file" \
 
 # This test is a proxy for other errors that may occur while trying to
 # add a message to the notmuch database, e.g. database locked.
-test_expect_code 0 "Insert non-message" \
+test_expect_code 1 "Insert non-message" \
     "echo bad_message | notmuch insert"
 
 test_begin_subtest "Database empty so far"
-- 
2.1.0

[PATCH v2 3/3] cli/insert: add post-insert hook

[Jani Nikula]

The post-new hook might no longer be needed or run very often if
notmuch insert is being used. Therefore a post-insert hook is needed
(arguably pre-insert not so much, so don't add one). Also add the
--no-hooks option to skip hooks.
---
 doc/man1/notmuch-insert.rst |  9 +++++++++
 doc/man5/notmuch-hooks.rst  | 11 +++++++++++
 notmuch-insert.c            |  7 +++++++
 3 files changed, 27 insertions(+)

diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
index e396f6cf2279..2c9c0d02c251 100644
--- a/doc/man1/notmuch-insert.rst
+++ b/doc/man1/notmuch-insert.rst
@@ -25,6 +25,9 @@ If the new message is a duplicate of an existing message in the database
 (it has same Message-ID), it will be added to the maildir folder and
 notmuch database, but the tags will not be changed.
 
+The **insert** command supports hooks. See **notmuch-hooks(5)** for
+more details on hooks.
+
 Option arguments must appear before any tag operation arguments.
 Supported options for **insert** include
 
@@ -44,6 +47,9 @@ Supported options for **insert** include
         fails. Ignore these errors and return exit status 0 to
         indicate succesful mail delivery.
 
+    ``--no-hooks``
+        Prevent hooks from being run.
+
 EXIT STATUS
 ===========
 
@@ -54,6 +60,9 @@ 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.
 
+The exit status of the **post-insert** hook does not affect the exit
+status of the **insert** command.
+
 SEE ALSO
 ========
 
diff --git a/doc/man5/notmuch-hooks.rst b/doc/man5/notmuch-hooks.rst
index 493abf20f003..f1c2528c5666 100644
--- a/doc/man5/notmuch-hooks.rst
+++ b/doc/man5/notmuch-hooks.rst
@@ -35,6 +35,17 @@ The currently available hooks are described below.
         Typically this hook is used to perform additional query-based
         tagging on the imported messages.
 
+    **post-insert**
+
+        This hook is invoked by the **insert** command after the
+        message has been delivered, added to the database, and initial
+        tags have been applied. The hook will not be run if there have
+        been any errors during the message delivery; what is regarded
+        as succesful delivery depends on the ``--keep`` option.
+
+        Typically this hook is used to perform additional query-based
+        tagging on the delivered messages.
+
 SEE ALSO
 ========
 
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 7074077ca699..e9d71efa1fb6 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -454,6 +454,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     const char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
     notmuch_bool_t keep = FALSE;
+    notmuch_bool_t no_hooks = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
@@ -464,6 +465,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -565,5 +567,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	}
     }
 
+    if (! no_hooks && status == NOTMUCH_STATUS_SUCCESS) {
+	/* Ignore hook failures. */
+	notmuch_run_hook (db_path, "post-insert");
+    }
+
     return status ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
2.1.0

[PATCH] WIP: new insert tests for failing indexing.

[David Bremner]

The second one fails because the initial write lock fails.
I guess this is maybe a doc bug.
---
 test/T070-insert.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

This is what I came up with when trying to induce indexing
failure. The best I could do was make the initial notmuch open fail.
This means my second test is broken.  When I was writing the commit
message I was thinking the man page was therefore wrong, but actually
it's ok.

I did wonder if it might be a good idea to move the database open
after the message is written to disk, is we want to maximize the
chance of insert --keep "succeeding".

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index aacc643..e7d0ae1 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -183,4 +183,25 @@ test_expect_code 1 "Invalid tags set exit code" \
 
 notmuch config set new.tags $OLDCONFIG
 
+
+gen_insert_msg
+test_python <<EOF &
+import notmuch, time, os
+with open("write-locker.pid","w") as pidfile:
+    pidfile.write(str(os.getpid()))
+db = notmuch.Database(mode=notmuch.Database.MODE.READ_WRITE)
+time.sleep(120)
+EOF
+
+# give time for the background process to get started
+sleep 1
+
+test_expect_code 1 "Non-zero exit code due to write lock" \
+    'notmuch insert < "$gen_msg_filename" 2>&1'
+
+test_expect_code 0 "Zero exit code due to --keep flag" \
+    'notmuch insert --keep < "$gen_msg_filename" 2>&1'
+
+kill $(cat ${TMP_DIRECTORY}/write-locker.pid) >/dev/null 2>&1
+
 test_done
-- 
2.1.0

[PATCH] test: add simple tests for post-insert hook

[David Bremner]

Most of the existing tests for pre/post-new hook don't seem to apply.
---
 test/T400-hooks.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/T400-hooks.sh b/test/T400-hooks.sh
index 77e8569..e741211 100755
--- a/test/T400-hooks.sh
+++ b/test/T400-hooks.sh
@@ -30,6 +30,8 @@ rm_hooks () {
 
 # add a message to generate mail dir and database
 add_message
+# create maildir structure for notmuch-insert
+mkdir -p "$MAIL_DIR"/{cur,new,tmp}
 
 test_begin_subtest "pre-new is run"
 rm_hooks
@@ -45,6 +47,16 @@ create_echo_hook "post-new" expected output
 notmuch new > /dev/null
 test_expect_equal_file expected output
 
+test_begin_subtest "post-insert hook is run"
+rm_hooks
+generate_message
+create_echo_hook "post-insert" expected output
+echo $gen_msg_filename
+cat output
+notmuch insert < "$gen_msg_filename"
+echo $?
+test_expect_equal_file expected output
+
 test_begin_subtest "pre-new is run before post-new"
 rm_hooks
 generate_message
@@ -82,6 +94,12 @@ test_expect_equal_file expected output
 # depends on the previous subtest leaving broken hook behind
 test_expect_code 1 "post-new non-zero exit status (notmuch status)" "notmuch new"
 
+rm_hooks
+generate_message
+create_failing_hook "post-insert"
+test_expect_success "post-insert hook does not affect insert status" \
+    "notmuch insert < \"$gen_msg_filename\" > /dev/null"
+
 # test_begin_subtest "hook without executable permissions"
 rm_hooks
 mkdir -p ${HOOK_DIR}
-- 
2.1.1

Re: [PATCH] test: add simple tests for post-insert hook

[Jani Nikula]

On Sat, 18 Oct 2014, David Bremner <david@tethera.net> wrote:
> Most of the existing tests for pre/post-new hook don't seem to apply.
> ---
>  test/T400-hooks.sh | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/test/T400-hooks.sh b/test/T400-hooks.sh
> index 77e8569..e741211 100755
> --- a/test/T400-hooks.sh
> +++ b/test/T400-hooks.sh
> @@ -30,6 +30,8 @@ rm_hooks () {
>  
>  # add a message to generate mail dir and database
>  add_message
> +# create maildir structure for notmuch-insert
> +mkdir -p "$MAIL_DIR"/{cur,new,tmp}
>  
>  test_begin_subtest "pre-new is run"
>  rm_hooks
> @@ -45,6 +47,16 @@ create_echo_hook "post-new" expected output
>  notmuch new > /dev/null
>  test_expect_equal_file expected output
>  
> +test_begin_subtest "post-insert hook is run"
> +rm_hooks
> +generate_message
> +create_echo_hook "post-insert" expected output
> +echo $gen_msg_filename
> +cat output

I presume the two lines above...

> +notmuch insert < "$gen_msg_filename"
> +echo $?

...and this line are leftover debug messages?

Otherwise LGTM. I guess this could be expanded with a subtest checking
that the hook does not get run on errors, and does get run with some
errors that are ignored with --keep.

BR,
Jani.

> +test_expect_equal_file expected output
> +
>  test_begin_subtest "pre-new is run before post-new"
>  rm_hooks
>  generate_message
> @@ -82,6 +94,12 @@ test_expect_equal_file expected output
>  # depends on the previous subtest leaving broken hook behind
>  test_expect_code 1 "post-new non-zero exit status (notmuch status)" "notmuch new"
>  
> +rm_hooks
> +generate_message
> +create_failing_hook "post-insert"
> +test_expect_success "post-insert hook does not affect insert status" \
> +    "notmuch insert < \"$gen_msg_filename\" > /dev/null"
> +
>  # test_begin_subtest "hook without executable permissions"
>  rm_hooks
>  mkdir -p ${HOOK_DIR}
> -- 
> 2.1.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] test: add simple tests for post-insert hook

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

>
> I presume the two lines above...
>
>> +notmuch insert < "$gen_msg_filename"
>> +echo $?
>
> ...and this line are leftover debug messages?

Uh, yeah. I should have fixed those a while ago when Tomi pointed them
out.

>
> Otherwise LGTM. I guess this could be expanded with a subtest checking
> that the hook does not get run on errors, and does get run with some
> errors that are ignored with --keep.

OK, I pushed these tests for now (less the debugging output).

d