utils: Make RedisSemaphore.acquire atomic

[ideaship]

Problem

RedisSemaphore.acquire() checked capacity and took a slot in two
separate Redis round-trips:

if self.redis.zcard(self.key) < self.maxsize:   # CHECK
    self.redis.zadd(self.key, {identifier: now}) # USE

This is a time-of-check-to-time-of-use race: two clients can both run
ZCARD, both observe a free slot, and both ZADD, leaving the sorted set
with more than maxsize members. The semaphore that bounds concurrent
NetBox connections (create_netbox_semaphore, maxsize = NETBOX_MAX_CONNECTIONS, default 5) therefore over-admits under contention
— exactly when the cap matters most. A local 50-client harness peaked at
22 holders against maxsize 5.

A second, opposite defect lived in the same method: the cleanup boundary
now - 60 was computed once before the retry loop, so it never advanced
while a caller waited. Holders that expired during the wait were never
reclaimed, causing spurious timeouts when capacity had actually freed.

Fix

Replace the check-and-acquire with a single server-side Lua EVAL that
performs expired-holder cleanup, the capacity check, and the slot
reservation in one atomic step, so no other client can interleave between
the check and the reservation. Because now is recomputed each iteration
and passed into the script, the reclaim boundary advances with wall-clock
time — fixing the second defect in the same change. The 60-second window
is named HOLDER_EXPIRY.

Testing

The race needs genuinely concurrent clients against a real Redis and is
not reproducible with a mock. It was validated locally with a threaded
real-Redis harness (over-admit on the old code, never on the fixed code).
The committed regression tests run the production Lua script against
in-memory fakeredis (added as a dev dependency, with lupa for Lua
support) and pin the two invariants: the holder set never exceeds
maxsize, and the reclaim boundary advances across retries so a slot
freed mid-wait is granted.

Notes

• Surfaced while reviewing the test-only PR below; this is a pre-existing
  production defect in code that PR does not touch, so it is fixed
  separately:
  • Add unit tests for osism/utils semaphore, redlock and task locks #2349
• That PR adds tests pinning the previous behavior (including the frozen
  cleanup boundary); whichever lands second will need those assertions
  reconciled against this change.

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've left some high level feedback:

• Consider using a monotonic clock (e.g. time.monotonic()) for the retry loop and now passed into the Lua script to avoid issues if the system clock is adjusted during acquisition retries.
• If this Lua script will be invoked frequently, you may want to load it and use EVALSHA instead of eval to reduce script parsing overhead on Redis and make it easier to reuse across RedisSemaphore instances.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider using a monotonic clock (e.g. `time.monotonic()`) for the retry loop and `now` passed into the Lua script to avoid issues if the system clock is adjusted during acquisition retries.
- If this Lua script will be invoked frequently, you may want to load it and use `EVALSHA` instead of `eval` to reduce script parsing overhead on Redis and make it easier to reuse across `RedisSemaphore` instances.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}