Add asyncio.Lock around rate limiter bucket state - #119
Conversation
The in-memory token-bucket rate limiter in `src/opensoar/middleware/rate_limit.py` read and mutated a module-level `_buckets: dict[str, list[float]]` without any synchronization. Under concurrent webhook load, two coroutines could both observe an under-capacity bucket and both append, allowing more than `max_requests` through the window. Fix: - Add a module-level `_lock: asyncio.Lock | None = None` guard. - Initialize it lazily via `_get_lock()` on first async call so we don't require a running event loop at import time. - Hold the lock across the read-modify-write of `_buckets[key]` in `RateLimitMiddleware.dispatch`. Tests cover: - `_get_lock()` returns an `asyncio.Lock` and is idempotent. - `max_requests + 1` concurrent dispatches yield exactly `max_requests` successes and exactly one 429. - After N concurrent successes the bucket holds exactly N entries. Closes #106
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/opensoar/middleware/rate_limit.py`:
- Around line 15-28: The module-level lazy async lock (_lock and _get_lock) is
unnecessary and the async-with guard around synchronous list ops adds overhead;
remove the _lock variable and the _get_lock() function and eliminate usage sites
that do "async with await _get_lock()" (replace them with the synchronous code
path that currently runs inside the block). If you actually need cross-thread
protection instead, replace the pattern with a threading.Lock instantiated at
module import and use regular with threading_lock; otherwise simply remove the
lock and its awaits so functions that referenced _get_lock (and the symbols
_lock/_get_lock) operate without the redundant async synchronization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f61e65fe-f420-4534-9eb3-ccd3ea7080d9
📒 Files selected for processing (2)
src/opensoar/middleware/rate_limit.pytests/test_rate_limit.py
| # Lock that guards access to ``_buckets``. Lazily initialized on first async | ||
| # call because there may not be a running event loop at import time (issue #106). | ||
| _lock: asyncio.Lock | None = None | ||
|
|
||
|
|
||
| async def _get_lock() -> asyncio.Lock: | ||
| """Return the module-level asyncio.Lock, creating it on first use. | ||
|
|
||
| Must be called from within a running event loop. | ||
| """ | ||
| global _lock | ||
| if _lock is None: | ||
| _lock = asyncio.Lock() | ||
| return _lock |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ast
import asyncio
from pathlib import Path
path = next(Path('.').rglob('rate_limit.py'))
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == 'RateLimitMiddleware':
for fn in node.body:
if isinstance(fn, ast.AsyncFunctionDef) and fn.name == 'dispatch':
awaits = sorted({n.lineno for n in ast.walk(fn) if isinstance(n, ast.Await)})
print("dispatch await lines:", awaits)
lock = asyncio.Lock()
print("constructed lock type:", type(lock).__name__)
PYRepository: opensoar-hq/opensoar-core
Length of output: 131
🏁 Script executed:
cat -n src/opensoar/middleware/rate_limit.py | sed -n '15,90p'Repository: opensoar-hq/opensoar-core
Length of output: 3069
The lock guards only synchronous operations and is unnecessary; also, the lazy initialization pattern is based on a misunderstanding of asyncio.Lock() behavior.
The code at lines 71–85 inside async with lock: contains no await statements—only list operations, comparisons, and a conditional return. On a single event loop, coroutines can only be preempted at await points, so this synchronous block runs atomically without needing explicit synchronization. The lock adds no real protection for the current code path.
Additionally, the comment at lines 15–16 claims asyncio.Lock() cannot be constructed without a running event loop, but asyncio.Lock() can be safely constructed at import time in modern Python. The _get_lock() wrapper function adds unnecessary indirection to solve a non-existent problem. If concurrent access from threads or processes is the actual concern, asyncio.Lock does not solve that anyway.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/opensoar/middleware/rate_limit.py` around lines 15 - 28, The module-level
lazy async lock (_lock and _get_lock) is unnecessary and the async-with guard
around synchronous list ops adds overhead; remove the _lock variable and the
_get_lock() function and eliminate usage sites that do "async with await
_get_lock()" (replace them with the synchronous code path that currently runs
inside the block). If you actually need cross-thread protection instead, replace
the pattern with a threading.Lock instantiated at module import and use regular
with threading_lock; otherwise simply remove the lock and its awaits so
functions that referenced _get_lock (and the symbols _lock/_get_lock) operate
without the redundant async synchronization.
Deploying opensoar-docs with
|
| Latest commit: |
ae75785
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://91f55549.opensoar-docs.pages.dev |
| Branch Preview URL: | https://fix-rate-limiter-asyncio-loc.opensoar-docs.pages.dev |
1 participant
Summary
src/opensoar/middleware/rate_limit.pyheld its token-bucket state in a module-leveldict[str, list[float]]with no synchronization, so two coroutines could both observe an under-capacity bucket and both append, slipping more thanmax_requeststhrough the window._lock: asyncio.Lock | None(via_get_lock()) so we don't assume a running event loop at import time._buckets[key]inRateLimitMiddleware.dispatchwithasync with lock:to make concurrency guarantees explicit.Test plan
ruff check src/ tests/passespytest tests/passes (549 passed)tests/test_rate_limit.py::TestRateLimitConcurrency:test_lock_is_asyncio_lock—_get_lock()returns anasyncio.Lockand is idempotenttest_concurrent_dispatch_exact_count—max_requests + 1concurrent requests yield exactlymax_requestssuccesses and exactly one 429test_concurrent_dispatch_no_list_corruption— after N concurrent successes the bucket holds exactly N entriestest_lock_is_asyncio_lockverified to fail against the pre-fix implementationCloses #106
Summary by CodeRabbit