Leave a message that is already in the destination folder where it is (#270)

Trashing a conversation with a message already in the bin queued that message as a move from Trash into Trash. webklex's move() then left the connection in EXAMINE and the server refused it ("NO Not permitted in EXAMINE state"), and the row had already lost its UID. ApplyImapFlagsHandler no longer sends a move into the folder a message is in (and gives the row its UID back), and ThreadStatusUpdater's trash(), archive() and restore() leave such messages out of the move.

Closes #269

Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -65,6 +65,15 @@ message alone; it is now counted from all of them. A conversation whose count is
 put right the next time one of its messages is marked. No schema change.
 [#266](https://github.com/karatektus/pl_mail/issues/266), by @phdelodder.
 
+**Trashing a conversation with one message already in the bin no longer fails.** On an IMAP account,
+the message that was already in the Trash was sent to the server as a move from Trash into Trash,
+which the server refused with "NO Not permitted in EXAMINE state". The rest of the conversation
+moved, but the refusal was logged as an error and recorded on the account as a change the server
+would not take, and the binned message lost its address until the next sync found it again. A
+message that is already where a trash, archive or restore sends it now stays put and keeps its
+address; its labels change as before. No schema change.
+[#269](https://github.com/karatektus/pl_mail/issues/269), by @phdelodder.
+
 ## v0.3.7 — 2026-10-20
 
 <!-- highlight screenshot=sso-only -->

src/Infrastructure/Messaging/Handler/ApplyImapFlagsHandler.php

@@ -269,6 +269,12 @@ final class ApplyImapFlagsHandler
         ApplyImapFlagsMessage $envelope,
         ?string               $destinationPath,
     ): void {
+        if (true === in_array($envelope->action, self::MOVE_ACTIONS, true) && $sourceMailbox->fullPath === $destinationPath) {
+            $this->stayInPlace($sourceMailbox, $messages, $envelope);
+
+            return;
+        }
+
         $folder = ImapFolderLocator::of($client, $sourceMailbox);
 
         if (null === $folder) {
@@ -313,6 +319,45 @@ final class ApplyImapFlagsHandler
         }
     }
 
+    /**
+     * A move into the folder the messages are already in: nothing to send, and
+     * the rows get their address back.
+     *
+     * Not merely a wasted command. webklex's move() opens the destination,
+     * EXAMINEs it, then opens the source again — and Client::openFolder() skips
+     * the SELECT when that folder is the one already open. With source and
+     * destination the same, the connection is left read-only and the server
+     * answers the MOVE with "NO Not permitted in EXAMINE state" (#269). That
+     * was logged as an error and recorded as a refused export, for a message
+     * that was exactly where it was meant to be.
+     *
+     * The caller cleared each row's UID when it queued the move
+     * (Message::relocateTo()), so the envelope's UID goes back on: the message
+     * never left the folder, and the address it had is still its address. A row
+     * that something else has re-pointed since, or that has a UID again, is
+     * left to the sync, as adopt() leaves it.
+     *
+     * @param Message[] $messages
+     */
+    private function stayInPlace(Mailbox $mailbox, array $messages, ApplyImapFlagsMessage $envelope): void
+    {
+        foreach ($messages as $msg) {
+            $uid = $envelope->sourceUidFor((int) $msg->id);
+
+            if (null === $uid || null !== $msg->imapUid || $msg->mailbox?->fullPath !== $mailbox->fullPath) {
+                continue;
+            }
+
+            $msg->relocateTo($msg->mailbox, $uid);
+        }
+
+        $this->logger->info('ApplyImapFlagsHandler: already in the destination, not moving', [
+            'mailbox' => $mailbox->fullPath,
+            'action'  => $envelope->action,
+            'count'   => count($messages),
+        ]);
+    }
+
     private function applyToMessage(
         Client  $client,
         Folder  $folder,

src/Service/Mail/ThreadStatusUpdater.php

@@ -14,6 +14,7 @@ use App\Entity\Mail\InboxCategory;
 use App\Domain\Enum\Mail\MessageFlag;
 use App\Entity\Label\Label;
 use App\Entity\Mail\Account;
+use App\Entity\Mail\Mailbox;
 use App\Entity\Mail\Message;
 use App\Entity\Mail\MessageThread;
 use App\Infrastructure\Messaging\Message\SendReadReceiptMessage;
@@ -173,13 +174,13 @@ final readonly class ThreadStatusUpdater
 
         $inboxLabel = $this->labels->findOneByRoleForUser(LabelRole::Inbox, $account->usr);
 
-        // Propagate BEFORE re-pointing mailboxes so the IMAP job captures
-        // the correct source folders.
-        $this->propagator->archive($messages);
-
         $archiveLabel   = $this->labelResolver->systemLabel(LabelRole::Archive, $account);
         $archiveMailbox = $archiveLabel->bindingFor($account)?->mailbox;
 
+        // Propagate BEFORE re-pointing mailboxes so the IMAP job captures
+        // the correct source folders.
+        $this->propagator->archive($this->notAlreadyIn($messages, $archiveMailbox));
+
         foreach ($messages as $message) {
             // Archiving used to be "remove Inbox" and nothing else, which read
             // as Gmail's rule and was not: Gmail keeps the mail in All Mail,
@@ -200,7 +201,7 @@ final readonly class ThreadStatusUpdater
             // and gets a new UID there that only the mover can report. Until
             // then the row names the destination and no address inside it —
             // see Message::relocateTo().
-            if (null !== $message->imapUid && null !== $archiveMailbox) {
+            if (null !== $message->imapUid && null !== $archiveMailbox && $message->mailbox !== $archiveMailbox) {
                 $message->relocateTo($archiveMailbox);
             }
         }
@@ -218,10 +219,10 @@ final readonly class ThreadStatusUpdater
         $inboxLabel = $this->labels->findOneByRoleForUser(LabelRole::Inbox, $account->usr);
         $trashLabel = $this->labelResolver->systemLabel(LabelRole::Trash, $account);
 
-        $this->propagator->trash($messages);
-
         $trashMailbox = $trashLabel->bindingFor($account)?->mailbox;
 
+        $this->propagator->trash($this->notAlreadyIn($messages, $trashMailbox));
+
         foreach ($messages as $message) {
             $message->addLabel($trashLabel);
 
@@ -229,7 +230,7 @@ final readonly class ThreadStatusUpdater
                 $message->removeLabel($inboxLabel);
             }
 
-            if (null !== $message->imapUid && null !== $trashMailbox) {
+            if (null !== $message->imapUid && null !== $trashMailbox && $message->mailbox !== $trashMailbox) {
                 $message->relocateTo($trashMailbox);
             }
         }
@@ -263,12 +264,12 @@ final readonly class ThreadStatusUpdater
         $spamLabel    = $this->labels->findOneByRoleForUser(LabelRole::Spam, $account->usr);
         $archiveLabel = $this->labels->findOneByRoleForUser(LabelRole::Archive, $account->usr);
 
+        $inboxMailbox = $inboxLabel->bindingFor($account)?->mailbox;
+
         // Before the mailbox is re-pointed, so the IMAP job still knows which
         // folder to move the message out of — the same ordering archive() and
         // trash() rely on, and for the same reason.
-        $this->propagator->restore($messages);
-
-        $inboxMailbox = $inboxLabel->bindingFor($account)?->mailbox;
+        $this->propagator->restore($this->notAlreadyIn($messages, $inboxMailbox));
 
         // Mail in Spam or the bin is not read for calendar events
         // (ExtractEventsHandler), so a message coming back out has never been
@@ -302,7 +303,7 @@ final readonly class ThreadStatusUpdater
                 }
             }
 
-            if (null !== $message->imapUid && null !== $inboxMailbox) {
+            if (null !== $message->imapUid && null !== $inboxMailbox && $message->mailbox !== $inboxMailbox) {
                 $message->relocateTo($inboxMailbox);
             }
         }
@@ -944,6 +945,34 @@ final readonly class ThreadStatusUpdater
         return $locations;
     }
 
+    /**
+     * The messages a move into $mailbox has to send to the server: all of them
+     * but the ones already in it.
+     *
+     * A conversation spans folders — a reply arrives in the Inbox while the
+     * rest of the thread sits in the bin — so trashing, archiving or restoring
+     * the whole thread reaches messages that are already where it is going.
+     * Queueing those as well asked the server to move a message into its own
+     * folder, which IMAP refuses (#269, see ApplyImapFlagsHandler::stayInPlace()),
+     * and the relocateTo() that follows cleared a UID that was still correct.
+     * Their labels change all the same; only the move is left out.
+     *
+     * @param list<Message> $messages
+     *
+     * @return list<Message>
+     */
+    private function notAlreadyIn(array $messages, ?Mailbox $mailbox): array
+    {
+        if (null === $mailbox) {
+            return $messages;
+        }
+
+        return array_values(array_filter(
+            $messages,
+            static fn (Message $message): bool => null === $message->imapUid || $message->mailbox !== $mailbox,
+        ));
+    }
+
     /**
      * Filing a conversation ends the wait it was put on.
      *

tests/Infrastructure/Messaging/Handler/ApplyImapFlagsOutboundOperationsTest.php

@@ -179,6 +179,27 @@ final class ApplyImapFlagsOutboundOperationsTest extends TestCase
         self::assertSame('INBOX.Trash', $present->movedTo);
     }
 
+    /**
+     * A message already in the Trash stays there, and keeps its address.
+     *
+     * webklex's move() into the folder it is moving out of leaves the
+     * connection in EXAMINE, and the server refuses the MOVE: "NO Not
+     * permitted in EXAMINE state" (#269). The row arrives with no UID —
+     * relocateTo() cleared it when the job was queued — and the envelope's UID
+     * is still right, because the message never left the folder that issued it.
+     */
+    public function testTrashingAMessageAlreadyInTheTrashDoesNotMoveIt(): void
+    {
+        $present = new RecordingImapMessage(uid: 6, rfcMessageId: 'abc@example.test');
+
+        $row = $this->issue($present, 'trash', rowMessageId: 'abc@example.test', rowLandsIn: 'INBOX.Trash', sourceIsTrash: true);
+
+        self::assertSame(0, $present->moveCalls, 'a move from Trash into Trash was sent to the server');
+        self::assertSame([], $present->calls);
+        self::assertSame('INBOX.Trash', $row->mailbox?->fullPath);
+        self::assertSame(6, $row->imapUid, 'the row should have its address in Trash back');
+    }
+
     public function testArchivingMovesToTheAccountsArchiveFolder(): void
     {
         $present = new RecordingImapMessage(uid: 6);
@@ -335,12 +356,15 @@ final class ApplyImapFlagsOutboundOperationsTest extends TestCase
         ?string              $rowMessageId = null,
         ?string              $rowLandsIn = null,
         bool                 $destinationExists = true,
+        bool                 $sourceIsTrash = false,
     ): Message {
         $account      = new Account();
         $account->usr = new User();
 
-        $source = $this->mailbox($account, 'INBOX', 'INBOX', MailboxSpecialUse::INBOX);
         $trash  = $this->mailbox($account, 'Trash', 'INBOX.Trash', MailboxSpecialUse::TRASH);
+        $source = true === $sourceIsTrash
+            ? $trash
+            : $this->mailbox($account, 'INBOX', 'INBOX', MailboxSpecialUse::INBOX);
 
         $row            = new Message();
         $row->account   = $account;

tests/Service/Mail/ThreadMoveTest.php

@@ -181,6 +181,54 @@ final class ThreadMoveTest extends KernelTestCase
         self::assertContains((int) $message->id, array_keys($moves[0]->messageIds));
     }
 
+    /**
+     * Trashing a conversation that is partly in the bin already moves only the
+     * part that is not.
+     *
+     * The shape is common: a message is deleted, a reply arrives in the Inbox,
+     * and the whole conversation is trashed. The binned message used to be
+     * queued too, as a move from Trash into Trash, which the server refuses
+     * (#269) — and its UID was cleared on the way, though the message never
+     * left the folder that issued it.
+     */
+    public function testTrashingAThreadLeavesTheMessageAlreadyInTheBinWhereItIs(): void
+    {
+        $inbox        = $this->labelResolver->systemLabel(LabelRole::Inbox, $this->account);
+        $trash        = $this->labelResolver->systemLabel(LabelRole::Trash, $this->account);
+        $trashMailbox = $this->seedMailbox('Trash');
+
+        $this->labelResolver->bindMailbox($inbox, $this->inboxMailbox);
+        $this->labelResolver->bindMailbox($trash, $trashMailbox);
+
+        $thread = $this->thread();
+        $reply  = $thread->messages->first();
+        $reply->addLabel($inbox);
+
+        $binned = $this->message('Already deleted');
+        $binned->relocateTo($trashMailbox, 4242);
+        $binned->addLabel($trash);
+        $thread->addMessage($binned);
+        $this->em->persist($binned);
+        $this->em->flush();
+
+        $this->status->trash([$reply, $binned]);
+
+        $queued = [];
+
+        foreach ($this->dispatched() as $job) {
+            if ('trash' === $job->action) {
+                array_push($queued, ...array_keys($job->messageIds));
+            }
+        }
+
+        self::assertContains((int) $reply->id, $queued, 'the Inbox message was not sent to the bin');
+        self::assertNotContains((int) $binned->id, $queued, 'a message already in the bin was queued to move into it');
+        self::assertSame($trashMailbox, $binned->mailbox);
+        self::assertSame(4242, $binned->imapUid, 'the binned message lost a UID that was still its address');
+        self::assertTrue($binned->labels->contains($trash), 'the binned message lost its Trash label');
+        self::assertSame($trashMailbox, $reply->mailbox, 'the Inbox message was not re-pointed at the bin');
+    }
+
     /**
      * The reason MessageThread::$categoryPinnedAt exists.
      *