Stop one unreadable message from holding its whole folder

A multipart message whose header reads `boundary = "…"`, with spaces
around the sign, is legal under RFC 2045 and was unreadable: the
library's pattern wants the sign hard against the word, found no
boundary, and threw "no content found". That is thrown while a page of
mail is being fetched, so it took the page and every page after it,
the mark never moved, and the folder failed at the same message on
every poll until somebody deleted it on the server (#45).

Two changes, because the header is one way to be unreadable and there
will be others.

  Boundary  ImapConnectionFactory configures a pattern that allows
            whitespace on both sides of the sign, takes a quoted value
            whole and ends an unquoted one at a semicolon or a space.
            The library's pattern with \s* added would not do: it
            keeps the space before a following semicolon, and the
            boundary then matches no line.
  Fetch     ImapPagedFetch replaces the library's chunked(). Soft-fail
            is on, and the pages are counted from the search rather
            than from what was handed over — chunked() counts the
            latter, so with one message dropped it asks for empty
            pages without end. What each page dropped is handed to
            processBatch() beside it and held like any message that
            would not store: five syncs, then skipped and logged.

The page is walked in UID order with the unreadable messages standing
where they would have stood. holdForRetry() counts the lowest failure
of a run by the order it is told of them, and in any other order two
such messages would take the counter from each other for good.

ImapConnectionFactory::config() is split out of connect() so a test can
parse a message the way a sync does. One test pins that the library's
own pattern still throws on the report's fixture, so the day it is
fixed upstream is the day that test says so.

Fixes #45

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -8,6 +8,17 @@ The published image tags: `latest` follows the most recent release below,
 
 ## Unreleased
 
+**One message could stop an IMAP folder from syncing, for good.** A multipart message whose
+header puts spaces around the equals sign — `boundary = "…"`, which the standard allows and some
+mailers write — could not be read, and because that failed while a page of mail was being fetched,
+the whole folder's sync failed with `no content found` on every poll until the message was deleted
+on the server. Nothing newer in that folder arrived, and nothing in plMail said so. Such a message
+is read now. And a message that cannot be read for any other reason no longer takes its folder with
+it: the rest of the page is stored, the message is asked for again on the next five syncs, and then
+it is skipped with an error in the log naming the folder and the UID. A folder that was stuck this
+way catches up by itself on the next poll after upgrading. No migration.
+[#45](https://github.com/karatektus/pl_mail/issues/45).
+
 **A reading pane beside the message list.** Opening a message used to replace the list; Settings →
 Reading pane now offers **Right**, which puts the message beside the list on a screen
 that has room for both. The open row is marked, the back arrow closes the message and leaves the

src/Domain/Helper/ImapConnectionFactory.php

@@ -29,6 +29,32 @@ class ImapConnectionFactory
      */
     public const string UNPARSEABLE_DATE = '1970-01-01 00:00:00 UTC';
 
+    /**
+     * How the boundary of a multipart message is read out of its Content-Type.
+     *
+     * The library's own pattern is `/boundary=(.*?(?=;)|(.*))/i`, which wants
+     * the equals sign hard against the word. RFC 2045 does not: a parameter is
+     * tokens under RFC 822's lexical rules, and whitespace may stand between
+     * them, so
+     *
+     *     Content-Type: multipart/mixed; boundary = "--=_Part_1"
+     *
+     * is a legal header that some mailers write. The library found no boundary
+     * in it, declared the message to have "no content", and — because that is
+     * thrown while a page of mail is being fetched — took the whole folder's
+     * sync down with it, on every poll, until the message was deleted (#45).
+     *
+     * Whitespace is allowed on both sides of the sign. A quoted value is taken
+     * whole, spaces and all, since quoting is how a boundary is allowed to
+     * contain them; an unquoted one ends at the first semicolon or whitespace.
+     * The branch reset `(?|…)` puts either form in group 1, which is the one
+     * group Header::find() returns. Nothing trails the value: the obvious
+     * smaller change — the library's pattern with `\s*` added — captures the
+     * space before a following semicolon, and the library's clean-up does not
+     * strip it, so the boundary would be found and then match no line.
+     */
+    public const string BOUNDARY_PATTERN = '/boundary\s*=\s*(?|"([^"]+)"|([^;\s]+))/i';
+
     public function __construct(
         private readonly OAuthTokenManager $tokenManager,
     ) {
@@ -79,10 +105,31 @@ class ImapConnectionFactory
             $accountConfig['timeout'] = $timeout;
         }
 
-        $client = new Client(Config::make([
+        $client = new Client(self::config($accountConfig));
+
+        $client->connect();
+
+        return $client;
+    }
+
+    /**
+     * The library's configuration as every connection of this application
+     * uses it: how mail is decoded and parsed, plus the one account.
+     *
+     * Public and static, and separate from connect(), so the parsing half can
+     * be had without a server — a test that parses a message has to parse it
+     * the way a sync would, and one that builds its own config is testing
+     * the library's defaults instead.
+     *
+     * @param array<string, mixed> $account the account's connection settings;
+     *                                      empty where nothing is being opened
+     */
+    public static function config(array $account = []): Config
+    {
+        return Config::make([
             'default'  => 'default',
             'accounts' => [
-                'default' => $accountConfig,
+                'default' => $account,
             ],
             // The library converts every body part from whatever charset it
             // declares, which is wrong for the senders that declare
@@ -96,11 +143,8 @@ class ImapConnectionFactory
             ],
             'options' => [
                 'fallback_date' => self::UNPARSEABLE_DATE,
+                'boundary'      => self::BOUNDARY_PATTERN,
             ],
-        ]));
-
-        $client->connect();
-
-        return $client;
+        ]);
     }
 }

src/Service/Imap/ImapPagedFetch.php

@@ -0,0 +1,68 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Service\Imap;
+
+use Webklex\PHPIMAP\Query\Query;
+use Webklex\PHPIMAP\Support\MessageCollection;
+
+/**
+ * A search fetched a page at a time, in which a message the library cannot
+ * read costs that message and not the folder.
+ *
+ * The library has a loop for this, Query::chunked(), and the syncer used it.
+ * It has two properties that between them let one message stop a folder for
+ * good (#45):
+ *
+ *   - **It gives up the whole fetch for one message.** A message whose MIME
+ *     structure the parser will not take throws while the page is being built,
+ *     and with the library's default the exception leaves the loop whole. The
+ *     page is lost, the pages after it are never asked for, and the syncer's
+ *     own retry-then-skip (MessageSyncer::holdForRetry()) never hears of the
+ *     message, because it is only ever handed messages that were built. The
+ *     mark does not move, so the next sync asks for the same range and dies at
+ *     the same place, every poll, for as long as the message exists.
+ *
+ *   - **Its soft-fail setting cannot be used to fix that.** With soft-fail on,
+ *     the unreadable message is dropped and the page comes back — but the loop
+ *     runs until the messages it has HANDED OVER add up to the messages the
+ *     search found, and one dropped message means they never do. It then asks
+ *     for page after empty page without end.
+ *
+ * So the pages are counted here, from the search, which is a number that does
+ * not depend on how many messages survived. Soft-fail is on, and what it
+ * dropped on each page is handed over beside the page, keyed by UID, so the
+ * caller can hold each for a retry like any other message it could not store.
+ *
+ * What still ends the fetch is what should: the server going away. That is not
+ * a property of one message, and trying the next page would only fail again.
+ */
+final readonly class ImapPagedFetch
+{
+    /**
+     * @return iterable<array{MessageCollection, array<int, \Throwable>}> each
+     *         page as the library built it, and the messages of that page it
+     *         could not build: UID => what was thrown
+     */
+    public function pages(Query $query, int $pageSize): iterable
+    {
+        $query->setSoftFail(true);
+
+        $found = $query->search();
+        $pages = (int) ceil($found->count() / $pageSize);
+
+        // The library keeps every failure of a query in one list, so each page
+        // is handed only what was added to it since the page before.
+        $reported = [];
+
+        for ($page = 1; $page <= $pages; ++$page) {
+            $batch = $query->limit($pageSize, $page)->curate_messages($found);
+
+            $unreadable = array_diff_key($query->errors(), $reported);
+            $reported  += $unreadable;
+
+            yield [$batch, $unreadable];
+        }
+    }
+}

src/Service/Imap/MessageSyncer.php

@@ -55,6 +55,7 @@ class MessageSyncer
         private readonly VanishedMessageReconciler $vanished,
         private readonly GhostMessageReaper $ghosts,
         private readonly ManagerRegistry $registry,
+        private readonly ImapPagedFetch $pagedFetch,
     ) {}
 
     /**
@@ -148,14 +149,16 @@ class MessageSyncer
         $lowestSkippedUid = null;
 
         try {
-            $folder->messages()
-                ->where(self::uidRangeCriteria($uidRange))
-                ->chunked(function ($batch) use ($mailboxId, $accountId, $lastSeenUid, &$synced, &$syncedUids, &$lowestSkippedUid, $presence) {
-                    $this->processBatch($batch, $mailboxId, $accountId, $lastSeenUid, $syncedUids, $lowestSkippedUid, $presence);
-                    $synced += count($batch);
-                    $this->startNextBatch();
-                    $this->logger->info(sprintf('Synced %d messages so far', $synced));
-                }, self::BATCH_SIZE);
+            // Not the library's chunked(): one message it cannot parse ends
+            // that loop for the whole folder, on every sync. See ImapPagedFetch.
+            $query = $folder->messages()->where(self::uidRangeCriteria($uidRange));
+
+            foreach ($this->pagedFetch->pages($query, self::BATCH_SIZE) as [$batch, $unreadable]) {
+                $this->processBatch($batch, $mailboxId, $accountId, $lastSeenUid, $syncedUids, $lowestSkippedUid, $presence, $unreadable);
+                $synced += count($batch);
+                $this->startNextBatch();
+                $this->logger->info(sprintf('Synced %d messages so far', $synced));
+            }
 
             $mailbox = $this->mailboxRepository->find($mailboxId);
 
@@ -197,6 +200,10 @@ class MessageSyncer
      *                                      registered within the same sync run
      *                                      (guards against duplicates inside a
      *                                      single chunked call)
+     * @param array<int, \Throwable> $unreadable  the UIDs of this page the
+     *                                      library could not build a message
+     *                                      from, and what it threw — see
+     *                                      ImapPagedFetch
      * @param int|null        $lowestSkippedUid  the run-wide lowest UID held
      *                                      back for a retry; the high-water
      *                                      mark is clamped below it
@@ -209,6 +216,7 @@ class MessageSyncer
         array            &$syncedUids,
         ?int             &$lowestSkippedUid,
         ImapUidPresence  $presence,
+        array            $unreadable = [],
     ): void {
         // The run as it stood before this batch. A batch the database refuses
         // is decided again from here, one message at a time.
@@ -217,7 +225,7 @@ class MessageSyncer
 
         try {
             [$mailbox, $messages, $rawBodies, $maxUid] = $this->storeMessages(
-                $batch, $mailboxId, $accountId, $lastSeenUid, $syncedUids, $lowestSkippedUid, $presence, false,
+                $batch, $mailboxId, $accountId, $lastSeenUid, $syncedUids, $lowestSkippedUid, $presence, false, $unreadable,
             );
         } catch (\Throwable $refused) {
             // Only a refused write closes the manager. Anything else, the
@@ -260,7 +268,7 @@ class MessageSyncer
             }
 
             [$mailbox, $messages, $rawBodies, $maxUid] = $this->storeMessages(
-                $batch, $mailboxId, $accountId, $lastSeenUid, $syncedUids, $lowestSkippedUid, $presence, true,
+                $batch, $mailboxId, $accountId, $lastSeenUid, $syncedUids, $lowestSkippedUid, $presence, true, $unreadable,
             );
         }
 
@@ -334,6 +342,7 @@ class MessageSyncer
         ?int             &$lowestSkippedUid,
         ImapUidPresence  $presence,
         bool             $oneAtATime,
+        array            $unreadable = [],
     ): array {
         $mailbox  = $this->mailboxRepository->find($mailboxId);
         $messages = [];
@@ -342,13 +351,39 @@ class MessageSyncer
         $rawBodies = [];
         $maxUid   = 0;
 
-        // Pass 1 — build + persist Message rows (no threading yet)
-        foreach ($batch as $imapMessage) {
+        // The page in UID order, with the messages the library could not
+        // build standing where they would have stood. The order is not
+        // tidiness: holdForRetry() counts the LOWEST failure of a run, and
+        // decides which that is by the order it is told of them. Handed the
+        // unreadable ones before or after the rest, two poison messages would
+        // take the counter from each other on every sync and neither would
+        // ever reach the attempt that lets it go.
+        /** @var array<int, ImapMessage|\Throwable> $page */
+        $page = $unreadable;
+
+        foreach ($batch as $fetched) {
             // Cast because webklex only declares it in a docblock
             // (`@method integer getUid()`), which static analysis cannot read
             // as an int. The value always is one: holdForRetry() takes it as
             // one and would have thrown.
-            $uid = (int) $imapMessage->getUid();
+            $page[(int) $fetched->getUid()] = $fetched;
+        }
+
+        ksort($page);
+
+        // Pass 1 — build + persist Message rows (no threading yet)
+        foreach ($page as $uid => $imapMessage) {
+            if ($imapMessage instanceof \Throwable) {
+                if (true === $this->holdUnreadable($mailbox, $uid, $imapMessage, $lastSeenUid, $syncedUids, $lowestSkippedUid)) {
+                    $maxUid = max($maxUid, $uid);
+                }
+
+                if (true === $oneAtATime) {
+                    $this->em->flush();
+                }
+
+                continue;
+            }
 
             // A `lastSeenUid+1:*` range still returns the highest-UID message when
             // nothing newer exists (`*` clamps to it), so every run re-delivers the
@@ -517,6 +552,55 @@ class MessageSyncer
         $this->registry->resetManager();
     }
 
+    /**
+     * A message the library could not build from what the server sent: held
+     * for a retry like one that would not store, and let go on the same count.
+     *
+     * These never used to get here. The library threw while it was fetching
+     * the page, the page never arrived, and the retry-then-skip below was
+     * never told there was anything to count — which is how one message with
+     * a header the parser disliked stopped a folder for good (#45).
+     *
+     * A UID this folder already holds is not a failure, whatever the library
+     * made of it this time: `lastSeenUid+1:*` re-delivers the newest message
+     * on every run, and a message that is stored was once read.
+     *
+     * @param array<int,bool> $syncedUids
+     *
+     * @return bool whether the mark may pass this UID — it is already held, or
+     *              it has failed often enough to be given up
+     */
+    private function holdUnreadable(
+        Mailbox    $mailbox,
+        int        $uid,
+        \Throwable $reason,
+        int        $lastSeenUid,
+        array      $syncedUids,
+        ?int       &$lowestSkippedUid,
+    ): bool {
+        if ($uid <= $lastSeenUid || true === isset($syncedUids[$uid])) {
+            return true;
+        }
+
+        $retrying = $this->holdForRetry($mailbox, $uid, $lowestSkippedUid);
+
+        $this->logger->error(
+            true === $retrying
+                ? 'Could not read a message the server sent; it is asked for again next sync'
+                : 'Could not read a message the server sent, too many times; skipping it for good',
+            [
+                'uid'       => $uid,
+                'mailboxId' => $mailbox->id,
+                'mailbox'   => $mailbox->fullPath,
+                'attempts'  => $mailbox->failedUidAttempts,
+                'error'     => $reason->getMessage(),
+                'exception' => $reason,
+            ],
+        );
+
+        return false === $retrying;
+    }
+
     /**
      * Keep a UID that could not be stored inside the next sync's range — up to
      * a point.

tests/Domain/Helper/ImapBoundaryParsingTest.php

@@ -0,0 +1,129 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Domain\Helper;
+
+use App\Domain\Helper\ImapConnectionFactory;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Webklex\PHPIMAP\Config;
+use Webklex\PHPIMAP\Exceptions\MessageContentFetchingException;
+use Webklex\PHPIMAP\Message;
+
+/**
+ * A multipart message is read whatever way its boundary is legally written.
+ *
+ * RFC 2045 lets whitespace stand around the equals sign of a parameter, and
+ * some mailers put it there: `boundary = "XYZ"`. The library's own pattern
+ * wants the sign hard against the word, found no boundary, and called the
+ * message one with "no content" — an exception thrown while a page of mail is
+ * fetched, which stopped the whole folder's sync on every poll until somebody
+ * deleted the message on the server (#45).
+ *
+ * Parsed through ImapConnectionFactory::config(), which is the configuration a
+ * sync parses with. That is half the point: the pattern is a config value, and
+ * a test that handed the library its own config would pass without the value
+ * ever reaching the header that reads it.
+ */
+final class ImapBoundaryParsingTest extends TestCase
+{
+    /**
+     * @return iterable<string, array{string}>
+     */
+    public static function waysToWriteABoundary(): iterable
+    {
+        yield 'hard against the sign, as most mailers write it' => ['boundary="XYZ"'];
+        yield 'spaced on both sides — the report'               => ['boundary = "XYZ"'];
+        yield 'spaced before only'                              => ['boundary ="XYZ"'];
+        yield 'spaced after only'                               => ['boundary= "XYZ"'];
+        yield 'unquoted'                                        => ['boundary=XYZ'];
+        yield 'unquoted and spaced'                             => ['boundary = XYZ'];
+        yield 'with a parameter after it'                       => ['boundary = "XYZ"; charset=utf-8'];
+        // The trap in the obvious smaller fix: a pattern that runs to the
+        // semicolon keeps the space before it, and "XYZ " matches no line.
+        yield 'with a space before the next parameter'          => ['boundary = "XYZ" ; charset=utf-8'];
+        yield 'unquoted, with a space before the next one'      => ['boundary = XYZ ; charset=utf-8'];
+        yield 'folded onto the next line'                       => ["\r\n boundary = \"XYZ\""];
+        yield 'in capitals'                                     => ['BOUNDARY = "XYZ"'];
+    }
+
+    #[DataProvider('waysToWriteABoundary')]
+    public function testTheBodyIsReadHoweverTheBoundaryIsWritten(string $parameter): void
+    {
+        $message = Message::fromString($this->multipart($parameter, 'XYZ'), ImapConnectionFactory::config());
+
+        self::assertSame('hello', trim((string) $message->getTextBody()));
+    }
+
+    /** Quoting is how a boundary is allowed to contain a space, so the space is part of it. */
+    public function testAQuotedBoundaryKeepsTheSpacesInsideIt(): void
+    {
+        $message = Message::fromString(
+            $this->multipart('boundary = "part one"', 'part one'),
+            ImapConnectionFactory::config(),
+        );
+
+        self::assertSame('hello', trim((string) $message->getTextBody()));
+    }
+
+    /** A part that is itself multipart reads its own boundary through the same pattern. */
+    public function testANestedMultipartWithASpacedBoundaryIsReadToo(): void
+    {
+        $raw = "From: sender@example.com\r\n"
+            . "To: rcpt@example.com\r\n"
+            . "Subject: nested\r\n"
+            . "MIME-Version: 1.0\r\n"
+            . "Content-Type: multipart/mixed; boundary = \"OUTER\"\r\n"
+            . "\r\n"
+            . "--OUTER\r\n"
+            . "Content-Type: multipart/alternative; boundary = \"INNER\"\r\n"
+            . "\r\n"
+            . "--INNER\r\n"
+            . "Content-Type: text/plain; charset=utf-8\r\n"
+            . "\r\n"
+            . "hello\r\n"
+            . "--INNER\r\n"
+            . "Content-Type: text/html; charset=utf-8\r\n"
+            . "\r\n"
+            . "<p>hello</p>\r\n"
+            . "--INNER--\r\n"
+            . "--OUTER--\r\n";
+
+        $message = Message::fromString($raw, ImapConnectionFactory::config());
+
+        self::assertSame('hello', trim((string) $message->getTextBody()));
+        self::assertSame('<p>hello</p>', trim((string) $message->getHTMLBody()));
+    }
+
+    /**
+     * The contrast, and the reason the pattern is configured at all: the same
+     * bytes through the library's defaults. If this ever stops throwing, the
+     * library has fixed its pattern and BOUNDARY_PATTERN can be looked at
+     * again — until then it is what stands between this header and a folder
+     * that never syncs.
+     */
+    public function testTheLibrarysOwnPatternDoesNotReadTheSpacedForm(): void
+    {
+        $this->expectException(MessageContentFetchingException::class);
+        $this->expectExceptionMessage('no content found');
+
+        Message::fromString($this->multipart('boundary = "XYZ"', 'XYZ'), Config::make([]));
+    }
+
+    /** The message of the report: synthetic, and CRLF as on the wire. */
+    private function multipart(string $boundaryParameter, string $boundary): string
+    {
+        return "From: sender@example.com\r\n"
+            . "To: rcpt@example.com\r\n"
+            . "Subject: boundary whitespace repro\r\n"
+            . "MIME-Version: 1.0\r\n"
+            . sprintf("Content-Type: multipart/mixed; %s\r\n", $boundaryParameter)
+            . "\r\n"
+            . sprintf("--%s\r\n", $boundary)
+            . "Content-Type: text/plain; charset=utf-8\r\n"
+            . "\r\n"
+            . "hello\r\n"
+            . sprintf("--%s--\r\n", $boundary);
+    }
+}

tests/Service/Imap/BatchStorageTest.php

@@ -180,9 +180,159 @@ final class BatchStorageTest extends KernelTestCase
         self::assertSame(103, (int) $mailbox['last_seen_uid'], 'the mark must pass a UID that is already stored');
     }
 
+    /**
+     * A message the library cannot build never reached this code: the fetch
+     * threw, the page never arrived, and nothing counted the failure — so the
+     * folder stopped at it on every sync for as long as the message existed
+     * (#45). It arrives beside its page now, and is held like any other
+     * message that would not store: the rest of the page is written, the mark
+     * waits below it, and the mailbox records which UID it is waiting for.
+     */
+    public function testAMessageTheLibraryCouldNotReadIsHeldForARetryAndCostsNothingElse(): void
+    {
+        $this->storeBatchWith([102 => new \RuntimeException('no content found')], $this->mail(101), $this->mail(103));
+
+        self::assertSame([101, 103], $this->storedUids(), 'its neighbours are stored');
+
+        $mailbox = $this->mailboxRow();
+
+        self::assertSame(102, (int) $mailbox['failed_uid']);
+        self::assertSame(1, (int) $mailbox['failed_uid_attempts']);
+        self::assertSame(101, (int) $mailbox['last_seen_uid'], 'the mark waits below it, so the next sync asks again');
+    }
+
+    /**
+     * Held, not held for ever. A message the parser will never take is let go
+     * after MessageSyncer::MAX_UID_ATTEMPTS syncs, and the mark moves past it
+     * — which is the whole difference between losing one message and losing a
+     * folder.
+     */
+    public function testAfterEnoughSyncsItIsLetGoAndTheMarkMovesPastIt(): void
+    {
+        $unreadable = [102 => new \RuntimeException('no content found')];
+
+        for ($sync = 1; $sync <= 4; ++$sync) {
+            $this->storeBatchWith($unreadable, $this->mail(101), $this->mail(103));
+
+            self::assertSame(101, (int) $this->mailboxRow()['last_seen_uid'], sprintf('still held after sync %d', $sync));
+        }
+
+        $this->storeBatchWith($unreadable, $this->mail(101), $this->mail(103));
+
+        $mailbox = $this->mailboxRow();
+
+        self::assertSame(5, (int) $mailbox['failed_uid_attempts']);
+        self::assertSame(103, (int) $mailbox['last_seen_uid'], 'the fifth failure lets it go');
+        self::assertSame([101, 103], $this->storedUids(), 'and nothing was stored twice on the way');
+    }
+
+    /**
+     * It is given up even when it is the newest message in the folder, with
+     * nothing above it to carry the mark past. Otherwise it would be fetched
+     * and fail on every poll until other mail arrived.
+     */
+    public function testTheNewestMessageInTheFolderIsLetGoToo(): void
+    {
+        for ($sync = 1; $sync <= 5; ++$sync) {
+            $this->storeBatchWith([102 => new \RuntimeException('no content found')], $this->mail(101));
+        }
+
+        self::assertSame(102, (int) $this->mailboxRow()['last_seen_uid']);
+    }
+
+    /**
+     * The retry counter belongs to the LOWEST failure of a run. Told of the
+     * unreadable messages in any other order than by UID, two of them in one
+     * folder would take the counter from each other on every sync, neither
+     * would reach the attempt that lets it go, and the folder would be held
+     * between them for good.
+     */
+    public function testTwoUnreadableMessagesDoNotHoldAFolderBetweenThem(): void
+    {
+        $unreadable = [
+            104 => new \RuntimeException('no content found'),
+            102 => new \RuntimeException('no content found'),
+        ];
+
+        for ($sync = 1; $sync <= 5; ++$sync) {
+            $this->storeBatchWith($unreadable, $this->mail(101), $this->mail(103), $this->mail(105));
+        }
+
+        $mailbox = $this->mailboxRow();
+
+        self::assertSame(103, (int) $mailbox['last_seen_uid'], 'the lower one was counted five times and let go');
+        self::assertSame(104, (int) $mailbox['failed_uid'], 'and the counter has moved on to the next');
+        self::assertSame(1, (int) $mailbox['failed_uid_attempts']);
+    }
+
+    /**
+     * `lastSeenUid+1:*` hands back the newest message on every sync. If the
+     * library trips over it this time, it is still a message this folder
+     * holds, and holding it back would reset the counter a real failure is
+     * being counted on.
+     */
+    public function testAMessageAlreadyStoredIsNotAFailureWhateverTheLibraryMadeOfItThisTime(): void
+    {
+        $this->storeBatch($this->mail(101));
+
+        $synced = [101 => true];
+
+        $this->invokeProcessBatch([], $synced, [101 => new \RuntimeException('no content found')]);
+
+        $mailbox = $this->mailboxRow();
+
+        self::assertNull($mailbox['failed_uid']);
+        self::assertSame(101, (int) $mailbox['last_seen_uid']);
+    }
+
     private function storeBatch(ImapMessage ...$batch): void
     {
-        $synced   = [];
+        $synced = [];
+
+        $this->invokeProcessBatch($batch, $synced, []);
+    }
+
+    /**
+     * One sync's worth: the mark and the stored UIDs read fresh, as
+     * syncMailbox() reads them before it fetches.
+     *
+     * @param array<int, \Throwable> $unreadable
+     */
+    private function storeBatchWith(array $unreadable, ImapMessage ...$batch): void
+    {
+        $synced = array_fill_keys($this->storedUids(), true);
+
+        $this->invokeProcessBatch($batch, $synced, $unreadable, (int) $this->mailboxRow()['last_seen_uid']);
+    }
+
+    /** @return list<int> */
+    private function storedUids(): array
+    {
+        return array_map('intval', $this->connection->fetchFirstColumn(
+            'SELECT imap_uid FROM message WHERE mailbox_id = ? ORDER BY imap_uid',
+            [$this->mailboxId],
+        ));
+    }
+
+    /** @return array<string, mixed> */
+    private function mailboxRow(): array
+    {
+        $this->em->clear();
+
+        return (array) $this->connection->fetchAssociative(
+            'SELECT failed_uid, failed_uid_attempts, last_seen_uid FROM mailbox WHERE id = ?',
+            [$this->mailboxId],
+        );
+    }
+
+    /**
+     * @param array<int, ImapMessage> $batch
+     * @param array<int, bool>        $synced
+     * @param array<int, \Throwable>  $unreadable
+     */
+    private function invokeProcessBatch(array $batch, array &$synced, array $unreadable, int $lastSeenUid = 0): void
+    {
+        // The run-wide lowest held UID: a run of one batch starts with none.
         $lowest   = null;
         $presence = new ImapUidPresence(
             $this->account,
@@ -192,7 +342,7 @@ final class BatchStorageTest extends KernelTestCase
 
         new \ReflectionMethod(MessageSyncer::class, 'processBatch')->invokeArgs(
             static::getContainer()->get(MessageSyncer::class),
-            [$batch, $this->mailboxId, (int) $this->account->id, 0, &$synced, &$lowest, $presence],
+            [$batch, $this->mailboxId, (int) $this->account->id, $lastSeenUid, &$synced, &$lowest, $presence, $unreadable],
         );
     }
 

tests/Service/Imap/FakeQuery.php

@@ -0,0 +1,76 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Service\Imap;
+
+use Webklex\PHPIMAP\Exceptions\GetMessagesFailedException;
+use Webklex\PHPIMAP\Message;
+use Webklex\PHPIMAP\Query\Query;
+use Webklex\PHPIMAP\Support\MessageCollection;
+use Illuminate\Support\Collection;
+
+/**
+ * A search with a scripted answer: which UIDs it finds, and what building each
+ * one into a message does.
+ *
+ * It keeps the library's own behaviour in the two places ImapPagedFetch
+ * depends on, copied from Query rather than imagined: a message that cannot be
+ * built is recorded against its UID, and whether that ends the fetch is decided
+ * by soft-fail alone. Everything that would need a server — the search, the
+ * fetch — is replaced by the script.
+ *
+ * Made with newInstanceWithoutConstructor(), because the real constructor asks
+ * a connected client for its configuration.
+ */
+final class FakeQuery extends Query
+{
+    /** @var array<int, Message|\Throwable> UID => the message, or what building it throws */
+    private array $script = [];
+
+    /** @var list<int> the page number of every fetch, in order */
+    public array $fetchedPages = [];
+
+    /**
+     * @param array<int, Message|\Throwable> $script
+     */
+    public static function finding(array $script): self
+    {
+        $query = new \ReflectionClass(self::class)->newInstanceWithoutConstructor();
+        $query->script = $script;
+
+        return $query;
+    }
+
+    public function search(): Collection
+    {
+        return new Collection(array_keys($this->script));
+    }
+
+    public function curate_messages(Collection $available_messages): MessageCollection
+    {
+        $this->fetchedPages[] = $this->page;
+
+        $messages = MessageCollection::make();
+
+        foreach ($available_messages->forPage($this->page, $this->limit) as $uid) {
+            $entry = $this->script[$uid];
+
+            if ($entry instanceof \Throwable) {
+                $this->setError($uid, $entry instanceof \Exception ? $entry : new \RuntimeException($entry->getMessage()));
+
+                // Query::handleException(), which is what makes one unreadable
+                // message end a fetch the library was not told to soften.
+                if (false === $this->soft_fail) {
+                    throw new GetMessagesFailedException($entry->getMessage(), 0, $entry);
+                }
+
+                continue;
+            }
+
+            $messages->put((string) $uid, $entry);
+        }
+
+        return $messages;
+    }
+}

tests/Service/Imap/ImapPagedFetchTest.php

@@ -0,0 +1,134 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Service\Imap;
+
+use App\Domain\Helper\ImapConnectionFactory;
+use App\Service\Imap\ImapPagedFetch;
+use PHPUnit\Framework\TestCase;
+use Webklex\PHPIMAP\Exceptions\GetMessagesFailedException;
+use Webklex\PHPIMAP\Exceptions\MessageContentFetchingException;
+use Webklex\PHPIMAP\Message;
+
+/**
+ * One message the library cannot read costs that message, not the fetch.
+ *
+ * The library's own paging gave up the whole fetch for one unreadable message,
+ * so the folder it was in never synced again (#45). Its soft-fail setting is
+ * the obvious answer and cannot be used with its own loop, which runs until
+ * the messages handed over add up to the messages found — a number a dropped
+ * message makes unreachable, so it asks for empty pages for ever. Both faults
+ * are pinned here: every page is fetched exactly once, and what was dropped is
+ * handed over with the page it was dropped from.
+ *
+ * Against a scripted query (see FakeQuery) rather than a server: the claims
+ * are about a loop — how many times it goes round, and what it reports each
+ * time — and a server adds nothing to them but a way to be slow.
+ */
+final class ImapPagedFetchTest extends TestCase
+{
+    public function testAnUnreadableMessageIsReportedWithItsPageAndTheRestStillArrive(): void
+    {
+        $query = FakeQuery::finding([
+            1 => $this->mail(1),
+            2 => new MessageContentFetchingException('no content found'),
+            3 => $this->mail(3),
+            4 => $this->mail(4),
+            5 => $this->mail(5),
+        ]);
+
+        $pages = $this->collect($query, 2);
+
+        self::assertSame([[1], [3, 4], [5]], array_column($pages, 'uids'), 'everything readable arrives, page by page');
+        self::assertSame([[2], [], []], array_column($pages, 'unreadable'), 'and the one that is not is named once, with its page');
+    }
+
+    /**
+     * The loop this replaces counted what it handed over until it reached what
+     * the search found. With one message dropped it never got there.
+     */
+    public function testEveryPageIsAskedForOnceEvenWhenAMessageIsDropped(): void
+    {
+        $query = FakeQuery::finding([
+            1 => new MessageContentFetchingException('no content found'),
+            2 => $this->mail(2),
+            3 => $this->mail(3),
+        ]);
+
+        $this->collect($query, 2);
+
+        self::assertSame([1, 2], $query->fetchedPages);
+    }
+
+    /** A whole page of them is still a page: nothing arrives, and the fetch goes on. */
+    public function testAPageOfNothingButUnreadableMessagesDoesNotEndTheFetch(): void
+    {
+        $query = FakeQuery::finding([
+            1 => new MessageContentFetchingException('no content found'),
+            2 => new MessageContentFetchingException('no content found'),
+            3 => $this->mail(3),
+        ]);
+
+        $pages = $this->collect($query, 2);
+
+        self::assertSame([[], [3]], array_column($pages, 'uids'));
+        self::assertSame([[1, 2], []], array_column($pages, 'unreadable'));
+    }
+
+    public function testASearchThatFindsNothingFetchesNothing(): void
+    {
+        $query = FakeQuery::finding([]);
+
+        self::assertSame([], $this->collect($query, 50));
+        self::assertSame([], $query->fetchedPages);
+    }
+
+    /**
+     * The contrast that says the fake is honest: left as the library ships,
+     * the same script ends at the unreadable message. If this did not throw,
+     * the tests above would be passing against a query that never failed.
+     */
+    public function testWithoutSoftFailTheSameSearchEndsAtTheUnreadableMessage(): void
+    {
+        $query = FakeQuery::finding([
+            1 => $this->mail(1),
+            2 => new MessageContentFetchingException('no content found'),
+        ]);
+
+        $this->expectException(GetMessagesFailedException::class);
+
+        $query->limit(50, 1)->curate_messages($query->search());
+    }
+
+    /**
+     * @return list<array{uids: list<int>, unreadable: list<int>}>
+     */
+    private function collect(FakeQuery $query, int $pageSize): array
+    {
+        $pages = [];
+
+        foreach (new ImapPagedFetch()->pages($query, $pageSize) as [$batch, $unreadable]) {
+            $uids = [];
+
+            foreach ($batch as $message) {
+                $uids[] = (int) $message->getUid();
+            }
+
+            $pages[] = ['uids' => $uids, 'unreadable' => array_keys($unreadable)];
+        }
+
+        return $pages;
+    }
+
+    private function mail(int $uid): Message
+    {
+        $raw = "From: sender@example.test\r\n"
+            . sprintf("Subject: Mail %d\r\n", $uid)
+            . "Content-Type: text/plain; charset=utf-8\r\n"
+            . "\r\n"
+            . "hello\r\n";
+
+        return Message::fromString($raw, ImapConnectionFactory::config())->setUid($uid);
+    }
+}