Mail the AI host could not be reached for is asked again by the catch-up (#231)

The categorisation stamp was written even when the call never reached a model, so a short outage left every message in flight without a category for good. A timeout, an unreachable host or a refusal by 401, 403, 408, 429 or a 5xx now leaves the message unstamped for the catch-up; an answer, a 404 and a refusal of the request itself (400, 413, 422) are stamped as before. Closes #222.

Co-authored-by: Philippe Delodder <philippe@delodder.be>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

tests/Service/Ai/OpenAiClientTest.php

@@ -103,6 +103,24 @@ final class OpenAiClientTest extends TestCase
 
         self::assertFalse($result->succeeded);
         self::assertSame(OllamaClient::ERROR_HTTP_404, $result->errorKind);
+        self::assertSame(404, $result->httpStatus);
+    }
+
+    /**
+     * The status itself travels with the refusal, because the kind folds every
+     * one but 404 together, and a caller deciding whether to ask again needs
+     * to tell a 503 that will pass from a content filter's 400 that will not
+     * (ClassifyMailHandler, #222).
+     */
+    public function testARefusalCarriesItsStatus(): void
+    {
+        foreach ([400, 503] as $status) {
+            $result = $this->answering('{"error":{"message":"no"}}', $status)
+                ->chat($this->connection(), 'a-model', [['role' => 'user', 'content' => 'hi']]);
+
+            self::assertSame(OllamaClient::ERROR_HTTP_STATUS, $result->errorKind);
+            self::assertSame($status, $result->httpStatus);
+        }
     }
 
     /** A 200 that is a proxy's HTML is a failure with a name, not an exception. */
@@ -240,6 +258,7 @@ final class OpenAiClientTest extends TestCase
         self::assertSame(['Half a sen'], $tokens, 'what arrived was shown');
         self::assertFalse($result->succeeded);
         self::assertSame(OllamaClient::ERROR_HTTP_STATUS, $result->errorKind);
+        self::assertNull($result->httpStatus, 'the response was a 200; what failed was inside it');
     }
 
     public function testARefusedStreamYieldsNothingAndSaysWhy(): void

tests/Service/Mail/ClassificationHoldFlowTest.php

@@ -37,6 +37,7 @@ use DateTimeImmutable;
 use Doctrine\DBAL\Connection;
 use Doctrine\ORM\EntityManagerInterface;
 use Psr\Log\NullLogger;
+use PHPUnit\Framework\Attributes\DataProvider;
 use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
 use Symfony\Component\HttpClient\MockHttpClient;
 use Symfony\Component\HttpClient\Response\MockResponse;
@@ -233,9 +234,9 @@ final class ClassificationHoldFlowTest extends KernelTestCase
 
     /**
      * The model host is a box on somebody's network and may simply be off.
-     * Held mail must come out anyway, at once, under the rules' answer. It
-     * is stamped as asked, like any other unanswered message: the question was
-     * put once and is not put again.
+     * Held mail must come out anyway, at once, under the rules' answer. It is
+     * not stamped as asked, because nothing was: the catch-up sweep offers it
+     * again once the host is back (#222).
      */
     public function testAHostThatDoesNotAnswerReleasesTheMailUnderTheRules(): void
     {
@@ -250,10 +251,70 @@ final class ClassificationHoldFlowTest extends KernelTestCase
         $this->em->refresh($message->thread);
 
         self::assertFalse($message->isCategoryHeld());
-        self::assertNotNull($message->aiCategorisedAt);
+        self::assertNull($message->aiCategorisedAt, 'an unreachable host asked nothing');
         self::assertSame(MessageCategory::Primary, $message->thread->category);
     }
 
+    /**
+     * #222. A minute's outage left 327 messages with no category and no second
+     * attempt, because the failed call was stamped as an answer. A refusal
+     * that says "not now" is the same case: a credential being rotated, a
+     * proxy in front of a host that is restarting, a rate limit, any 5xx.
+     */
+    #[DataProvider('passingRefusals')]
+    public function testARefusalThatWillPassLeavesTheMessageForTheCatchUp(int $status): void
+    {
+        $message = $this->askedAbout(new MockResponse('', ['http_code' => $status]));
+
+        self::assertNull($message->aiCategorisedAt, $status . ' is not an answer');
+        self::assertContains((int) $message->id, $this->unclassified());
+    }
+
+    /** @return iterable<string, array{int}> */
+    public static function passingRefusals(): iterable
+    {
+        foreach ([401, 403, 408, 429, 500, 502, 503, 504] as $status) {
+            yield (string) $status => [$status];
+        }
+    }
+
+    /**
+     * A refusal of the request itself comes back the same on every retry: a
+     * content filter's 400 for this mail, a 413, a 422. Retried, a handful of
+     * them would take the catch-up's whole budget (it offers the newest
+     * unstamped mail first) every quarter of an hour, for good. And a 404 is a
+     * model the host does not have: a setting to change, not an outage.
+     */
+    #[DataProvider('lastingRefusals')]
+    public function testARefusalThatWouldRepeatIsStamped(int $status): void
+    {
+        $message = $this->askedAbout(new MockResponse('', ['http_code' => $status]));
+
+        self::assertNotNull($message->aiCategorisedAt, $status . ' would only repeat');
+        self::assertNotContains((int) $message->id, $this->unclassified());
+    }
+
+    /** @return iterable<string, array{int}> */
+    public static function lastingRefusals(): iterable
+    {
+        foreach ([400, 404, 413, 422] as $status) {
+            yield (string) $status => [$status];
+        }
+    }
+
+    /**
+     * The other half of the promise: a host that did answer is not asked
+     * twice, however useless the answer. The stamp is what keeps a message the
+     * model cannot sort from being offered on every pass for ever.
+     */
+    public function testAHostThatAnswersWithNothingUsableIsNotAskedAgain(): void
+    {
+        $message = $this->askedAbout(new MockResponse((string) json_encode(['message' => ['content' => 'hmm']])));
+
+        self::assertNotNull($message->aiCategorisedAt);
+        self::assertNotContains((int) $message->id, $this->unclassified());
+    }
+
     /**
      * What Admin → AI shows beside the switch: how long holding delayed mail,
      * and how often the wait ran out with nothing to show for it.
@@ -366,13 +427,40 @@ final class ClassificationHoldFlowTest extends KernelTestCase
      * assistant is the container's own.
      */
     private function handlerWithAModelThatSays(string $answer): ClassifyMailHandler
+    {
+        return $this->handlerAround(new MockResponse((string) json_encode(['message' => ['content' => $answer]])));
+    }
+
+    /** One fresh message put to a host that gives this response, and the message as the handler left it. */
+    private function askedAbout(MockResponse $response): Message
+    {
+        $this->assistantSorts();
+        $this->importFinished();
+
+        $message = $this->message('asked', '-1 minute');
+        $this->ingest($message);
+
+        ($this->handlerAround($response))(new ClassifyMailMessage([(int) $message->id]));
+
+        $this->em->refresh($message);
+
+        return $message;
+    }
+
+    /** @return list<int> what the catch-up sweep would offer this person */
+    private function unclassified(): array
+    {
+        return self::getContainer()->get(MessageRepository::class)->unclassifiedIdsForUser((int) $this->user->id, 100);
+    }
+
+    private function handlerAround(MockResponse $response): ClassifyMailHandler
     {
         $container = self::getContainer();
 
         $assistant = new AiAssistant(
             $container->get(AiSettingsRepository::class),
             new OllamaClient(
-                new MockHttpClient(new MockResponse((string) json_encode(['message' => ['content' => $answer]]))),
+                new MockHttpClient($response),
                 new NullLogger(),
             ),
             new OpenAiClient(new MockHttpClient([]), new NullLogger()),