Failsafe on ActiveRecord::ConnectionFailed

[navidemad]

Fixes #307

Stacked on #308 by @ajaynomics: that commit is included here unchanged and should merge first. Once it lands, the diff of this PR is only test/unit/failsafe_test.rb.

#308 adds ActiveRecord::ConnectionFailed to TRANSIENT_ACTIVE_RECORD_ERRORS and runs the whole FailureSafetyBehavior against it. This PR adds the error handler side of the contract in SolidCacheFailsafeErrorsTest:

• ConnectionFailed reaches the error_handler, with returning: nil and the exception instance.
• ConnectionFailed is re-raised when the handler raises.
• StatementInvalid and NoDatabaseError still propagate. ConnectionFailed < QueryAborted < StatementInvalid and NoDatabaseError < StatementInvalid, so these are the nearest neighbours of the listed classes; the point of the list (Using Solid Cache for low-level caching produces warning -- already initialized constant #182) is not to hide configuration errors or bugs behind a cache miss, and this test keeps that boundary explicit.

The tests use emulating_errors from #308.

Verified locally on SQLite: full suite green, and test_connection_failed_is_passed_to_the_error_handler errors 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.

[navidemad]

Thanks for the careful review, agreed on all points.

I rebased this on top of #308: Ajay's commit is included unchanged, so #308 should merge first and keeps priority. This PR is now down to test/unit/failsafe_test.rb. The three tests use emulating_errors, the unused @cache is gone, and the handler test only asserts returning and the exception class, not the internal :read_entry label. Once #308 lands, the diff here is that one file.