fix(ci): make multiprocessing rate-limit test picklable on Python 3.14#46
Conversation
Default docker-compose stack now runs Redis and sets RATELIMIT_STORAGE_URI=redis://redis:6379/0 so the 5/min login cap is enforced globally instead of per worker (fixes #44). Adds regression tests documenting single-process enforcement and the per-process memory:// behavior that multi-worker deployments must avoid. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
Move attempt_logins to module scope so Process can pickle the target on forkserver-based multiprocessing (Python 3.14 CI). Add NOSONAR for intentional CSRF disable in the subprocess test helper. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change documents configurable rate-limit storage, provisions Redis in Docker Compose for shared Gunicorn-worker limits, and tests login throttling plus per-process behavior with in-memory storage. ChangesLogin rate-limit storage
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
SonarCloud flagged the new WTF_CSRF_ENABLED=False line in test_login_rate_limit_blocks_after_five_attempts as an unreviewed Security Hotspot (S4502), dropping the PR's Security Rating to D. Mark it with the same NOSONAR justification already used for the equivalent line in the subprocess helper.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In @.env.example:
- Around line 26-28: Update the RATELIMIT_STORAGE_URI example so copying
.env.example preserves Compose’s Redis-backed rate-limit configuration: leave
the key unset/commented, or set it to the Redis URI. If using the Redis value,
document the memory:// override for single-process local development in the
surrounding environment guidance.
In `@tests/test_login_routes.py`:
- Around line 56-63: Set RATELIMIT_STORAGE_URI to the intended memory:// backend
before each create_app() call, including the subprocess helper
_attempt_login_rate_limit_in_subprocess and the other application setup at the
referenced location. Ensure the environment override occurs before application
initialization so externally configured Redis values cannot affect these
isolation tests.
- Around line 108-114: Update the multiprocessing test around the worker
start/join and queue collection to use finite timeouts for both worker.join()
and queue.get(), so crashed or stalled children fail promptly instead of hanging
CI. After each join, inspect the worker’s exit code and fail the test when it is
nonzero, while preserving the existing accepted_per_worker assertion for
successful workers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49d83180-6244-4a20-a17a-f4e30924cd7e
📒 Files selected for processing (5)
.env.exampleCHANGELOG.mdREADME.mddocker-compose.ymltests/test_login_routes.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2860249036
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SECRET_KEY=<generate-with-python3-secrets-token-hex-32> | ||
| # Shared rate-limit store for multi-worker Gunicorn (docker-compose defaults to redis://redis:6379/0). | ||
| # Use memory:// only for single-process local dev (python3 app.py). | ||
| RATELIMIT_STORAGE_URI=memory:// |
There was a problem hiding this comment.
When a user follows the README quick start and copies .env.example to .env, Compose uses that .env value for interpolation, and ${RATELIMIT_STORAGE_URI:-redis://redis:6379/0} in docker-compose.yml only falls back to Redis when the variable is unset or empty. Because this new line sets a non-empty memory://, the Docker stack will start Redis but configure the panel to use per-process memory limits, so multi-worker deployments still allow 5 × worker_count login attempts per minute; leave this unset/commented or set it to redis://redis:6379/0 in the example.
Useful? React with 👍 / 👎.
| for worker in workers: | ||
| worker.join() | ||
|
|
||
| accepted_per_worker = [queue.get() for _ in workers] |
There was a problem hiding this comment.
If any child process exits before queue.put(...) (for example because app initialization fails under a CI environment variable or the forkserver import path regresses), the parent joins it and then blocks forever waiting for a result that will never arrive. Checking each worker.exitcode and/or using bounded join/queue.get(timeout=...) would turn those subprocess failures into actionable test failures instead of CI timeouts.
Useful? React with 👍 / 👎.
- .env.example: stop shipping RATELIMIT_STORAGE_URI=memory:// as a
concrete value. Since docker-compose's ${VAR:-redis://...} fallback
only triggers when the var is unset/empty, copying .env.example to
.env per the README quick start silently re-enabled per-worker
memory limiting and reopened the brute-force gap from #44. Leave it
commented out with the memory:// override documented for local dev.
- tests: pin RATELIMIT_STORAGE_URI to memory:// via
patch("app.Config.RATELIMIT_STORAGE_URI", ...) before create_app()
in both rate-limit tests so they stay hermetic regardless of the
ambient environment.
- tests: bound the multiprocessing isolation test's worker.join() and
queue.get() with timeouts and check exit codes, so a crashed child
fails the test instead of hanging CI indefinitely.
|
3 participants
Fixes CI failure on #45.
The
test_login_rate_limit_is_per_process_with_memory_storagetest failed in GitHub Actions withPicklingErrorbecause Python 3.14 uses forkserver-based multiprocessing, which cannot pickle nested functions passed toProcess._attempt_login_rate_limit_in_subprocess)NOSONARfor intentional CSRF disable in the subprocess test helperNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Security
Configuration
RATELIMIT_STORAGE_URIto control where rate-limit state is stored (e.g., Redis for multi-worker, memory for local).Documentation
CHANGELOG.md,README.md, and.env.examplewith the new configuration details.Tests