From 66f3ccba5522170c625eb384b3c72e613cb1328d Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Wed, 8 Jul 2026 08:24:15 +0200 Subject: [PATCH 1/2] fix(core): stop ActiveSync log spam and folder loss on IMAP outage When the mail server is unreachable during ActiveSync polling, a single IMAP connection failure was caught, logged at ERR/WARN, and re-thrown at every layer (IMAP factory, _getMailFolders, getFolders), producing several log lines per poll per device. getFolders() also returned an empty array, which made FOLDERSYNC diff against an empty hierarchy and could make clients drop all their mail folders. Classify transient connection failures (DISCONNECT / SERVER_CONNECT / LOGIN_UNAVAILABLE, detected via the preserved exception chain) in the IMAP factory: log once at NOTICE and throw Horde_ActiveSync_Exception_TemporaryFailure. Drop the redundant re-logging in the driver, and have getFolderList() return false on a temporary failure so the state engine skips folder-change detection this round and the device retries later instead of losing folders. Genuine (non-connection) errors are unchanged: still logged at ERR. --- lib/Horde/Core/ActiveSync/Driver.php | 37 ++++++++++++++--- lib/Horde/Core/ActiveSync/Imap/Factory.php | 46 ++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/lib/Horde/Core/ActiveSync/Driver.php b/lib/Horde/Core/ActiveSync/Driver.php index 9654b694..2695a5a8 100644 --- a/lib/Horde/Core/ActiveSync/Driver.php +++ b/lib/Horde/Core/ActiveSync/Driver.php @@ -585,13 +585,25 @@ public function getWasteBasket($class) /** * Return an array of stats for the server's folder list. * - * @return array An array of folder stats @see self::statFolder() + * @return array|boolean An array of folder stats + * (@see self::statFolder()), or false if the mail + * server is temporarily unavailable and folder + * change detection should be skipped this round. * @todo Horde 6 move to base class */ public function getFolderList() { $this->_logger->meta('Horde_Core_ActiveSync_Driver::getFolderList()'); - $folderlist = $this->getFolders(); + try { + $folderlist = $this->getFolders(); + } catch (Horde_ActiveSync_Exception_TemporaryFailure $e) { + // Mail server temporarily unavailable. Returning false tells the + // state engine to skip folder-change detection this round instead + // of computing a diff against an empty hierarchy (which would + // delete the client's mail folders). The device retries on the + // next sync. Already logged once by the IMAP factory. + return false; + } $folders = []; foreach ($folderlist as $f) { $folders[] = $this->statFolder( @@ -610,6 +622,8 @@ public function getFolderList() * Return an array of the server's folder objects. * * @return array An array of Horde_ActiveSync_Message_Folder objects. + * @throws Horde_ActiveSync_Exception_TemporaryFailure If the mail server + * is temporarily unreachable (transient connection failure). */ public function getFolders() { @@ -776,8 +790,16 @@ public function getFolders() // at least an INBOX, even with email sync turned off. try { $folders = array_merge($folders, $this->_getMailFolders()); + } catch (Horde_ActiveSync_Exception_TemporaryFailure $e) { + // Mail server temporarily unreachable. Do not fall through to + // returning a partial/empty hierarchy (that can make clients + // drop their mail folders); propagate so the caller can defer + // the sync and the device retries later. Already logged once + // by the IMAP factory. + $this->_endBuffer(); + throw $e; } catch (Horde_ActiveSync_Exception $e) { - $this->_logger->err($e->getMessage()); + $this->_logger->meta($e->getMessage()); $this->_endBuffer(); return []; } @@ -4122,9 +4144,14 @@ protected function _getMailFolders() try { $imap_folders = $this->_imap->getMailboxes(true); } catch (Horde_ActiveSync_Exception $e) { - $this->_logger->err( + // The IMAP factory already logged the root cause once; do + // not re-log the same failure at every layer. Keep a debug + // trace and re-throw the original (which preserves the + // Horde_ActiveSync_Exception_TemporaryFailure type on a + // transient connection failure). + $this->_logger->meta( sprintf( - 'Problem loading mail folders from IMAP server: %s', + 'Mail folder list unavailable: %s', $e->getMessage() ) ); diff --git a/lib/Horde/Core/ActiveSync/Imap/Factory.php b/lib/Horde/Core/ActiveSync/Imap/Factory.php index bc42b82c..34356428 100644 --- a/lib/Horde/Core/ActiveSync/Imap/Factory.php +++ b/lib/Horde/Core/ActiveSync/Imap/Factory.php @@ -86,6 +86,19 @@ public function getMailboxes($force = false) } } } catch (Horde_Exception $e) { + if ($this->_isTransientImapError($e)) { + // The mail server is temporarily unreachable. This is an + // expected, recoverable condition during ActiveSync + // polling, so log it once here at a low severity and let + // the caller defer the sync instead of treating it as a + // hard error (which spams the log at every layer and can + // make clients drop their mail folders). + Horde::log(sprintf( + 'Mail server temporarily unavailable while retrieving mailbox list: %s', + $e->getMessage() + ), Horde_Log::NOTICE); + throw new Horde_ActiveSync_Exception_TemporaryFailure($e); + } Horde::log(sprintf( 'Error retrieving mailbox list: %s', $e->getMessage() @@ -174,4 +187,37 @@ public function getMsgFlags() return $msgFlags; } + /** + * Determine whether an exception represents a transient IMAP failure + * (mail server unreachable or connection dropped) as opposed to a hard + * error such as an authentication or configuration problem. + * + * The relevant Horde_Imap_Client_Exception is preserved in the previous + * exception chain even when wrapped by outer exceptions (e.g. the + * injector's "Cannot create IMP_Ftree"), so walk the chain to classify. + * + * @author Torben Dannhauer + * + * @param Throwable $e The caught exception. + * + * @return boolean True if the failure looks transient/connection related. + */ + protected function _isTransientImapError($e) + { + $transient = [ + Horde_Imap_Client_Exception::DISCONNECT, + Horde_Imap_Client_Exception::SERVER_CONNECT, + Horde_Imap_Client_Exception::LOGIN_UNAVAILABLE, + ]; + + for ($current = $e; $current !== null; $current = $current->getPrevious()) { + if ($current instanceof Horde_Imap_Client_Exception + && in_array($current->getCode(), $transient, true)) { + return true; + } + } + + return false; + } + } From 1dcaf9d6c4eb63e136ec5c1cab75b018bf90ba7d Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Wed, 8 Jul 2026 20:40:53 +0200 Subject: [PATCH 2/2] fix(core): address review - inject logger and stop folder loss on hard IMAP errors Incorporate review feedback on the ActiveSync IMAP outage handling: - Imap_Factory: inject a Horde_Log_Logger via the constructor instead of the static Horde::log() facade (wired from the injector in the ActiveSyncBackend factory); a null logger is used when none is given. - Driver::getFolders(): propagate hard IMAP errors instead of returning an empty/partial hierarchy. Returning [] let FOLDERSYNC diff against a reduced list and could make clients delete the user's mail folders - the same risk previously only guarded for transient failures. - Driver::getFolderList(): catch the base Horde_ActiveSync_Exception so any folder-enumeration failure (transient or hard) skips folder-change detection this round; the device retries later. Log severity is still decided once in the factory (NOTICE transient, ERR hard). --- lib/Horde/Core/ActiveSync/Driver.php | 40 ++++++++++---------- lib/Horde/Core/ActiveSync/Imap/Factory.php | 32 +++++++++++++--- lib/Horde/Core/Factory/ActiveSyncBackend.php | 4 +- 3 files changed, 51 insertions(+), 25 deletions(-) diff --git a/lib/Horde/Core/ActiveSync/Driver.php b/lib/Horde/Core/ActiveSync/Driver.php index 2695a5a8..7459663d 100644 --- a/lib/Horde/Core/ActiveSync/Driver.php +++ b/lib/Horde/Core/ActiveSync/Driver.php @@ -586,9 +586,9 @@ public function getWasteBasket($class) * Return an array of stats for the server's folder list. * * @return array|boolean An array of folder stats - * (@see self::statFolder()), or false if the mail - * server is temporarily unavailable and folder - * change detection should be skipped this round. + * (@see self::statFolder()), or false if the folder + * hierarchy could not be built and folder change + * detection should be skipped this round. * @todo Horde 6 move to base class */ public function getFolderList() @@ -596,12 +596,14 @@ public function getFolderList() $this->_logger->meta('Horde_Core_ActiveSync_Driver::getFolderList()'); try { $folderlist = $this->getFolders(); - } catch (Horde_ActiveSync_Exception_TemporaryFailure $e) { - // Mail server temporarily unavailable. Returning false tells the - // state engine to skip folder-change detection this round instead - // of computing a diff against an empty hierarchy (which would - // delete the client's mail folders). The device retries on the - // next sync. Already logged once by the IMAP factory. + } catch (Horde_ActiveSync_Exception $e) { + // The folder hierarchy could not be built - the mail server is + // temporarily unavailable or returned a hard error. Returning + // false tells the state engine to skip folder-change detection + // this round instead of computing a diff against an incomplete + // hierarchy (which would delete the client's mail folders). The + // device retries on the next sync. The root cause was already + // logged once at its appropriate severity by the IMAP factory. return false; } $folders = []; @@ -790,18 +792,18 @@ public function getFolders() // at least an INBOX, even with email sync turned off. try { $folders = array_merge($folders, $this->_getMailFolders()); - } catch (Horde_ActiveSync_Exception_TemporaryFailure $e) { - // Mail server temporarily unreachable. Do not fall through to - // returning a partial/empty hierarchy (that can make clients - // drop their mail folders); propagate so the caller can defer - // the sync and the device retries later. Already logged once - // by the IMAP factory. - $this->_endBuffer(); - throw $e; } catch (Horde_ActiveSync_Exception $e) { - $this->_logger->meta($e->getMessage()); + // The mail folders could not be retrieved - either a transient + // outage (Horde_ActiveSync_Exception_TemporaryFailure) or a + // hard IMAP error. In neither case may we fall through to + // returning a partial hierarchy that omits the mail folders: + // FOLDERSYNC would diff against the reduced list and the client + // could delete the user's mail folders. Propagate so the caller + // (getFolderList()) can skip folder-change detection this round; + // the device retries later. The root cause was already logged + // once by the IMAP factory (NOTICE for transient, ERR for hard). $this->_endBuffer(); - return []; + throw $e; } $this->_endBuffer(); diff --git a/lib/Horde/Core/ActiveSync/Imap/Factory.php b/lib/Horde/Core/ActiveSync/Imap/Factory.php index 34356428..01d33f9f 100644 --- a/lib/Horde/Core/ActiveSync/Imap/Factory.php +++ b/lib/Horde/Core/ActiveSync/Imap/Factory.php @@ -7,8 +7,6 @@ * @package Core */ -use Horde\Core\Horde; - /** * Horde_Core_ActiveSync_Imap_Factory implements a factory/builder for * providing a Horde_ActiveSync_Imap_Adapter object as well as building a tree @@ -25,6 +23,30 @@ class Horde_Core_ActiveSync_Imap_Factory implements Horde_ActiveSync_Interface_I protected $_mailboxlist; protected $_specialMailboxlist; + /** + * Logger. + * + * @var Horde_Log_Logger + */ + protected $_logger; + + /** + * Constructor. + * + * @param array $params Parameters: + * - logger: (Horde_Log_Logger) The logger to use. Optional; a null + * logger is used when none is supplied. + * + * @author Torben Dannhauer + */ + public function __construct(array $params = []) + { + $this->_logger = (isset($params['logger']) + && $params['logger'] instanceof Horde_Log_Logger) + ? $params['logger'] + : new Horde_Log_Logger(new Horde_Log_Handler_Null()); + } + /** * Return a Horde_Imap_Client * @@ -93,13 +115,13 @@ public function getMailboxes($force = false) // the caller defer the sync instead of treating it as a // hard error (which spams the log at every layer and can // make clients drop their mail folders). - Horde::log(sprintf( + $this->_logger->log(sprintf( 'Mail server temporarily unavailable while retrieving mailbox list: %s', $e->getMessage() ), Horde_Log::NOTICE); throw new Horde_ActiveSync_Exception_TemporaryFailure($e); } - Horde::log(sprintf( + $this->_logger->log(sprintf( 'Error retrieving mailbox list: %s', $e->getMessage() ), Horde_Log::ERR); @@ -132,7 +154,7 @@ public function getSpecialMailboxes() try { $this->_specialMailboxlist = $registry->mail->getSpecialMailboxes(); } catch (Horde_Exception $e) { - Horde::log(sprintf( + $this->_logger->log(sprintf( 'Error retrieving specialmailbox list: %s', $e->getMessage() ), Horde_Log::ERR); diff --git a/lib/Horde/Core/Factory/ActiveSyncBackend.php b/lib/Horde/Core/Factory/ActiveSyncBackend.php index 4c030326..37ac9540 100644 --- a/lib/Horde/Core/Factory/ActiveSyncBackend.php +++ b/lib/Horde/Core/Factory/ActiveSyncBackend.php @@ -31,7 +31,9 @@ public function create(Horde_Injector|Injector $injector) // Backend driver and dependencies $params = ['registry' => $registry]; - $adapter_params = ['factory' => new Horde_Core_ActiveSync_Imap_Factory()]; + $adapter_params = ['factory' => new Horde_Core_ActiveSync_Imap_Factory([ + 'logger' => $injector->getInstance('Horde_Log_Logger'), + ])]; // Determine emailsync setting - force to off if we don't have a mail API. // Use local variable instead of mutating global $conf.