하나의 이벤트 루프 회전, 하나의 잘못된 Redis 용량 오류

This is a submission for DEV’s Summer Bug Smash: Clear the Lineup powered by Sentry.

Project Overview

redis-py is the Python client for Redis. Its asynchronous cluster implementation maintains a per-node connection pool with an in-use set, a free queue, and optional max_connections capacity.

The bug was not a leaked connection or a deadlock. It was one event-loop turn in which a usable pool slot existed in neither place.

Bug Fix or Performance Improvement

An active async cluster connection can be marked for reconnect while a previous disconnect is still waiting for the socket to close. The disconnect clears the reconnect flag before it suspends, but another error or maintenance notification can mark the connection again during that wait.

By the time ClusterNode.release() receives it, the connection is closed but marked again.

The old release path treated every marked connection the same:

if connection.should_reconnect():
    task = asyncio.create_task(self._disconnect_and_release(connection))
    self._background_tasks.add(task)
    task.add_done_callback(self._background_tasks.discard)
    return
self._free.append(connection)

Enter fullscreen mode Exit fullscreen mode

For an already-closed connection, the second disconnect performs no I/O. Its only useful operation is appending the connection to _free, and that operation is deferred until the background task runs.

At max_connections=1, a concurrent acquire in that gap sees:

_free is empty
len(_connections) == max_connections

Enter fullscreen mode Exit fullscreen mode

and raises MaxConnectionsError. Capacity is about to return, but the caller receives a real application error first.

Code

The diagnosis and proposed directions came from petyaslavova in issue #4247. She traced the ownership gap, identified the reachable re-marking triggers, and suggested either returning an already-closed connection inline or making release() async.

My contribution in redis-py PR #4256 was to implement the smaller guarded fix and add a deterministic regression test through the real execute_command() path.

The repaired branch keeps the background disconnect for connections that are still open. If the connection is already closed, it clears the stale flag and returns the slot immediately:

if connection.should_reconnect():
    if connection.is_connected:
        task = asyncio.create_task(self._disconnect_and_release(connection))
        self._background_tasks.add(task)
        task.add_done_callback(self._background_tasks.discard)
        return
    connection.reset_should_reconnect()
self._free.append(connection)

Enter fullscreen mode Exit fullscreen mode

This preserves the safety rule that a marked, connected socket must never go back into circulation. It also avoids scheduling a no-op disconnect for one that is already closed.

My Improvements

Reproduce the race through the real command path

A test that manually set two flags and called release() would prove the branch, but not that production control flow can reach it. The regression test instead drives ClusterNode.execute_command() with a scripted connection and uses asyncio.Event objects to control the interleaving:

1. Start sending the command.
2. Mark active connections for reconnect.
3. Let the response complete so the normal error path starts disconnecting.
4. Wait until the disconnect has suspended.
5. Mark the same connection again.
6. Allow the disconnect to finish.
7. Acquire the next connection immediately.

The final assertions check the behavior that matters:

assert node.acquire_connection() is connection
assert node._background_tasks == set()
assert connection.should_reconnect() is False
assert connection.disconnect_calls == 1

Enter fullscreen mode Exit fullscreen mode

Before the fix, that acquire can hit the transient false-capacity window and the release path schedules a redundant second disconnect. After the fix, the same connection is available inline.

Prefer the narrow invariant repair

One alternative was to add a waiting acquisition API and turn more of the pool surface asynchronous. That would change public behavior and add coordination around a symptom.

The smaller fix restores the existing pool invariant: once a closed connection is released, it should be reusable immediately. No new public method, retry policy, timeout, or queue is required.

Keep the scope honest

This affects the async cluster pool when a node is at an explicitly configured connection limit and a reconnect mark lands during disconnect. It does not affect the synchronous cluster pool or the standalone pools. It causes a transient spurious MaxConnectionsError, not a permanent client hang.

The focused cluster connection-handling suite passed 13 tests on Windows and Ubuntu WSL. The changed code passed the repository’s lint task, Ruff checks, formatting, vulture, and git diff --check. Cursor Bugbot passed on the public PR. The upstream GitHub Actions for CI, Docs CI, CodeQL, and spellcheck currently await maintainer approval, so I am not treating the full repository matrix as passed.

Result

An already-closed, re-marked connection now returns to the free queue in the same release call. The pool no longer reports false exhaustion simply because a no-op background task has not received its event-loop turn yet.

Concurrency bugs are often described as timing problems, but the useful question is usually about ownership: at every suspension point, which structure owns the resource, and can another task observe a state in which nobody can use it? Here, the implementation changed 6 additions and 4 deletions in production code, while the larger part of the patch made the execute_command() regression deterministic.