Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Refine error messages for detecting lost connections (Debian bookworm compatibility) #53794

Merged
merged 1 commit into from
Dec 8, 2024

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Dec 8, 2024

Summary

Last year I added those two via #47398 Please see details and reasoning there, it still applies; TL;DR:

Ever since I moved from AWS Aurora to using AWS RDS Proxy, which
(similar to pgbouncer) is essential to not overload the primary database
with connection, I sporadically see these messages. This affects
scheduled tasks, worker, HTTP requests; it's across the whole board.

What I didn't knew back then was that the order/format of those messages very much dependent on the underlying libraries (libpq and or libssl presumably) and once we started to upgrade from Debian bullseye to bookworm, they changed and now are randomly interrupting our service again.

The new (complete) messages look like this:

SQLSTATE[08006] [7] connection to server at "…" (…), port 5… failed: SSL error: sslv3 alert unexpected message (Connection: main, SQL: …

That is, it still contains "SSL error: sslv3 alert unexpected message" but there's now connection specific information after the SQLSTATE prefix.

Since lost connection detection is based on exact string matching and not regex, we're just removing the magic numbers in front and still keep backwards compatibility.

Notes

Please accept this this PR against Laravel 10.x because we're still using and it requires the fix there (we're in the progress moving to L11, but it still takes time).

Thanks

Last year I added those two via laravel#47398
Please see details and reasoning there, it still applies; TL;DR:
> Ever since I moved from AWS Aurora to using AWS RDS Proxy, which
> (similar to pgbouncer) is essential to not overload the primary database
> with connection, I sporadically see these messages. This affects
> scheduled tasks, worker, HTTP requests; it's across the whole board.

What I didn't knew back then was that the order/format of those messages
very much dependent on the underlying libraries (libpq and or libssl
presumably) and once we started to upgrade from Debian bullseye to
bookworm, they changed and now are randomly interrupting our service
again.

The new (complete) messages look like this:
> `SQLSTATE[08006] [7] connection to server at "…" (…), port 5… failed: SSL error: sslv3 alert unexpected message (Connection: main, SQL: …`

That is, it still contains "SSL error: sslv3 alert unexpected message"
but there's now connection specific information before the SQLSTATE
prefix.

Since lost connection detection is based on exact string matching and
not regex, we're just removing the magic numbers in front and still
keep backwards compatibility.

I opened this PR against Laravel 10.x because we're still using and it
requires the fix there (we're in the progress moving to L11, but it still
takes time).

Thanks
@mfn mfn changed the title [10.x] Refine error messages for detecting lost connections [10.x] Refine error messages for detecting lost connections (Debian bookworm compatibility) Dec 8, 2024
@taylorotwell taylorotwell merged commit bd0e7cc into laravel:10.x Dec 8, 2024
26 checks passed
@mfn mfn deleted the mfn-rds-proxy-caused-by-v2 branch December 8, 2024 16:36
@mfn
Copy link
Contributor Author

mfn commented Dec 16, 2024

@taylorotwell et al: can we please have a new Laravel 10 release 🙏🏼

taylorotwell pushed a commit that referenced this pull request Jan 9, 2025
…ookworm compatibility) (#54111)

Same as #53794 , which was for
L10, but this time for L11
taylorotwell pushed a commit to illuminate/database that referenced this pull request Jan 9, 2025
…ookworm compatibility) (#54111)

Same as laravel/framework#53794 , which was for
L10, but this time for L11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants