fix(ENG-26): wire up expired token cleanup#10
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ai_trading_discipline_copilot/routers/auth.py`:
- Around line 93-100: The three cleanup operations in run_cleanup_task are
currently coupled, so a failure in RefreshTokenService.cleanup_expired_sessions
can prevent EmailVerificationService.cleanup_expired_tokens and
PasswordResetService.cleanup_expired_tokens from running. Update
run_cleanup_task to isolate each service call independently, ideally with
separate try/except handling around each cleanup invocation while still using
the AsyncSessionFactory session. Keep the existing method names and flow, but
ensure one transient failure does not skip the remaining cleanup tasks.
🪄 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: 1a002362-13da-4b10-9e5a-1cc970609eb7
📒 Files selected for processing (2)
src/ai_trading_discipline_copilot/routers/auth.pytests/conftest.py
| async def run_cleanup_task() -> None: | ||
| """Background task to clean up expired sessions using a fresh DB session.""" | ||
| """Clean up expired sessions and tokens using a fresh DB session.""" | ||
| from ai_trading_discipline_copilot.core.database import AsyncSessionFactory | ||
|
|
||
| async with AsyncSessionFactory() as db: | ||
| await RefreshTokenService.cleanup_expired_sessions(db) | ||
| await EmailVerificationService.cleanup_expired_tokens(db) | ||
| await PasswordResetService.cleanup_expired_tokens(db) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the scheduler/caller of run_cleanup_task to check for exception handling at the call site.
rg -nP -C5 '\brun_cleanup_task\s*\(' --type=pyRepository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning run_cleanup_task =="
rg -n --hidden --glob '!*.pyc' 'run_cleanup_task' .
echo
echo "== files mentioning cleanup task scheduling or background task setup =="
rg -n --hidden --glob '!*.pyc' 'cleanup.*task|schedule.*cleanup|BackgroundTasks|APScheduler|cron|scheduler|periodic' src tests .Repository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 1785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,190p' src/ai_trading_discipline_copilot/routers/auth.py | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 6924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cleanup methods =="
rg -n -C4 'def cleanup_expired_(sessions|tokens)\(' src/ai_trading_discipline_copilot/services
echo
echo "== background task helpers =="
rg -n -C4 'def send_verification_email_task|background_tasks.add_task\(run_cleanup_task\)|BackgroundTasks' src/ai_trading_discipline_copilot/routers/auth.pyRepository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 4792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== refresh token cleanup =="
sed -n '158,230p' src/ai_trading_discipline_copilot/services/refresh_token_service.py | cat -n
echo
echo "== email verification cleanup =="
sed -n '177,245p' src/ai_trading_discipline_copilot/services/email_verification_service.py | cat -n
echo
echo "== password reset cleanup =="
sed -n '186,255p' src/ai_trading_discipline_copilot/services/password_reset_service.py | cat -n
echo
echo "== auth routes around admin cleanup endpoint =="
sed -n '300,345p' src/ai_trading_discipline_copilot/routers/auth.py | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 7494
🌐 Web query:
FastAPI BackgroundTasks exception behavior sequential execution request completes background task errors stop remaining tasks
💡 Result:
In FastAPI, BackgroundTasks are executed after the response is sent to the client [1][2]. Their execution behavior regarding errors and sequence is summarized below: Sequential Execution: Background tasks added to a single BackgroundTasks object are executed sequentially in the order they were added [2]. They do not run concurrently by default; if you require concurrent execution, you must explicitly manage this, for example by using asyncio.gather inside a single background task [3]. Exception Behavior: If a background task raises an unhandled exception, it does not automatically stop the execution of subsequent tasks in the same queue [2]. However, because background tasks run outside the request-response cycle, unhandled exceptions in these tasks will not be caught by FastAPI's standard exception handlers, which are designed to return responses to clients [4][5]. Instead, these exceptions will typically bubble up to the ASGI server (e.g., Uvicorn), where they will be logged to the server's standard output or error logs [4]. Because the response has already been sent to the client by the time the background task runs, the task cannot return a response or trigger an HTTP error status [4][5]. Recommendation: To ensure robustness, you should wrap the logic inside each background task in a try-except block to handle potential errors internally [6]. This prevents the task from failing silently or impacting the server process logs, allowing you to implement custom logging, alerting, or cleanup logic as needed [6].
Citations:
- 1: https://fastapi.tiangolo.com/tutorial/background-tasks/
- 2: https://fastapi.tiangolo.com/reference/background/?h=background
- 3: how to make backgroud tasks that are added in one path operation call to run concurrently? fastapi/fastapi#13724
- 4: Raising exceptions with registered exception handlers from BackgroundTasks causes starlette exception fastapi/fastapi#3589
- 5: BackgroundTasks do not run when request failed fastapi/fastapi#2604
- 6: https://stackoverflow.com/questions/75145424/fastapi-starlette-how-to-handle-exceptions-inside-background-tasks
Isolate the three cleanup calls (src/ai_trading_discipline_copilot/routers/auth.py:93-100). run_cleanup_task runs as a FastAPI background task on login, so a failure in cleanup_expired_sessions aborts the email-verification and password-reset cleanups in that run. Wrap each call independently so one transient DB error doesn’t skip the others.
🤖 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 `@src/ai_trading_discipline_copilot/routers/auth.py` around lines 93 - 100, The
three cleanup operations in run_cleanup_task are currently coupled, so a failure
in RefreshTokenService.cleanup_expired_sessions can prevent
EmailVerificationService.cleanup_expired_tokens and
PasswordResetService.cleanup_expired_tokens from running. Update
run_cleanup_task to isolate each service call independently, ideally with
separate try/except handling around each cleanup invocation while still using
the AsyncSessionFactory session. Keep the existing method names and flow, but
ensure one transient failure does not skip the remaining cleanup tasks.
2 participants
Summary by CodeRabbit