Repository navigation
fix: bound subscriber Redis commands so a silent primary can't stall delivery - #118
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
If the Redis primary stops answering without closing its connections, the subscriber stops delivering for good. Common causes are a frozen VM, a network partition, or a host that loses power before Sentinel fails over. The subscriber's fetch, claim and ack commands have no time limit of their own, so they wait on the dead socket until the kernel gives up on TCP.
That limit comes from redis-py's
socket_timeout, which the caller controls. redis-py 8.0 defaults it to 5s. Versions 5.0.1 through 7.4.0 default it toNone, and this package allowsredis>=5.0.1. A hung fetch stops the poll loop. A hung ack or claim keeps itsmax_concurrentslot forever, so once enough of them hang, the loop stops fetching too.The Sentinel drill from #117 reproduces this. With
socket_timeout=Noneand the primary paused, Sentinel promotes the replica and publishes recover, but none of the 100 timers due after the failure is delivered within 90s.Fix
command_timeout, available on@broker.subscriberandTimersRoute. It defaults toNone, which meanslease_ttl. Values must be finite, greater than 0, and at mostlease_ttl. Past the lease, another worker may claim the timer anyway, so a larger value gains nothing.due), the claim and the ack/reject removal each run underanyio.fail_after(command_timeout). When a command is cancelled mid-read, redis-py disconnects that connection (checked back to 5.0.1), so the next command opens a fresh one, and with a Sentinel pool that finds the new primary. The existing error handling takes it from there: the fetch loop logs and backs off, and a timed-out ack or claim is logged and frees its slot. A timer whose ack timed out is redelivered after its lease, which the at-least-once contract already allows.lease_ttl=600for slow handlers would otherwise wait 10 minutes per hung command. The risk on the other side: if the value is set too low, slow but healthy commands time out and reconnect.publish()and the inspection methods still use the caller's socket timeout. They run in the caller's own code, where the caller can wrap them in a timeout.Tests
tests/test_command_timeout.pyruns the client through a small TCP proxy that freezes every open connection but lets new ones through, which is what a silent primary followed by failover looks like. Both tests failed before the fix:test_a_fetch_hung_on_a_silent_connection_is_abandonedtest_an_ack_hung_on_a_silent_connection_frees_its_handler_slot(max_concurrent=1)TimersRoute. The defaults invariant intest_tuning_defaults.pynow includescommand_timeout.just test-drfrom test: Sentinel failover drills (just test-dr) #117 withsocket_timeout=None: thepausedrill went from no delivery after the failure to recovery in 10.8s, andkillstill passes (11.0s).