diff --git a/app/Jobs/PacedMail.php b/app/Jobs/PacedMail.php index 1264f69..eaa459c 100644 --- a/app/Jobs/PacedMail.php +++ b/app/Jobs/PacedMail.php @@ -4,68 +4,117 @@ namespace App\Jobs; use App\Services\Mail\MailLane; use App\Support\Settings; -use DateTimeInterface; use Illuminate\Mail\SendQueuedMailable; use Illuminate\Queue\Middleware\RateLimited; /** * Der Auftrag, der eine Mail im Takt ihrer Spur verschickt. * - * `SendQueuedMailable` kennt keine `middleware()` — nachgesehen im Framework: - * die Klasse hat handle, backoff, retryUntil, failed, displayName, __clone und - * sonst nichts. Eine `middleware()` auf der MAILKLASSE liest deshalb niemand. - * Der Weg führt über diesen eigenen Auftrag, den `Mailable::newQueuedJob()` - * über den Container erzeugt — eine Bindung tauscht ihn für alle Mails aus, - * ohne dass eine einzige Absendestelle sich ändert. + * `SendQueuedMailable` selbst hat keine `middleware()` — nachgezählt im + * Framework: handle, backoff, retryUntil, failed, displayName, __clone. Der + * eigene Auftrag ist trotzdem nicht der einzige Weg: eine `middleware()` auf + * der MAILKLASSE wird sehr wohl gelesen, weil `Mailable::newQueuedJob()` sie + * einsammelt und per `->through()` an den Auftrag weiterreicht. + * + * Der Takt sitzt hier statt im Trait, weil er dann auch für Mailklassen greift, + * die den Trait NICHT benutzen (Pakete, Fortify) — und weil er sich mit einer + * Mailable-eigenen `middleware()` verträgt, statt sie zu verdrängen: die + * Bindung im Container tauscht den Auftrag für jede Mail aus, ohne dass eine + * einzige Absendestelle sich ändert. * * Zur Falle mit den Versuchen: der Arbeiter läuft mit --tries=3, und eine * gedrosselte Rückstellung zählt als Versuch. Deshalb `retryUntil()` statt - * eines Versuchszählers — eine zeitliche Grenze kennt keine Rückstellungen, - * sondern nur ein Ende. + * eines Versuchszählers — aber nur dort, wo wirklich gedrosselt wird. */ class PacedMail extends SendQueuedMailable { /** @return array */ public function middleware(): array + { + $spur = $this->gedrosselteSpur(); + + return $spur === null ? [] : [new RateLimited($spur)]; + } + + /** + * Sechs Stunden statt eines Versuchszählers — auf den gedrosselten Spuren. + * + * Der Arbeiter prüft `retryUntil` VOR `--tries`: liegt sie in der Zukunft, + * steigt er aus, bevor er `attempts()` gegen die Versuchszahl hält. Genau + * das wird hier gebraucht, denn ein Zähler könnte nicht zwischen „dreimal + * zurückgelegt" und „dreimal gescheitert" unterscheiden — die Rechnung, die + * am Abend an der Drossel ansteht, wäre nach dem dritten Mal endgültig + * gescheitert statt verschickt. + * + * Und genau deshalb hängt die Grenze an derselben Bedingung wie die + * Drossel. Ein Zeitfenster hebelt `--tries` nicht bloß für Rückstellungen + * aus, sondern GANZ: `Worker::markJobAsFailedIfWillExceedMaxAttempts` + * prüft die Versuchszahl ausdrücklich nur `if (! $job->retryUntil())`. Wo + * nie gedrosselt wird — Direktspur, Notschalter aus —, gibt es keine + * Rückstellungen abzufangen, wohl aber eine Mail, die aus echtem Grund + * wirft: die würde sechs Stunden lang wiederholt statt nach drei Versuchen + * abgelegt. Ohne Pause obendrein, weil der Arbeiter ohne `--backoff` läuft + * und `Worker::calculateBackoff` dann auf null Sekunden fällt. Ihr Fehler + * stünde sechs Stunden lang nicht in `failed_jobs` und wäre für jede + * Alarmierung nicht vorhanden; auf der Direktspur zöge der Arbeiter sie + * obendrein bei jedem Durchlauf zuerst — sechs Stunden lang keine + * Kennwort-Zurücksetzung. + * + * Sechs Stunden ist weit genug, dass auch ein Lauf über die ganze Nacht + * durchkommt, und eng genug, dass eine Mail, die dann noch nicht draußen + * ist, nicht am nächsten Tag zwischen den neuen auftaucht. + * + * Ohne eigene Rückgabeangabe, wie die Elternklasse: die lässt neben einem + * Zeitpunkt auch einen Zeitstempel zu (`Queue::getJobExpiration` behandelt + * beides), und eine engere Angabe hier zerbräche eine Mailklasse, die von + * diesem Spielraum Gebrauch macht. + * + * @return \DateTimeInterface|int|null + */ + public function retryUntil() + { + // Die Mailklasse zuerst: die Elternklasse reicht an + // `$this->mailable->retryUntil` weiter. Heute deklariert das keine der + // vierzehn — aber eine Überschreibung, die nicht zurückfragt, nähme + // der nächsten ihre Entscheidung still weg. + if (($eigene = parent::retryUntil()) !== null) { + return $eigene; + } + + return $this->gedrosselteSpur() === null ? null : now()->addHours(6); + } + + /** + * Die Spur, deren Drossel für diesen Auftrag gilt — oder null. + * + * Eine Stelle für beide Fragen, weil `middleware()` und `retryUntil()` + * sonst auseinanderlaufen könnten. Beide Richtungen tun weh: ein + * Zeitfenster ohne Drossel nimmt die Versuchszahl weg, ohne etwas dafür zu + * geben, und eine Drossel ohne Zeitfenster ist genau die Falle, um die es + * in dieser Aufgabe geht. + */ + private function gedrosselteSpur(): ?string { if (! Settings::bool('mail.pace.enabled', true)) { - return []; + return null; } // Die Schlange am Mailable, nicht `MailLane::for(...)`: das hier ist // der Name, den `Mailable::queue()` an `pushOn()` gegeben hat — die // Spur, in der dieser Auftrag TATSÄCHLICH liegt. `MailLane::for(...)` - // läge zwischen Einreihen und Abarbeiten eine Zuordnung später: würde - // der Betreiber die Rechnung von ruhig auf wichtig schieben, während - // zweihundert davon in der ruhigen Schlange warten, drosselte sie ab - // diesem Moment das falsche Kontingent. + // läse die Einstellungen erst im Arbeiter: würde der Betreiber die + // Rechnung von ruhig auf wichtig schieben, während zweihundert davon + // in der ruhigen Schlange warten, drosselte sie ab diesem Moment das + // falsche Kontingent. // // `?? null` statt eines nackten Zugriffs, weil `$this->mailable` nur // die Schnittstelle zusichert: eine Mailklasse, die den Trait nicht // benutzt, hat die Eigenschaft womöglich gar nicht — und liegt dann // auch in keiner Spur, gehört also nicht gedrosselt. return match ($this->mailable->queue ?? null) { - MailLane::URGENT => [new RateLimited(MailLane::URGENT)], - MailLane::CALM => [new RateLimited(MailLane::CALM)], - default => [], + MailLane::URGENT => MailLane::URGENT, + MailLane::CALM => MailLane::CALM, + default => null, }; } - - /** - * Sechs Stunden statt eines Versuchszählers. - * - * Der Arbeiter prüft `retryUntil` VOR `--tries`: liegt sie in der Zukunft, - * steigt er aus, bevor er `attempts()` gegen die Versuchszahl hält. Ein - * Zähler dagegen könnte nicht zwischen „dreimal zurückgelegt" und „dreimal - * gescheitert" unterscheiden — die Rechnung, die am Abend zweihundertmal an - * der Drossel ansteht, wäre nach dem dritten Mal endgültig gescheitert. - * - * Weit genug, dass auch ein Lauf über die ganze Nacht durchkommt, und eng - * genug, dass eine Mail, die nach sechs Stunden noch nicht draußen ist, - * nicht am nächsten Tag zwischen den neuen auftaucht. - */ - public function retryUntil(): DateTimeInterface - { - return now()->addHours(6); - } } diff --git a/app/Providers/MailPaceServiceProvider.php b/app/Providers/MailPaceServiceProvider.php index a592c31..ad6e466 100644 --- a/app/Providers/MailPaceServiceProvider.php +++ b/app/Providers/MailPaceServiceProvider.php @@ -37,13 +37,34 @@ class MailPaceServiceProvider extends ServiceProvider // läuft im Arbeiter je Auftrag, und eine Änderung des Betreibers wirkt // damit ab dem nächsten Auftrag statt erst ab dem nächsten Neustart. RateLimiter::for(MailLane::URGENT, fn () => Limit::perMinutes( - (int) Settings::get('mail.pace.urgent.minutes', 5), - (int) Settings::get('mail.pace.urgent.count', 30), + $this->mindestensEins('mail.pace.urgent.minutes', 5), + $this->mindestensEins('mail.pace.urgent.count', 30), )); RateLimiter::for(MailLane::CALM, fn () => Limit::perMinutes( - (int) Settings::get('mail.pace.calm.minutes', 10), - (int) Settings::get('mail.pace.calm.count', 20), + $this->mindestensEins('mail.pace.calm.minutes', 10), + $this->mindestensEins('mail.pace.calm.count', 20), )); } + + /** + * Eine Zahl aus den Einstellungen, mindestens eins. + * + * Die Vorgabe in `Settings::get()` greift nur bei FEHLENDER Zeile. Eine + * gespeicherte `null` — oder ein Wort statt einer Zahl — kommt als Wert + * durch und wird von `(int)` zu 0, ohne dass jemand 0 eingetippt hätte. + * Ein Kontingent von 0 hieße: `tooManyAttempts` ist immer wahr, jede Mail + * wird endlos zurückgelegt und nach sechs Stunden gesammelt zu + * `failed_jobs` — genau der Ausgang, den dieser Takt verhindern soll, nur + * über einen anderen Weg. Ein Fenster von 0 Minuten hieße umgekehrt: gar + * keine Drossel. + * + * Die Untergrenze steht hier, weil der Verlust hier passiert. Was ein + * eingetipptes 0 bedeuten SOLL, ist eine Frage an die Eingabe und gehört + * an die Konsolenseite — nicht an diese Lesestelle. + */ + private function mindestensEins(string $key, int $default): int + { + return max(1, (int) Settings::get($key, $default)); + } } diff --git a/tests/Feature/Mail/MailPaceTest.php b/tests/Feature/Mail/MailPaceTest.php index d61a50a..f1b5d7e 100644 --- a/tests/Feature/Mail/MailPaceTest.php +++ b/tests/Feature/Mail/MailPaceTest.php @@ -9,6 +9,7 @@ use App\Support\Settings; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Mail\Mailable; +use Illuminate\Mail\Mailables\Attachment; use Illuminate\Mail\Mailables\Content; use Illuminate\Mail\Mailables\Envelope; use Illuminate\Queue\Events\JobProcessing; @@ -48,6 +49,62 @@ class TaktProbeMail extends Mailable implements ShouldQueue } } +/** + * Eine Mail, die aus echtem Grund kippt — nicht wegen der Drossel. + * + * Der Anhang, weil genau dort der teuerste echte Fehler dieses Projekts sitzt: + * `InvoiceMail::attachments()` rendert das PDF beim Senden. Kippt das, ist der + * Auftrag nicht gedrosselt, sondern kaputt, und gehört nach drei Versuchen ins + * Fehlerprotokoll statt sechs Stunden lang wiederholt. + */ +class GiftigeDirektMail extends Mailable implements ShouldQueue +{ + use Queueable, RidesALane, SerializesModels; + + public function envelope(): Envelope + { + return new Envelope(from: 'takt@example.test', subject: 'Giftig'); + } + + public function content(): Content + { + return new Content(htmlString: '

Giftig

'); + } + + /** @return array */ + public function attachments(): array + { + throw new RuntimeException('Das Rendern des Anhangs ist gekippt.'); + } +} + +/** + * Eine Mailklasse, die ihre eigene zeitliche Grenze mitbringt. + * + * Heute tut das keine der vierzehn. Aber `SendQueuedMailable::retryUntil()` + * reicht an die Mailklasse weiter, und eine Überschreibung, die das still + * wegnimmt, würde die nächste ohne einen Hinweis übergehen. + */ +class EigeneGrenzeMail extends Mailable implements ShouldQueue +{ + use Queueable, RidesALane, SerializesModels; + + public function retryUntil(): DateTimeInterface + { + return now()->addMinutes(11); + } + + public function envelope(): Envelope + { + return new Envelope(from: 'takt@example.test', subject: 'Eigene Grenze'); + } + + public function content(): Content + { + return new Content(htmlString: '

Eigene Grenze

'); + } +} + /** * Der Auftrag, so wie ihn das Einreihen erzeugt. * @@ -80,6 +137,23 @@ function drosselName(RateLimited $middleware): string return (new ReflectionProperty($middleware, 'limiterName'))->getValue($middleware); } +/** Ein Durchlauf des echten Arbeiters, so wie er im Betrieb läuft. */ +function einDurchlaufDesArbeiters(): void +{ + Artisan::call('queue:work', [ + '--once' => true, + '--tries' => 3, + '--sleep' => 0, + '--queue' => implode(',', [MailLane::DIRECT, MailLane::URGENT, MailLane::CALM]), + ]); +} + +/** Die Nutzlast des einen Auftrags, der gerade in der Schlange liegt. */ +function nutzlastDesAuftrags(): array +{ + return json_decode((string) DB::table('jobs')->value('payload'), true); +} + /** * Die Drossel darf keine Mail verlieren. * @@ -134,12 +208,7 @@ it('verliert keine Mail, wenn das Kontingent kleiner ist als der Lauf', function continue; } - Artisan::call('queue:work', [ - '--once' => true, - '--tries' => 3, - '--sleep' => 0, - '--queue' => implode(',', [MailLane::DIRECT, MailLane::URGENT, MailLane::CALM]), - ]); + einDurchlaufDesArbeiters(); } expect(DB::table('failed_jobs')->count())->toBe(0) @@ -147,6 +216,77 @@ it('verliert keine Mail, wenn das Kontingent kleiner ist als der Lauf', function ->and(max($versuche))->toBeGreaterThan(3); }); +/** + * Die Kehrseite: wo nicht gedrosselt wird, muss `--tries` weiter greifen. + * + * Eine zeitliche Grenze schaltet den Versuchszähler nicht bloß für die Drossel + * ab, sondern ganz: `Worker::markJobAsFailedIfWillExceedMaxAttempts` prüft die + * Versuche ausdrücklich nur `if (! $job->retryUntil())`. Eine Mail, die aus + * ECHTEM Grund wirft, würde damit sechs Stunden lang wiederholt — ohne Pause, + * weil der Arbeiter ohne `--backoff` läuft und `calculateBackoff` dann auf null + * Sekunden fällt. + * + * Zwei Schäden: der Fehler ist sechs Stunden lang nicht in `failed_jobs`, also + * für jede Alarmierung nicht vorhanden. Und sobald der Arbeiter die Spuren in + * Prioritätsreihenfolge liest, zieht er diese eine giftige Direkt-Mail bei + * JEDEM Durchlauf zuerst — sechs Stunden lang keine Kennwort-Zurücksetzung. + */ +it('lässt eine Direkt-Mail, die aus echtem Grund wirft, nach drei Versuchen scheitern', function () { + MailLane::assign(GiftigeDirektMail::class, MailLane::DIRECT); + + config(['queue.default' => 'database']); + + Mail::to('kunde@example.test')->queue(new GiftigeDirektMail); + + // Ohne Zeitfenster wird der Auftrag beim dritten Versuch abgelegt und die + // Schlange ist leer. Mit Zeitfenster käme er endlos wieder — die Schranke + // bei zehn ist der Unterschied zwischen einem roten Test und einem Test, + // der nie fertig wird. + $durchlaeufe = 0; + + while (DB::table('jobs')->where('available_at', '<=', now()->getTimestamp())->exists()) { + if (++$durchlaeufe > 10) { + throw new RuntimeException('Der Auftrag hört nicht auf — die Versuchszahl greift nicht.'); + } + + einDurchlaufDesArbeiters(); + } + + expect($durchlaeufe)->toBe(3) + ->and(DB::table('failed_jobs')->count())->toBe(1) + ->and(DB::table('jobs')->count())->toBe(0); +}); + +it('gibt einem Auftrag ohne Drossel gar kein Zeitfenster', function (string $aufbau) { + config(['queue.default' => 'database']); + + $aufbau === 'direkt' + ? MailLane::assign(TaktProbeMail::class, MailLane::DIRECT) + : Settings::set('mail.pace.enabled', false); + + Mail::to('kunde@example.test')->queue(new TaktProbeMail(1)); + + expect(nutzlastDesAuftrags()['retryUntil'])->toBeNull(); +})->with(['direkt', 'notschalter aus']); + +/** + * Bringt die Mailklasse ihre eigene Grenze mit, gilt ihre. + * + * `SendQueuedMailable::retryUntil()` reicht an `$this->mailable->retryUntil` + * weiter. Eine Überschreibung, die das nicht zurückfragt, nähme der nächsten + * Mailklasse ihre Entscheidung still weg. + */ +it('lässt der Mailklasse ihre eigene zeitliche Grenze', function () { + config(['queue.default' => 'database']); + + Mail::to('kunde@example.test')->queue(new EigeneGrenzeMail); + + // Auf einer gedrosselten Spur — sonst bewiese der Test nur, dass ohne + // Drossel nichts passiert, und nicht, dass die eigene Grenze vorgeht. + expect(MailLane::for(EigeneGrenzeMail::class))->toBe(MailLane::CALM) + ->and(nutzlastDesAuftrags()['retryUntil'])->toBe(now()->addMinutes(11)->getTimestamp()); +}); + /** * Was der Arbeiter liest, ist der Auftrag in der Schlange, nicht das Objekt. * @@ -219,6 +359,32 @@ it('nimmt beide Kontingente aus den Einstellungen', function () { ->and($ruhig->decaySeconds)->toBe(540); }); +/** + * Eine unbrauchbare Einstellung darf kein Kontingent von null ergeben. + * + * Die Vorgabe in `Settings::get()` greift nur bei FEHLENDER Zeile. Eine + * gespeicherte `null` — oder ein Wort statt einer Zahl — kommt als Wert durch + * und wird von `(int)` zu 0. Ein Kontingent von 0 hieße: `tooManyAttempts` ist + * immer wahr, jede Mail wird endlos zurückgelegt und nach sechs Stunden + * gesammelt zu `failed_jobs` — genau der Ausgang, den dieser Takt verhindern + * soll, nur über einen anderen Weg. Ein Fenster von 0 Minuten hieße + * umgekehrt: gar keine Drossel. + */ +it('macht aus einer unbrauchbaren Einstellung kein Kontingent von null', function () { + Settings::set('mail.pace.urgent.count', null); + Settings::set('mail.pace.urgent.minutes', null); + Settings::set('mail.pace.calm.count', 'viele'); + Settings::set('mail.pace.calm.minutes', -3); + + $wichtig = RateLimiter::limiter(MailLane::URGENT)(null); + $ruhig = RateLimiter::limiter(MailLane::CALM)(null); + + expect($wichtig->maxAttempts)->toBe(1) + ->and($wichtig->decaySeconds)->toBe(60) + ->and($ruhig->maxAttempts)->toBe(1) + ->and($ruhig->decaySeconds)->toBe(60); +}); + it('taktet ohne Einstellung dreißig je fünf und zwanzig je zehn Minuten', function () { $wichtig = RateLimiter::limiter(MailLane::URGENT)(null); $ruhig = RateLimiter::limiter(MailLane::CALM)(null);