* notmuch insert --keep fails in parallel w/ notmuch new @ 2016-01-24 15:48 Maarten Aertsen 2016-01-26 15:09 ` David Bremner 0 siblings, 1 reply; 18+ messages in thread From: Maarten Aertsen @ 2016-01-24 15:48 UTC (permalink / raw) To: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: notmuch insert --keep fails in parallel w/ notmuch new 2016-01-24 15:48 notmuch insert --keep fails in parallel w/ notmuch new Maarten Aertsen @ 2016-01-26 15:09 ` David Bremner 2016-02-07 21:13 ` [PATCH] cli: avoid non-zero exits in notmuch insert --keep Maarten Aertsen 0 siblings, 1 reply; 18+ messages in thread From: David Bremner @ 2016-01-26 15:09 UTC (permalink / raw) To: Maarten Aertsen, Notmuch Mail 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] cli: avoid non-zero exits in notmuch insert --keep 2016-01-26 15:09 ` David Bremner @ 2016-02-07 21:13 ` Maarten Aertsen 2016-02-08 11:56 ` David Bremner 0 siblings, 1 reply; 18+ messages in thread From: Maarten Aertsen @ 2016-02-07 21:13 UTC (permalink / raw) To: Notmuch Mail 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, ¬much)) - 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, ¬much); + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] cli: avoid non-zero exits in notmuch insert --keep 2016-02-07 21:13 ` [PATCH] cli: avoid non-zero exits in notmuch insert --keep Maarten Aertsen @ 2016-02-08 11:56 ` David Bremner 2016-11-28 12:16 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner 0 siblings, 1 reply; 18+ messages in thread From: David Bremner @ 2016-02-08 11:56 UTC (permalink / raw) To: Maarten Aertsen, Notmuch Mail 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, ¬much); > + 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] cli/insert: delay database open until after writing mail file 2016-02-08 11:56 ` David Bremner @ 2016-11-28 12:16 ` David Bremner 2016-11-28 12:16 ` [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors David Bremner 2016-11-28 12:23 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner 0 siblings, 2 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 12:16 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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, ¬much)) - 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, ¬much)) + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors 2016-11-28 12:16 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner @ 2016-11-28 12:16 ` David Bremner 2016-11-28 12:22 ` David Bremner 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner 2016-11-28 12:23 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner 1 sibling, 2 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 12:16 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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, ¬much)) - return EXIT_FAILURE; + status = notmuch_database_open (notmuch_config_get_database_path (config), + NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much); + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors 2016-11-28 12:16 ` [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors David Bremner @ 2016-11-28 12:22 ` David Bremner 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner 1 sibling, 0 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 12:22 UTC (permalink / raw) To: Maarten Aertsen, Notmuch Mail 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, ¬much)) > - return EXIT_FAILURE; > + status = notmuch_database_open (notmuch_config_get_database_path (config), > + NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much); > + 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* v2 of insert tempfail series 2016-11-28 12:16 ` [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors David Bremner 2016-11-28 12:22 ` David Bremner @ 2016-11-28 22:12 ` David Bremner 2016-11-28 22:12 ` [PATCH 1/3] test: gdb insert: redirect input inside gdb script David Bremner ` (3 more replies) 1 sibling, 4 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 22:12 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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, ¬much); 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/3] test: gdb insert: redirect input inside gdb script 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner @ 2016-11-28 22:12 ` David Bremner 2016-11-28 22:12 ` [PATCH 2/3] cli/insert: delay database open until after writing mail file David Bremner ` (2 subsequent siblings) 3 siblings, 0 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 22:12 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail; +Cc: Tomi Ollila 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] cli/insert: delay database open until after writing mail file 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner 2016-11-28 22:12 ` [PATCH 1/3] test: gdb insert: redirect input inside gdb script David Bremner @ 2016-11-28 22:12 ` David Bremner 2016-11-28 22:12 ` [PATCH 3/3] cli/insert: return EX_TEMPFAIL for some errors David Bremner 2016-12-04 20:51 ` v2 of insert tempfail series Tomi Ollila 3 siblings, 0 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 22:12 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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, ¬much)) - 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, ¬much)) + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] cli/insert: return EX_TEMPFAIL for some errors 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner 2016-11-28 22:12 ` [PATCH 1/3] test: gdb insert: redirect input inside gdb script David Bremner 2016-11-28 22:12 ` [PATCH 2/3] cli/insert: delay database open until after writing mail file David Bremner @ 2016-11-28 22:12 ` David Bremner 2016-12-04 20:51 ` v2 of insert tempfail series Tomi Ollila 3 siblings, 0 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 22:12 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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, ¬much)) - return EXIT_FAILURE; + status = notmuch_database_open (notmuch_config_get_database_path (config), + NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much); + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: v2 of insert tempfail series 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner ` (2 preceding siblings ...) 2016-11-28 22:12 ` [PATCH 3/3] cli/insert: return EX_TEMPFAIL for some errors David Bremner @ 2016-12-04 20:51 ` Tomi Ollila 2016-12-07 11:27 ` NEWS/docs for insert tempfail changes David Bremner 3 siblings, 1 reply; 18+ messages in thread From: Tomi Ollila @ 2016-12-04 20:51 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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, ¬much); > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* NEWS/docs for insert tempfail changes 2016-12-04 20:51 ` v2 of insert tempfail series Tomi Ollila @ 2016-12-07 11:27 ` David Bremner 2016-12-07 11:27 ` [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL David Bremner ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: David Bremner @ 2016-12-07 11:27 UTC (permalink / raw) To: Tomi Ollila, David Bremner, Maarten Aertsen, Notmuch Mail 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL 2016-12-07 11:27 ` NEWS/docs for insert tempfail changes David Bremner @ 2016-12-07 11:27 ` David Bremner 2016-12-15 13:01 ` David Bremner 2016-12-07 11:27 ` [PATCH 2/2] NEWS: news for notmuch-insert error handling David Bremner 2016-12-10 19:26 ` NEWS/docs for insert tempfail changes Tomi Ollila 2 siblings, 1 reply; 18+ messages in thread From: David Bremner @ 2016-12-07 11:27 UTC (permalink / raw) To: Tomi Ollila, David Bremner, Maarten Aertsen, Notmuch Mail --- 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL 2016-12-07 11:27 ` [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL David Bremner @ 2016-12-15 13:01 ` David Bremner 0 siblings, 0 replies; 18+ messages in thread From: David Bremner @ 2016-12-15 13:01 UTC (permalink / raw) To: Tomi Ollila, Maarten Aertsen, Notmuch Mail David Bremner <david@tethera.net> writes: > --- > doc/man1/notmuch-insert.rst | 7 +++++++ series pushed to release and master ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] NEWS: news for notmuch-insert error handling 2016-12-07 11:27 ` NEWS/docs for insert tempfail changes David Bremner 2016-12-07 11:27 ` [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL David Bremner @ 2016-12-07 11:27 ` David Bremner 2016-12-10 19:26 ` NEWS/docs for insert tempfail changes Tomi Ollila 2 siblings, 0 replies; 18+ messages in thread From: David Bremner @ 2016-12-07 11:27 UTC (permalink / raw) To: Tomi Ollila, David Bremner, Maarten Aertsen, Notmuch Mail --- 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: NEWS/docs for insert tempfail changes 2016-12-07 11:27 ` NEWS/docs for insert tempfail changes David Bremner 2016-12-07 11:27 ` [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL David Bremner 2016-12-07 11:27 ` [PATCH 2/2] NEWS: news for notmuch-insert error handling David Bremner @ 2016-12-10 19:26 ` Tomi Ollila 2 siblings, 0 replies; 18+ messages in thread From: Tomi Ollila @ 2016-12-10 19:26 UTC (permalink / raw) To: David Bremner, Maarten Aertsen, Notmuch Mail 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] cli/insert: delay database open until after writing mail file 2016-11-28 12:16 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner 2016-11-28 12:16 ` [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors David Bremner @ 2016-11-28 12:23 ` David Bremner 1 sibling, 0 replies; 18+ messages in thread From: David Bremner @ 2016-11-28 12:23 UTC (permalink / raw) To: Maarten Aertsen, Notmuch Mail 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, ¬much)) > - 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, ¬much)) > + return EXIT_FAILURE; > + > + notmuch_exit_if_unmatched_db_uuid (notmuch); > + > + One extra newline here. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-12-15 13:01 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-24 15:48 notmuch insert --keep fails in parallel w/ notmuch new Maarten Aertsen 2016-01-26 15:09 ` David Bremner 2016-02-07 21:13 ` [PATCH] cli: avoid non-zero exits in notmuch insert --keep Maarten Aertsen 2016-02-08 11:56 ` David Bremner 2016-11-28 12:16 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner 2016-11-28 12:16 ` [PATCH 2/2] cli/insert: return EX_TEMPFAIL for some errors David Bremner 2016-11-28 12:22 ` David Bremner 2016-11-28 22:12 ` v2 of insert tempfail series David Bremner 2016-11-28 22:12 ` [PATCH 1/3] test: gdb insert: redirect input inside gdb script David Bremner 2016-11-28 22:12 ` [PATCH 2/3] cli/insert: delay database open until after writing mail file David Bremner 2016-11-28 22:12 ` [PATCH 3/3] cli/insert: return EX_TEMPFAIL for some errors David Bremner 2016-12-04 20:51 ` v2 of insert tempfail series Tomi Ollila 2016-12-07 11:27 ` NEWS/docs for insert tempfail changes David Bremner 2016-12-07 11:27 ` [PATCH 1/2] cli/insert: document the use of EX_TEMPFAIL David Bremner 2016-12-15 13:01 ` David Bremner 2016-12-07 11:27 ` [PATCH 2/2] NEWS: news for notmuch-insert error handling David Bremner 2016-12-10 19:26 ` NEWS/docs for insert tempfail changes Tomi Ollila 2016-11-28 12:23 ` [PATCH 1/2] cli/insert: delay database open until after writing mail file David Bremner
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).