fix: bound the queue read so a dead Redis socket cannot park a worker forever - #16
Merged
Merged
Conversation
… forever
On 2026-08-15 the TCP connections from the GPU fleet to the Redis box were
silently dropped at ~04:05 UTC. Seven server_types stopped consuming within
fifteen minutes of each other and stayed dead for hours; flux_2_dev alone lost
2h45m of throughput. Nothing restarted, nothing alerted, and the endpoints
still reported healthy.
The worker sat in `blpop("ml_tasks")` with no timeout. That is a pure
read-wait: the client transmits nothing, so the kernel never retransmits,
never hits tcp_retries2, and never raises. The thread parked forever on a
socket the server had already discarded — which is why Redis reported
blocked_clients:0 while every worker believed it was blocked. Threads that
were *sending* commands did surface the failure, as ETIMEDOUT roughly
eighteen minutes later, and reconnected normally. The reader never could.
Three changes:
- `blpop` now takes BLPOP_TIMEOUT and the loop treats None as "keep waiting".
Completing the read is what lets a dead socket be noticed at all. The
timeout is kept below SOCKET_TIMEOUT because redis-py applies the socket
read deadline to blocking commands too.
- The connection pool sets socket_keepalive (with per-platform TCP knobs),
socket_timeout, socket_connect_timeout, health_check_interval and
retry_on_timeout, so a half-open connection is reaped instead of trusted.
- Background loops run their body inside `_guarded_iteration`. A bare
`while True:` in a thread was one unhandled exception away from being gone
permanently, and the pruning thread died exactly that way on every replica
of several endpoints while the process carried on looking fine.
Tests cover all three and were mutation-tested: restoring the unbounded
blpop, stripping the pool options, and making the guard re-raise each turn
the corresponding test red.
Recovery from a dead connection costs socket_timeout to detect plus RETRY_DELAY to wait, so this constant is the tail of every stall. 30s made that tail longer than the detection it followed. Jitter matters as much as the value. Connection loss here is a shared event, not an independent one: on 2026-08-15 every worker on a node logged its failure in the same second, because one upstream path change broke all their connections at once. A fixed delay marches that entire herd back into Redis in lockstep, which is the worst possible moment to arrive together. The delay is now spread over [0.5x, 1.5x] of nominal, and RETRY_JITTER = 0 restores deterministic behaviour for tests. The logged delay is now the actual one slept rather than the nominal constant, so the line stays truthful.
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.
What broke
On 2026-08-15 ~04:05 UTC the TCP connections from the GPU fleet to the Redis box were silently dropped. Seven
server_types stopped consuming within fifteen minutes of each other:Every one of them needed a manual restart. Nothing alerted —
serverless healthreported healthy, 0.0% error rate throughout, because it measures the HTTP invoke path, not the queue.Why
The worker sat in
blpop("ml_tasks")with no timeout.That is a pure read-wait. The client transmits nothing, so the kernel never retransmits, never reaches
tcp_retries2, and never raises. The thread parked forever on a socket the server had already discarded. This is why Redis reportedblocked_clients: 0while every worker believed it was blocked — the server had forgotten the connection; only the client still thought it existed.Threads that were sending commands did surface the failure, as
[Errno 110] Connection timed outabout eighteen minutes later (the standardtcp_retries2give-up window), and reconnected normally through_RedisWithRetry. The reader never could —_RedisWithRetryonly retries on a raised error, and an unboundedBLPOPnever raises.The Redis server itself was healthy the whole time: 0 rejected connections, 0 evictions, 28 days uptime. The GPUs were idle at 29-31°C with the model still resident. This is a client bug.
The fix
blpopnow takesBLPOP_TIMEOUTand the loop treatsNoneas "keep waiting". Completing the read is what lets a dead socket be noticed at all. Kept belowSOCKET_TIMEOUTbecause redis-py applies the socket read deadline to blocking commands too.socket_keepalive(with per-platform TCP knobs),socket_timeout,socket_connect_timeout,health_check_intervalandretry_on_timeout, so a half-open connection gets reaped instead of trusted._guarded_iteration. A barewhile True:in a thread was one unhandled exception away from being gone permanently — the pruning thread died exactly that way on every replica of several endpoints while the process carried on looking fine.Testing
6 new tests in
tests/test_connection_liveness.py. Suite goes 51 → 57, no existing test touched.Mutation-tested — each fix reverted individually, confirming the matching test turns red:
blpoptest_worker_blpop_passes_a_timeouttest_connection_pool_enables_half_open_detection_guarded_iterationre-raisetest_guarded_iteration_swallows_and_continues,test_heartbeat_loop_survives_a_raising_bodyNotes for the reviewer
prune_old_task_resultsSCAN storm, which is what triggered the pruning-thread crash; this PR stops any such error from being fatal. Both are worth having, in either order.**kwargsis deliberately not forwarded intoConnectionPool— callers pass unrelated kwargs to__init__today and forwarding them would raise at connect time.self.worker_healthyis setTrueonce at init andFalseatstart_workers, and never back to True. A one-way latch — once tripped the worker refuses tasks until the process restarts. It did not cause this outage; it deserves its own PR and a deliberate re-check policy.LLEN ml_tasks > 0and nothing has been consumed for N minutes, so the orchestrator can act on a stall this class of bug cannot itself detect.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.