From 254674f456398918c91179403db38b44a451bcc3 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 16:36:14 +0200 Subject: [PATCH] Bound the mail test-send and real send to a timeout instead of hanging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A blocked outbound port (587 filtered at a firewall, as on the live server) left both MailboxTester and MailboxTransport blocking until PHP's default_socket_timeout (60s) or the reverse proxy cut the request — a 504 with no error the operator ever saw. Mail::build() honours a 'timeout' config key by calling SocketStream::setTimeout() (confirmed by reading MailManager::configureSmtpTransport() and by an end-to-end run against a genuinely unroutable host), so MailboxTester now passes 'timeout' => 10 through it. MailboxTransport builds its EsmtpTransport directly rather than through Mail::build(), so it sets the same call on the transport's stream, at 30s — a queue worker can afford to wait longer than an operator watching a browser, and a timed-out send is retried rather than lost. The test button also now shows a visible "sending" state while the request is in flight, matching the wire:loading span pattern already used on admin/host-create's save button — previously only wire:loading.attr="disabled" fired, leaving a greyed-out button with no other sign anything was happening. Co-Authored-By: Claude Opus 5 --- app/Mail/Transport/MailboxTransport.php | 14 +++++++ app/Services/Mail/MailboxTester.php | 15 +++++++ lang/de/mail_settings.php | 1 + lang/en/mail_settings.php | 1 + resources/views/livewire/admin/mail.blade.php | 12 +++++- tests/Feature/Mail/MailboxTesterTest.php | 42 +++++++++++++++++++ tests/Feature/Mail/MailboxTransportTest.php | 32 ++++++++++++++ 7 files changed, 115 insertions(+), 2 deletions(-) diff --git a/app/Mail/Transport/MailboxTransport.php b/app/Mail/Transport/MailboxTransport.php index 9885bd1..ba28ea9 100644 --- a/app/Mail/Transport/MailboxTransport.php +++ b/app/Mail/Transport/MailboxTransport.php @@ -14,6 +14,7 @@ use Symfony\Component\Mailer\Envelope; use Symfony\Component\Mailer\SentMessage; use Symfony\Component\Mailer\Transport\NullTransport; use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; +use Symfony\Component\Mailer\Transport\Smtp\Stream\SocketStream; use Symfony\Component\Mailer\Transport\TransportInterface; use Symfony\Component\Mime\RawMessage; @@ -181,6 +182,19 @@ class MailboxTransport implements TransportInterface $transport = $policy->apply(new EsmtpTransport($host, $port, $policy->implicit)); + // Neither this transport nor MailboxTester bounded its socket + // before this fix: a filtered outbound port (587 dropped by a + // firewall, unlike a closed one that refuses instantly) hung + // until PHP's default_socket_timeout (60s ini default) — for a + // queue worker, that is one send blocking every other job behind + // it. 30s, not MailboxTester's 10s: a worker can afford to wait + // longer than an operator watching a browser, and a send that + // times out is retried by the queue, not lost. + $stream = $transport->getStream(); + if ($stream instanceof SocketStream) { + $stream->setTimeout(30); + } + // Codex R15#4, P1b: only called when this mailbox actually // authenticates. smtpUsername()'s address fallback means it is // NEVER empty, so calling setUsername() unconditionally would diff --git a/app/Services/Mail/MailboxTester.php b/app/Services/Mail/MailboxTester.php index 022b0c1..9281bed 100644 --- a/app/Services/Mail/MailboxTester.php +++ b/app/Services/Mail/MailboxTester.php @@ -127,6 +127,21 @@ final class MailboxTester // silently prove a different policy than real sends enforce. 'auto_tls' => $policy->autoTls, 'require_tls' => $policy->requireTls, + // Confirmed by reading MailManager::configureSmtpTransport() + // (vendor/laravel/framework/src/Illuminate/Mail/MailManager.php:221) + // AND by hand, end to end through Mail::build(): it reads + // this key and calls SocketStream::setTimeout() whenever the + // stream is a SocketStream — true for both 'smtp' and + // 'smtps', the only schemes this file ever passes. Without + // it, a FILTERED outbound port (587 dropped by a firewall, + // unlike a closed one that refuses instantly) blocked until + // PHP's default_socket_timeout (60s ini default) or the + // reverse proxy cut the request first — a 504 with no error + // ever reaching the operator, which is the bug this exists + // to fix. 10s, not MailboxTransport's 30s: an operator is + // watching a browser, not a queue worker that can afford to + // wait and retry. + 'timeout' => 10, ...$credentials, ])->raw( __('mail_settings.test_body', ['key' => $box->key]), diff --git a/lang/de/mail_settings.php b/lang/de/mail_settings.php index 9089372..4629148 100644 --- a/lang/de/mail_settings.php +++ b/lang/de/mail_settings.php @@ -52,6 +52,7 @@ return [ 'purpose_unknown_mailbox' => 'Dieses Postfach existiert nicht mehr. Bitte eines aus der Liste wählen.', 'test' => 'Testmail senden', + 'testing' => 'Wird gesendet…', 'test_recipient' => 'Testempfänger', 'test_hint' => 'Der Testversand geht wirklich raus — auch auf dieser Installation, auf der gewöhnliche Mails nur protokolliert werden.', 'test_recipient_required' => 'Bitte eine Empfängeradresse eingeben.', diff --git a/lang/en/mail_settings.php b/lang/en/mail_settings.php index b7edcfd..fb653b7 100644 --- a/lang/en/mail_settings.php +++ b/lang/en/mail_settings.php @@ -52,6 +52,7 @@ return [ 'purpose_unknown_mailbox' => 'That mailbox no longer exists. Please choose one from the list.', 'test' => 'Send test mail', + 'testing' => 'Sending…', 'test_recipient' => 'Test recipient', 'test_hint' => 'The test send really goes out — even on this installation, where ordinary mail is only logged.', 'test_recipient_required' => 'Please enter a recipient address.', diff --git a/resources/views/livewire/admin/mail.blade.php b/resources/views/livewire/admin/mail.blade.php index f67ad4e..3b00c8e 100644 --- a/resources/views/livewire/admin/mail.blade.php +++ b/resources/views/livewire/admin/mail.blade.php @@ -92,10 +92,18 @@ {{ $box->last_verified_at?->local()->isoFormat('DD.MM.YYYY HH:mm') ?? __('mail_settings.never_verified') }} - {{-- R18: icon beside its text, single line, size-4. --}} + {{-- R18: icon beside its text, single line, size-4. + Two spans swapped by wire:loading, same + pattern as admin/host-create.blade.php's save + button — the disabled attribute alone left an + operator staring at a greyed-out button with + no sign anything was happening for up to the + full connect timeout. --}} - {{ __('mail_settings.test') }} + + {{ __('mail_settings.test') }} + {{ __('mail_settings.testing') }} diff --git a/tests/Feature/Mail/MailboxTesterTest.php b/tests/Feature/Mail/MailboxTesterTest.php index 5b1a732..06541d5 100644 --- a/tests/Feature/Mail/MailboxTesterTest.php +++ b/tests/Feature/Mail/MailboxTesterTest.php @@ -390,3 +390,45 @@ it('refuses to send when the mail server port is not a positive number, rather t expect($result['ok'])->toBeFalse() ->and($result['error'])->toBe(__('mail_settings.test_port_not_configured')); }); + +/* +| LOAD-BEARING for the timeout fix itself, and the reason this test cannot +| reuse 127.0.0.1:1 the way every other test in this file does: a closed +| port refuses in milliseconds via TCP RST, so it can prove a FAST failure +| but never that a timeout actually FIRES. 10.255.255.1 is a private address +| this container has no route to — confirmed by hand, three separate runs, +| not assumed: it blocks for exactly the configured timeout and fails with +| ETIMEDOUT, never an instant ECONNREFUSED/ENETUNREACH. That is what the +| production report describes from the client's side — port 587 filtered +| outbound at a firewall, not a server actively refusing the connection — +| and it is the only way to prove the 10s bound is real rather than merely +| configured and ignored. Bounds are wide enough to absorb container/CI +| jitter (measured by hand, twice, at ~10.02s) while still landing nowhere +| near the OLD behaviour: PHP's default_socket_timeout (60s) or whatever a +| reverse proxy gives up at first — which is the actual 504 this fix exists +| to prevent. +*/ +it('bounds the test send to about 10 seconds against a host that never answers, instead of hanging toward the 60s default', function () { + Settings::set('mail.host', '10.255.255.1'); + Settings::set('mail.port', 587); + + $box = Mailbox::factory()->create(['address' => 'no-reply@clupilot.com']); + + $start = microtime(true); + $result = app(MailboxTester::class)->run($box, 'ziel@example.com'); + $elapsed = microtime(true) - $start; + + expect($result['ok'])->toBeFalse() + // Symfony's own wording ("... Connection timed out"), passed through + // verbatim — the same "operating system's own sentence IS the + // diagnosis" convention every other test in this file relies on. An + // operator reading this can tell it apart from a rejected password + // (which comes back as an SMTP response code and text, not this). + ->and($result['error'])->toContain('timed out') + // Actually exercised the timeout — not an instant failure that + // happens to also contain the word "timed out" by coincidence. + ->and($elapsed)->toBeGreaterThanOrEqual(9.5) + // The whole point: bounded near the configured 10s, nowhere close + // to the 60s PHP default this bug used to hang against. + ->and($elapsed)->toBeLessThan(15.0); +}); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index 6072af2..51c95eb 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/tests/Feature/Mail/MailboxTransportTest.php @@ -10,6 +10,7 @@ use Illuminate\Support\Facades\Mail; use Symfony\Component\Mailer\Exception\TransportException; use Symfony\Component\Mailer\Transport\NullTransport; use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; +use Symfony\Component\Mailer\Transport\Smtp\Stream\SocketStream; use Symfony\Component\Mailer\Transport\TransportInterface; use Tests\Support\FakeSmtpServer; @@ -438,3 +439,34 @@ it('refuses to send when the mail server port is not a positive number', functio expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class); }); + +/* +| A property check, not a real hang, on purpose: MailboxTesterTest carries +| the real end-to-end timed proof (a genuinely unroutable host, elapsed time +| asserted) for the 10s user-facing bound — this file's own convention for +| the identical situation is "property assertions... prove the transport is +| CONFIGURED correctly" (see the TLS policy tests above), reserving an actual +| timed connection for where mirroring the wire behaviour, not just the +| config, earns its cost. A real 30s hang here would buy nothing beyond that: +| SocketStream::setTimeout() is a plain property setter with no branching to +| miss, and 30 extra real seconds on every suite run is a poor trade for +| proving what MailManager's own use of the identical call already lets +| MailboxTesterTest verify behaviourally. +*/ +it('bounds the delegate to a 30 second socket timeout, so a queue worker cannot hang toward the 60s default', function () { + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + Settings::set('mail.host', '10.255.255.1'); // never answers — see MailboxTesterTest; irrelevant here, nothing connects + Settings::set('mail.port', 587); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $delegate = mailboxDelegate($transport); + + expect($delegate)->toBeInstanceOf(EsmtpTransport::class) + ->and($delegate->getStream())->toBeInstanceOf(SocketStream::class) + // 30.0, not the 60.0 that ini's default_socket_timeout would leave + // getTimeout() reporting if delegate() had never called setTimeout() + // at all — the exact way this fix could silently do nothing. + ->and($delegate->getStream()->getTimeout())->toBe(30.0); +});