Bound the mail test-send and real send to a timeout instead of hanging
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 <noreply@anthropic.com>feat/granted-plans
parent
9fc74eedc3
commit
254674f456
|
|
@ -14,6 +14,7 @@ use Symfony\Component\Mailer\Envelope;
|
||||||
use Symfony\Component\Mailer\SentMessage;
|
use Symfony\Component\Mailer\SentMessage;
|
||||||
use Symfony\Component\Mailer\Transport\NullTransport;
|
use Symfony\Component\Mailer\Transport\NullTransport;
|
||||||
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
|
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
|
||||||
|
use Symfony\Component\Mailer\Transport\Smtp\Stream\SocketStream;
|
||||||
use Symfony\Component\Mailer\Transport\TransportInterface;
|
use Symfony\Component\Mailer\Transport\TransportInterface;
|
||||||
use Symfony\Component\Mime\RawMessage;
|
use Symfony\Component\Mime\RawMessage;
|
||||||
|
|
||||||
|
|
@ -181,6 +182,19 @@ class MailboxTransport implements TransportInterface
|
||||||
|
|
||||||
$transport = $policy->apply(new EsmtpTransport($host, $port, $policy->implicit));
|
$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
|
// Codex R15#4, P1b: only called when this mailbox actually
|
||||||
// authenticates. smtpUsername()'s address fallback means it is
|
// authenticates. smtpUsername()'s address fallback means it is
|
||||||
// NEVER empty, so calling setUsername() unconditionally would
|
// NEVER empty, so calling setUsername() unconditionally would
|
||||||
|
|
|
||||||
|
|
@ -127,6 +127,21 @@ final class MailboxTester
|
||||||
// silently prove a different policy than real sends enforce.
|
// silently prove a different policy than real sends enforce.
|
||||||
'auto_tls' => $policy->autoTls,
|
'auto_tls' => $policy->autoTls,
|
||||||
'require_tls' => $policy->requireTls,
|
'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,
|
...$credentials,
|
||||||
])->raw(
|
])->raw(
|
||||||
__('mail_settings.test_body', ['key' => $box->key]),
|
__('mail_settings.test_body', ['key' => $box->key]),
|
||||||
|
|
|
||||||
|
|
@ -52,6 +52,7 @@ return [
|
||||||
'purpose_unknown_mailbox' => 'Dieses Postfach existiert nicht mehr. Bitte eines aus der Liste wählen.',
|
'purpose_unknown_mailbox' => 'Dieses Postfach existiert nicht mehr. Bitte eines aus der Liste wählen.',
|
||||||
|
|
||||||
'test' => 'Testmail senden',
|
'test' => 'Testmail senden',
|
||||||
|
'testing' => 'Wird gesendet…',
|
||||||
'test_recipient' => 'Testempfänger',
|
'test_recipient' => 'Testempfänger',
|
||||||
'test_hint' => 'Der Testversand geht wirklich raus — auch auf dieser Installation, auf der gewöhnliche Mails nur protokolliert werden.',
|
'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.',
|
'test_recipient_required' => 'Bitte eine Empfängeradresse eingeben.',
|
||||||
|
|
|
||||||
|
|
@ -52,6 +52,7 @@ return [
|
||||||
'purpose_unknown_mailbox' => 'That mailbox no longer exists. Please choose one from the list.',
|
'purpose_unknown_mailbox' => 'That mailbox no longer exists. Please choose one from the list.',
|
||||||
|
|
||||||
'test' => 'Send test mail',
|
'test' => 'Send test mail',
|
||||||
|
'testing' => 'Sending…',
|
||||||
'test_recipient' => 'Test recipient',
|
'test_recipient' => 'Test recipient',
|
||||||
'test_hint' => 'The test send really goes out — even on this installation, where ordinary mail is only logged.',
|
'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.',
|
'test_recipient_required' => 'Please enter a recipient address.',
|
||||||
|
|
|
||||||
|
|
@ -92,10 +92,18 @@
|
||||||
{{ $box->last_verified_at?->local()->isoFormat('DD.MM.YYYY HH:mm') ?? __('mail_settings.never_verified') }}
|
{{ $box->last_verified_at?->local()->isoFormat('DD.MM.YYYY HH:mm') ?? __('mail_settings.never_verified') }}
|
||||||
</td>
|
</td>
|
||||||
<td class="py-3 text-right space-x-2">
|
<td class="py-3 text-right space-x-2">
|
||||||
{{-- 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. --}}
|
||||||
<x-ui.button size="sm" variant="ghost" wire:click="test('{{ $box->uuid }}')"
|
<x-ui.button size="sm" variant="ghost" wire:click="test('{{ $box->uuid }}')"
|
||||||
wire:loading.attr="disabled" wire:target="test('{{ $box->uuid }}')">
|
wire:loading.attr="disabled" wire:target="test('{{ $box->uuid }}')">
|
||||||
<x-ui.icon name="send" class="size-4" />{{ __('mail_settings.test') }}
|
<x-ui.icon name="send" class="size-4" />
|
||||||
|
<span wire:loading.remove wire:target="test('{{ $box->uuid }}')">{{ __('mail_settings.test') }}</span>
|
||||||
|
<span wire:loading wire:target="test('{{ $box->uuid }}')">{{ __('mail_settings.testing') }}</span>
|
||||||
</x-ui.button>
|
</x-ui.button>
|
||||||
<x-ui.button size="sm" variant="ghost"
|
<x-ui.button size="sm" variant="ghost"
|
||||||
x-on:click="$dispatch('openModal', { component: 'edit-mailbox', arguments: { uuid: '{{ $box->uuid }}' } })">
|
x-on:click="$dispatch('openModal', { component: 'edit-mailbox', arguments: { uuid: '{{ $box->uuid }}' } })">
|
||||||
|
|
|
||||||
|
|
@ -390,3 +390,45 @@ it('refuses to send when the mail server port is not a positive number, rather t
|
||||||
expect($result['ok'])->toBeFalse()
|
expect($result['ok'])->toBeFalse()
|
||||||
->and($result['error'])->toBe(__('mail_settings.test_port_not_configured'));
|
->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);
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ use Illuminate\Support\Facades\Mail;
|
||||||
use Symfony\Component\Mailer\Exception\TransportException;
|
use Symfony\Component\Mailer\Exception\TransportException;
|
||||||
use Symfony\Component\Mailer\Transport\NullTransport;
|
use Symfony\Component\Mailer\Transport\NullTransport;
|
||||||
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
|
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
|
||||||
|
use Symfony\Component\Mailer\Transport\Smtp\Stream\SocketStream;
|
||||||
use Symfony\Component\Mailer\Transport\TransportInterface;
|
use Symfony\Component\Mailer\Transport\TransportInterface;
|
||||||
use Tests\Support\FakeSmtpServer;
|
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);
|
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);
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue