From: Tony Finch Date: Thu, 29 Nov 2012 18:39:52 +0000 (+0000) Subject: Fix ultimate retry timeouts for intermittently deliverable recipients. X-Git-Tag: exim-4_81_RC1~3^2~31 X-Git-Url: https://git.exim.org/exim.git/commitdiff_plain/ba9af0af397dd7d395378b883f8d9beb3bdd5ffd 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. --- 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; } }