Failsafe on ActiveRecord::ConnectionFailed

[navidemad]

Fixes #307

When the database server terminates a connection mid-query, the adapter raises ActiveRecord::ConnectionFailed. Postgres does this on idle_in_transaction_session_timeout (which is how we hit it: Entry.lock_and_write holds a row lock while the block runs, and under load the session idles past the timeout), MySQL on ER_CONNECTION_KILLED / CR_SERVER_GONE_ERROR / CR_SERVER_LOST, Trilogy on the equivalent.

ConnectionFailed is a subclass of QueryAborted / StatementInvalid, not of ConnectionNotEstablished, so TRANSIENT_ACTIVE_RECORD_ERRORS did not match it and the error propagated to the caller instead of degrading to a cache miss.

This adds it to the list. Active Record already treats it as a retryable connection error next to ConnectionNotEstablished (AbstractAdapter#retryable_connection_error?), which is in the list. It is also the only QueryAborted subclass that was missing. I kept the list explicit rather than collapsing to QueryAborted, so future Active Record error classes have to be opted in on purpose.

Tests: emulating_timeouts in test/test_helper.rb now takes the error class to raise (default unchanged). A new SolidCacheConnectionFailedFailsafeTest in test/unit/solid_cache_test.rb runs the whole FailureSafetyBehavior against ConnectionFailed, so every cache operation is checked to degrade to a miss. New test/unit/failsafe_test.rb covers the error handler contract: ConnectionFailed reaches the error_handler, and is re-raised when the handler raises. It also checks that StatementInvalid and NoDatabaseError still propagate, since the point of the list (#182) is not to hide configuration errors.

Overlaps with #308 by @ajaynomics, which has the same one-line change and the FailureSafetyBehavior approach (taken from there). This one adds the error handler and non-transient error tests. Happy for either to land.

Verified locally on SQLite with Rails 8.1.2.1: full suite green, and the two ConnectionFailed tests fail without the one-line change to failsafe.rb.

[a-abdellatif98]

Reviewed both this and #308. Ran each locally on Ruby 3.4.8 against sqlite: #308 is 400 runs / 0 failures, this one 405 runs / 0 failures. The production diff is byte identical in both, so the only thing to decide is the tests.

I think the best outcome is a merge of the two, and I say that as the person who suggested the signature this PR uses.

#308's helper naming is better than mine. I proposed emulating_timeouts(error) without thinking about how the call site would read. emulating_timeouts(ActiveRecord::ConnectionFailed) states something false: a server terminating the connection is not a timeout. #308 renames the primitive to emulating_errors(error_class) and keeps emulating_timeouts as a zero argument wrapper, so no existing call site changes and every call site reads correctly. That is the version I would take.

The most valuable test here is not the ConnectionFailed one. It is test_non_transient_errors_are_not_swallowed. The standing risk with TRANSIENT_ACTIVE_RECORD_ERRORS is that it keeps growing until a real bug becomes a silent cache miss, and that test is what makes the boundary explicit. Worth confirming the assertions bite: NoDatabaseError and StatementInvalid are both ancestors or siblings of the listed classes (ConnectionFailed < QueryAborted < StatementInvalid, NoDatabaseError < StatementInvalid), so they are genuinely the nearest neighbours that must keep propagating, not arbitrary picks. #308 has no equivalent.

So concretely: take #308 as the base, then rebase this one down to test/unit/failsafe_test.rb with the three tests switched to emulating_errors. End state is #308's naming plus this PR's coverage, and #308 keeps priority since it landed first in June.

Two small things if this one is what gets merged instead:

SolidCacheFailsafeErrorsTest#setup assigns @cache but nothing reads it, since each test builds its own store through lookup_store(namespace: @namespace). Can go.

test_connection_failed_is_passed_to_the_error_handler asserting :read_entry is correct, that is the symbol passed at entries.rb:39, but it couples the test to the internal failsafe label. Asserting only returning and the exception class would survive a rename of that symbol. Minor, take it or leave it.

Nothing blocking in either. On the timeline correction, thanks, you are right, and retryable_connection_error? is the better precedent than the hierarchy argument I made.