From 412d67955a9a80b715ec275f34128add995e9d17 Mon Sep 17 00:00:00 2001 From: nexxo Date: Mon, 3 Aug 2026 15:02:18 +0200 Subject: [PATCH] Fix-Runde 2: Drossel-Marke erst nach Erfolg, Kommentar richtiggestellt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Die Marke stand bisher VOR dem Versandversuch - scheiterte das Einreihen, schwieg das Subjekt fuer den Rest der Stunde, obwohl null statt einer Mail angekommen war. Jetzt steht sie in notifyInstanceOwner() erst nach dem erfolgreichen queue() im try-Block, in notifyHostManagers() erst nach mindestens einem geglueckten Einreihen (Merker ueber die Schleife, da ein einzelnes abgelehntes Postfach weder die uebrigen Betreiber noch die Marke fuer alle blockieren darf). Kein Sturm-Risiko: Sperren entstehen ohnehin nur ab der Zehner-Schwelle, nicht bei jedem Fehlversuch. Die Kommentare behaupteten außerdem, das try/catch finge Zustellungsfehler ab - tatsaechlich faengt es nur, was beim EINREIHEN schiefgeht (synchron, vor der Warteschlange); ein Zustellungsfehler passiert spaeter im Warteschlangen-Arbeiter und steht in dessen Protokoll. Beide Docblocks richtiggestellt. Neuer Testfall haengt einen Wrapper vor die gefakte Mail-Fassade, dessen erster to()-Aufruf wirft und ab dem zweiten an die echte Fake-Instanz durchreicht - MailFake::queue() selbst kann einen Fehlschlag nicht simulieren, weil es den Mailable nur ablegt und dabei nie wirft. Als Gegenprobe testweise auf den alten Code zurueckgesetzt: Testfall lief rot mit der erwarteten Meldung, Datei danach byte-identisch wiederhergestellt. Co-Authored-By: Claude Opus 5 --- app/Services/Security/BlockAddress.php | 45 ++++++++++++++----- .../Security/SecurityBlockMailTest.php | 38 ++++++++++++++++ 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/app/Services/Security/BlockAddress.php b/app/Services/Security/BlockAddress.php index 54f9cd7..57d6021 100644 --- a/app/Services/Security/BlockAddress.php +++ b/app/Services/Security/BlockAddress.php @@ -89,13 +89,21 @@ class BlockAddress * Empfängerlogik (Betreiber statt Kunde), gleiche Drossel-Idee. * * Zustellung ist nicht die Bedingung für Schutz: die Sperre steht bereits, - * bevor hier auch nur versucht wird zu verschicken, und die Drossel wird - * VOR dem Versandversuch gesetzt, nicht danach — sonst würde ein - * dauerhaft kaputtes Postfach bei jeder weiteren Sperre an derselben - * Instanz erneut versuchen und denselben Fehler immer wieder melden. Ein - * Throwable beim Verschicken wird über `report()` gemeldet und - * verschluckt: ein kaputtes Postfach darf die Sperre nicht rückgängig - * machen, die es eigentlich ankündigen sollte. + * bevor hier auch nur versucht wird zu verschicken. Die Drossel-Marke + * wird erst NACH dem erfolgreichen Einreihen gesetzt, nicht davor — + * sonst bliebe die Instanz nach einem einzigen fehlgeschlagenen Versuch + * für den Rest der Stunde ganz ohne Meldung, ausgerechnet dann, wenn statt + * einer Mail null angekommen sind. Ein Sturm bei dauerhaft kaputtem + * Postfach ist trotzdem ausgeschlossen: Sperren selbst entstehen schon + * nur ab der Zehner-Schwelle, nicht bei jedem Fehlversuch. + * + * `try/catch` fängt hier nur, was beim EINREIHEN schiefgeht — ein + * synchroner Fehler, bevor die Mail überhaupt in der Warteschlange liegt + * (`SecurityBlockMail` implementiert `ShouldQueue`). Was bei der + * tatsächlichen ZUSTELLUNG schiefgeht, passiert später, in einem + * Warteschlangen-Arbeiter, lange nachdem diese Methode zurückgekehrt ist, + * und steht in dessen Protokoll, nicht hier. Die Sperre bleibt in beiden + * Fällen stehen — das ist die eigentliche Zusage. */ private function notifyInstanceOwner(Instance $instance, SecurityBlock $block): void { @@ -106,10 +114,9 @@ class BlockAddress return; } - Settings::set($key, now()->toIso8601String()); - try { Mail::to($instance->customer->email)->queue(new SecurityBlockMail($block)); + Settings::set($key, now()->toIso8601String()); } catch (Throwable $e) { report($e); } @@ -132,6 +139,17 @@ class BlockAddress * neben dem Owner, oder eine, in der `hosts.manage` gerade niemandem * zugewiesen ist, bekommt schlicht keine Mail — die Sperre selbst steht * trotzdem, unabhängig davon. + * + * Die Drossel-Marke wird erst gesetzt, wenn mindestens ein Einreihen + * geglückt ist — aus demselben Grund wie bei `notifyInstanceOwner()`: ein + * Host, bei dem in einer Stunde ausgerechnet null statt einer Mail + * ankamen, soll nicht auch noch für den Rest der Stunde verstummen. Jede + * Adresse steht in ihrem EIGENEN `try/catch`, das nur das EINREIHEN + * abdeckt (synchron, vor der Warteschlange) — nicht die tatsächliche + * Zustellung, die später im Warteschlangen-Arbeiter passiert und dort + * protokolliert wird. Ein einzelnes abgelehntes Postfach darf so weder + * die übrigen Betreiber um ihre Meldung bringen noch die Marke für alle + * verhindern, solange wenigstens einer sie bekommen hat. */ private function notifyHostManagers(Host $host, SecurityBlock $block): void { @@ -151,17 +169,20 @@ class BlockAddress return; } - Settings::set($key, now()->toIso8601String()); + $queued = false; - // Einzeln umschlossen: ein Postfach, das ablehnt, darf die übrigen - // Betreiber nicht auch noch um ihre Meldung bringen. foreach ($recipients as $email) { try { Mail::to($email)->queue(new SecurityBlockMail($block)); + $queued = true; } catch (Throwable $e) { report($e); } } + + if ($queued) { + Settings::set($key, now()->toIso8601String()); + } } /** Läuft für diese Adresse an diesem Subjekt schon eine Sperre? */ diff --git a/tests/Feature/Security/SecurityBlockMailTest.php b/tests/Feature/Security/SecurityBlockMailTest.php index a3fbe65..1efeba8 100644 --- a/tests/Feature/Security/SecurityBlockMailTest.php +++ b/tests/Feature/Security/SecurityBlockMailTest.php @@ -55,6 +55,44 @@ it('laesst die Sperre stehen, wenn die Mail scheitert', function () { ->and(SecurityBlock::count())->toBe(1); }); +it('drosselt nicht, wenn gar keine Mail hinausging', function () { + // Sonst schweigt eine Instanz eine volle Stunde, obwohl NULL Meldungen + // ankamen — genau dann, wenn ohnehin schon etwas klemmt. + Mail::fake(); + + // Ein Postfach, das den ERSTEN Aufruf von Mail::to() wirft und danach an + // die echte Fake-Implementierung durchreicht — so laesst sich "der erste + // Versandversuch scheitert, der zweite geht durch" nachstellen, ohne die + // Zusicherung am Ende (Mail::assertQueued) aufzugeben. + $fake = Mail::getFacadeRoot(); + Mail::swap(new class($fake) + { + private int $calls = 0; + + public function __construct(private readonly object $inner) {} + + public function __call(string $name, array $arguments): mixed + { + if ($name === 'to' && ++$this->calls === 1) { + throw new RuntimeException('Postfach kaputt'); + } + + return $this->inner->{$name}(...$arguments); + } + }); + + $instance = Instance::factory()->create(['status' => 'active', 'vmid' => 101]); + + // erster Versuch: Versand scheitert + app(BlockAddress::class)->forInstance($instance, '203.0.113.7', 12); + // zweiter Versuch kurz darauf: Versand geht durch + app(BlockAddress::class)->forInstance($instance, '203.0.113.8', 12); + + // Die zweite Sperre muss eine Mail erzeugen, obwohl die erste eine + // Stunde 'verbraucht' zu haben schien. + Mail::assertQueued(SecurityBlockMail::class); +}); + it('geht standardmaessig aus dem System-Postfach und folgt der Wegwahl', function () { Mailbox::factory()->create(['key' => 'no-reply', 'address' => 'no-reply@clupilot.com', 'active' => true]); $info = Mailbox::factory()->create(['key' => 'info', 'active' => true]);