rq/scheduler.py (1)

161-164: 💤 Low value

Iterate over successful_locks instead of _queue_names.

The loop updates locks for all queue names, but only successful_locks are actually owned. While xx=True prevents creating keys that don't exist, this issues unnecessary Redis commands for queues where the lock was lost.

♻️ Suggested fix

-        for name in self._queue_names:
+        for name in successful_locks:
             self.connection.set(
                 self.get_locking_key(name), f'{self._ppid}:{self._pid}:{self._token}', ex=self.interval + 60, xx=True
             )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rq/scheduler.py` around lines 161 - 164, The loop currently iterates
self._queue_names and issues Redis SETs for every queue even if the lock wasn't
acquired; change the loop to iterate over successful_locks (the collection of
queues we actually own) and call self.connection.set(self.get_locking_key(name),
f'{self._ppid}:{self._pid}:{self._token}', ex=self.interval + 60, xx=True) only
for those names so you avoid unnecessary Redis commands for lost locks while
keeping the same expiration and xx=True behavior.

rq/scheduler.py (1)

162-165: 💤 Low value

Consider iterating over successful_locks instead of _queue_names.

The loop updates locks for all queue names, but only successful_locks contain locks this scheduler owns. While SET with xx=True harmlessly fails for unowned locks, iterating over successful_locks would be more efficient and clearer in intent.

♻️ Suggested change

-        for name in self._queue_names:
+        for name in successful_locks:
             self.connection.set(
                 self.get_locking_key(name), f'{self._ppid}:{self._pid}:{self._token}', ex=self.interval + 60, xx=True
             )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rq/scheduler.py` around lines 162 - 165, The loop currently iterates
self._queue_names and calls self.connection.set(...) for each name even though
only locks in successful_locks are actually owned; change the loop to iterate
over successful_locks instead of self._queue_names so you only refresh owned
locks. Locate the block using successful_locks, replace the iterator from
self._queue_names to successful_locks, keeping the call to
self.connection.set(self.get_locking_key(name),
f'{self._ppid}:{self._pid}:{self._token}', ex=self.interval + 60, xx=True) so
ttl logic (interval) and token composition remain unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

rq/scheduler.py (1)

162-165: 💤 Low value

Iterate over successful_locks instead of _queue_names when updating locks.

The loop updates locks for all queue names, but only successful_locks contain verified locks. The XX flag makes the spurious writes no-ops, but iterating over successful_locks is cleaner and avoids unnecessary Redis calls.

♻️ Suggested improvement

-        for name in self._queue_names:
+        for name in successful_locks:
             self.connection.set(
                 self.get_locking_key(name), f'{self._ppid}:{self._pid}:{self._token}', ex=self.interval + 60, xx=True
             )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rq/scheduler.py` around lines 162 - 165, The loop currently iterates over
self._queue_names when calling self.connection.set(..., xx=True) to refresh
locks; change it to iterate over successful_locks (the collection of verified
locks) instead so only held locks are updated. Specifically, replace usages of
self._queue_names in that loop with successful_locks while still using
self.get_locking_key(name) and the same value
f'{self._ppid}:{self._pid}:{self._token}' and expiry self.interval + 60; keep
the xx=True behavior.

rq/scripts.py (2)

23-31: ⚖️ Poor tradeoff

Consider using weak references for connection keys to prevent memory leaks.

The cache uses Redis connection objects as dictionary keys. If connections are recreated over the application lifetime, stale entries accumulate since there's no cleanup mechanism.

For long-running processes that create many connections, consider using weakref.WeakKeyDictionary or restructuring the cache to use a hashable connection identifier.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rq/scripts.py` around lines 23 - 31, The _registered_scripts cache currently
uses Redis connection objects as keys (see _registered_scripts and
get_registered_script) which can leak when connections are recreated; replace
the plain dict with a weakref.WeakKeyDictionary keyed by the Redis connection
(or alternatively key by a stable connection identifier such as
connection.client_id or a connection-specific string) so entries are removed
when connections are garbage-collected, and update get_registered_script to use
that WeakKeyDictionary and still call connection.register_script(source) when
missing.

---

34-52: 💤 Low value

Guard against empty keys list to avoid unnecessary Redis roundtrip.

The wrapper calls the Lua script even when keys is empty, which is a no-op but still incurs network overhead.

🛡️ Proposed guard

 def delete_scheduler_locks(connection: Redis, token: str, keys: Sequence[str]) -> int:
+    if not keys:
+        return 0
     return get_registered_script(connection, DELETE_SCHEDULER_LOCKS)(keys=keys, args=[token])

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rq/scripts.py` around lines 34 - 52, The delete_scheduler_locks wrapper
currently calls the Lua script even when keys is empty; add a guard in the
delete_scheduler_locks function to return 0 immediately if keys is empty (e.g.,
if not keys: return 0) to avoid the unnecessary Redis roundtrip, otherwise
proceed to call get_registered_script(connection,
DELETE_SCHEDULER_LOCKS)(keys=keys, args=[token]).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

rq/scripts.py (1)

23-31: 💤 Low value

Script cache may retain references to closed connections.

Using connection as part of the cache key prevents garbage collection of connection objects even after they're closed. In long-running processes that dynamically create/destroy connections, this could cause a slow memory leak.

For typical RQ usage with stable connections this is acceptable, but consider using a WeakValueDictionary or connection ID-based key if dynamic connections become a concern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rq/scripts.py` around lines 23 - 31, The cache currently uses the tuple
(source, connection) in _registered_scripts which holds strong refs to Redis
connection objects; replace this with a weak-reference keyed structure so closed
connections can be GC'd: import weakref and change _registered_scripts to a
weakref.WeakKeyDictionary mapping connection -> dict[source, Script], then
update get_registered_script to look up the per-connection dict (creating it if
missing) and register/cache the Script under the source key rather than
embedding the connection in the tuple; keep function names _registered_scripts
and get_registered_script and continue to call
connection.register_script(source) as currently done.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes