Fix ultimate retry timeouts for intermittently deliverable recipients.

When a queue runner is handling a message, Exim first routes the
recipient addresses, during which it prunes them based on the retry
hints database. After that it attempts to deliver the message to
any remaining recipients. It then updates the hints database using
the retry rules.

So if a recipient address works intermittently, it can get repeatedly
deferred at routing time. The retry hints record remains fresh so the
address never reaches the final cutoff time.

This is a fairly common occurrence when a user is bumping up against
their storage quota. Exim had some logic in its local delivery code
to deal with this. However it did not apply to per-recipient defers
in remote deliveries, e.g. over LMTP to a separate IMAP message store.

This commit adds a proper retry rule check during routing so that
the final cutoff time is checked against the message's age. I also
took the opportunity to unify three very similar blocks of code.

I suspect this new check makes the old local delivery cutoff check
redundant, but I have not verified this so I left the code in place.

3 files changed, 42 insertions, 84 deletions

diff --git a/src/src/deliver.c b/src/src/deliver.c
index 34f75fe13..c743fee33 100644
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -2383,48 +2383,12 @@ while (addr_local != NULL)
                retry_record->expired;
 
           /* If we haven't reached the retry time, there is one more check
-          to do, which is for the ultimate address timeout. */
+          to do, which is for the ultimate address timeout. We also do this
+          check during routing so this one might be redundant... */
 
           if (!ok)
-            {
-            retry_config *retry =
-              retry_find_config(retry_key+2, addr2->domain,
-                retry_record->basic_errno,
-                retry_record->more_errno);
-
-            DEBUG(D_deliver|D_retry)
-              {
-              debug_printf("retry time not reached for %s: "
-                "checking ultimate address timeout\n", addr2->address);
-              debug_printf("  now=%d first_failed=%d next_try=%d expired=%d\n",
-                (int)now, (int)retry_record->first_failed,
-                (int)retry_record->next_try, retry_record->expired);
-              }
-
-            if (retry != NULL && retry->rules != NULL)
-              {
-              retry_rule *last_rule;
-              for (last_rule = retry->rules;
-                   last_rule->next != NULL;
-                   last_rule = last_rule->next);
-              DEBUG(D_deliver|D_retry)
-                debug_printf("  received_time=%d diff=%d timeout=%d\n",
-                  received_time, (int)now - received_time, last_rule->timeout);
-              if (now - received_time > last_rule->timeout) ok = TRUE;
-              }
-            else
-              {
-              DEBUG(D_deliver|D_retry)
-                debug_printf("no retry rule found: assume timed out\n");
-              ok = TRUE;    /* No rule => timed out */
-              }
-
-            DEBUG(D_deliver|D_retry)
-              {
-              if (ok) debug_printf("on queue longer than maximum retry for "
-                "address - allowing delivery\n");
-              }
-            }
+            ok = retry_ultimate_address_timeout(retry_key, addr2->domain,
+                retry_record, now);
           }
         }
       else DEBUG(D_retry) debug_printf("no retry record exists\n");
@@ -5656,7 +5620,10 @@ while (addr_new != NULL)           /* Loop until all addresses dealt with */
     will be far too many attempts for an address that gets a 4xx error. In
     fact, after such an error, we should not get here because, the host should
     not be remembered as one this message needs. However, there was a bug that
-    used to cause this to  happen, so it is best to be on the safe side. */
+    used to cause this to  happen, so it is best to be on the safe side.
+
+    Even if we haven't reached the retry time in the hints, there is one
+    more check to do, which is for the ultimate address timeout. */
 
     else if (((queue_running && !deliver_force) || continue_hostname != NULL)
             &&
@@ -5666,6 +5633,9 @@ while (addr_new != NULL)           /* Loop until all addresses dealt with */
             ||
             (address_retry_record != NULL &&
               now < address_retry_record->next_try))
+            &&
+            !retry_ultimate_address_timeout(addr->address_retry_key,
+	      addr->domain, address_retry_record, now)
             )
       {
       addr->message = US"retry time not reached";
diff --git a/src/src/functions.h b/src/src/functions.h
index 174db07eb..604dd4a6a 100644
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -269,6 +269,8 @@ extern void    retry_add_item(address_item *, uschar *, int);
 extern BOOL    retry_check_address(uschar *, host_item *, uschar *, BOOL,
                  uschar **, uschar **);
 extern retry_config *retry_find_config(uschar *, uschar *, int, int);
+extern BOOL    retry_ultimate_address_timeout(uschar *, uschar *,
+                 dbdata_retry *, time_t);
 extern void    retry_update(address_item **, address_item **, address_item **);
 extern uschar *rewrite_address(uschar *, BOOL, BOOL, rewrite_rule *, int);
 extern uschar *rewrite_address_qualify(uschar *, BOOL);
diff --git a/src/src/retry.c b/src/src/retry.c
index e877bf7d0..480933542 100644
--- a/src/src/retry.c
+++ b/src/src/retry.c
@@ -17,26 +17,34 @@
 *************************************************/
 
 /* This function tests whether a message has been on the queue longer than
-the maximum retry time for a particular host.
+the maximum retry time for a particular host or address.
 
 Arguments:
-  host_key      the key to look up a host retry rule
+  retry_key     the key to look up a retry rule
   domain        the domain to look up a domain retry rule
-  basic_errno   a specific error number, or zero if none
-  more_errno    additional data for the error
+  retry_record  contains error information for finding rule
   now           the time
 
 Returns:        TRUE if the ultimate timeout has been reached
 */
 
-static BOOL
-ultimate_address_timeout(uschar *host_key, uschar *domain, int basic_errno,
-  int more_errno, time_t now)
+BOOL
+retry_ultimate_address_timeout(uschar *retry_key, uschar *domain,
+  dbdata_retry *retry_record, time_t now)
 {
-BOOL address_timeout = TRUE;   /* no rule => timed out */
+BOOL address_timeout;
+
+DEBUG(D_retry)
+  {
+  debug_printf("retry time not reached: checking ultimate address timeout\n");
+  debug_printf("  now=%d first_failed=%d next_try=%d expired=%d\n",
+    (int)now, (int)retry_record->first_failed,
+    (int)retry_record->next_try, retry_record->expired);
+  }
 
 retry_config *retry =
-  retry_find_config(host_key+2, domain, basic_errno, more_errno);
+  retry_find_config(retry_key+2, domain,
+    retry_record->basic_errno, retry_record->more_errno);
 
 if (retry != NULL && retry->rules != NULL)
   {
@@ -44,17 +52,23 @@ if (retry != NULL && retry->rules != NULL)
   for (last_rule = retry->rules;
        last_rule->next != NULL;
        last_rule = last_rule->next);
-  DEBUG(D_transport|D_retry)
+  DEBUG(D_retry)
     debug_printf("  received_time=%d diff=%d timeout=%d\n",
       received_time, (int)(now - received_time), last_rule->timeout);
   address_timeout = (now - received_time > last_rule->timeout);
   }
 else
   {
-  DEBUG(D_transport|D_retry)
+  DEBUG(D_retry)
     debug_printf("no retry rule found: assume timed out\n");
+  address_timeout = TRUE;
   }
 
+DEBUG(D_retry)
+  if (address_timeout)
+    debug_printf("on queue longer than maximum retry for address - "
+      "allowing delivery\n");
+
 return address_timeout;
 }
 
@@ -206,25 +220,10 @@ if (host_retry_record != NULL)
 
   if (now < host_retry_record->next_try && !deliver_force)
     {
-    DEBUG(D_transport|D_retry)
-      {
-      debug_printf("host retry time not reached: checking ultimate address "
-        "timeout\n");
-      debug_printf("  now=%d first_failed=%d next_try=%d expired=%d\n",
-        (int)now, (int)host_retry_record->first_failed,
-        (int)host_retry_record->next_try,
-        host_retry_record->expired);
-      }
-
     if (!host_retry_record->expired &&
-        ultimate_address_timeout(host_key, domain,
-          host_retry_record->basic_errno, host_retry_record->more_errno, now))
-      {
-      DEBUG(D_transport|D_retry)
-        debug_printf("on queue longer than maximum retry for "
-          "address - allowing delivery\n");
+        retry_ultimate_address_timeout(host_key, domain,
+          host_retry_record, now))
       return FALSE;
-      }
 
     /* We have not hit the ultimate address timeout; host is unusable. */
 
@@ -249,25 +248,12 @@ if (message_retry_record != NULL)
   *retry_message_key = message_key;
   if (now < message_retry_record->next_try && !deliver_force)
     {
-    DEBUG(D_transport|D_retry)
-      {
-      debug_printf("host+message retry time not reached: checking ultimate "
-        "address timeout\n");
-      debug_printf("  now=%d first_failed=%d next_try=%d expired=%d\n",
-        (int)now, (int)message_retry_record->first_failed,
-        (int)message_retry_record->next_try, message_retry_record->expired);
-      }
-    if (!ultimate_address_timeout(host_key, domain, 0, 0, now))
+    if (!retry_ultimate_address_timeout(host_key, domain,
+        message_retry_record, now))
       {
       host->status = hstatus_unusable;
       host->why = hwhy_retry;
       }
-    else
-      {
-      DEBUG(D_transport|D_retry)
-        debug_printf("on queue longer than maximum retry for "
-          "address - allowing delivery\n");
-      }
     return FALSE;
     }
   }