fix: use AsyncSession in fetch_parsing_status to prevent event loop blocking#651
Conversation
…locking
The fetch_parsing_status method was declared async but used a synchronous
SQLAlchemy Session, blocking the event loop on every call. Under concurrent
load this causes request queuing and degraded performance.
Updated to use AsyncSession with proper await on all database operations,
consistent with the existing fetch_parsing_status_by_repo implementation
in the same file.
Also updated the /parsing-status/{project_id} route in parsing_router.py
to inject AsyncSession via get_async_db instead of the synchronous get_db,
and updated the /parsing-status POST route similarly since
fetch_parsing_status_by_repo already expected AsyncSession.
Fixes potpie-ai#639
WalkthroughThe pull request converts synchronous database session usage to asynchronous patterns in the parsing status endpoints. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/modules/parsing/graph_construction/parsing_controller.py (1)
397-434:⚠️ Potential issue | 🔴 CriticalFix ParseHelper and ProjectService to handle AsyncSession correctly.
ParseHelper(db)receives anAsyncSessionat line 425, butParseHelper.__init__is typed to accept a syncSessionand passes it toProjectService. The problem manifests incheck_commit_status(): it callsawait self.project_manager.get_project_from_db_by_id(project_id), which internally invokesProjectService.get_project_by_id()— a method that usesdb.query(Project).filter(...).first().AsyncSessiondoes not support the.query()method; it only supportsselect()withawait db.execute(). This will raise anAttributeErrorat runtime.Either update
ParseHelperandProjectServiceto accept and work withAsyncSessionusing async patterns, or pass a syncSessioninstead ofAsyncSessiontoParseHelper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/modules/parsing/graph_construction/parsing_controller.py` around lines 397 - 434, ParseHelper is being constructed with an AsyncSession but its __init__ and downstream ProjectService still expect a sync Session, causing AttributeError when ProjectService.get_project_by_id uses db.query(...); update ParseHelper.__init__ to accept AsyncSession (or overload) and refactor ProjectService methods used by ParseHelper (e.g., get_project_by_id, get_project_from_db_by_id) to use async SQLAlchemy patterns: replace .query()/.filter()/first() with select(Project).where(...), call await db.execute(...) and use result.scalars().first(), mark methods async and await their calls (e.g., check_commit_status -> await project_manager.get_project_from_db_by_id), and adjust type hints from Session to AsyncSession; alternatively, if you prefer sync flow, ensure you pass a sync Session into ParseHelper instead of AsyncSession.
🧹 Nitpick comments (1)
app/modules/parsing/graph_construction/parsing_router.py (1)
29-30: Ruff B008 warnings are false positives for FastAPI dependency injection.
Depends(...)in argument defaults is the canonical FastAPI DI pattern and Ruff B008 is a well-known false positive here. The same pattern is used on pre-existing lines (e.g., Line 20, Line 21, Line 39) without suppression. If the project enforces Ruff strictly, consider adding a project-levelB008ignore for FastAPI router files, or inline# noqa: B008comments — but this is consistent with the existing codebase style.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/modules/parsing/graph_construction/parsing_router.py` around lines 29 - 30, Ruff B008 flags are false positives for FastAPI dependency injection in parsing_router.py; update the dependency argument lines that use Depends (e.g., the parameters using AsyncSession with get_async_db and user=Depends(AuthService.check_auth), and the other occurrence around line 38) to suppress B008 consistently with the codebase by adding an inline noqa comment (“# noqa: B008”) to those function parameter lines or alternatively add a project-level ignore for B008 for FastAPI router files so the Depends(...) patterns are not reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/modules/parsing/graph_construction/parsing_controller.py`:
- Around line 397-434: ParseHelper is being constructed with an AsyncSession but
its __init__ and downstream ProjectService still expect a sync Session, causing
AttributeError when ProjectService.get_project_by_id uses db.query(...); update
ParseHelper.__init__ to accept AsyncSession (or overload) and refactor
ProjectService methods used by ParseHelper (e.g., get_project_by_id,
get_project_from_db_by_id) to use async SQLAlchemy patterns: replace
.query()/.filter()/first() with select(Project).where(...), call await
db.execute(...) and use result.scalars().first(), mark methods async and await
their calls (e.g., check_commit_status -> await
project_manager.get_project_from_db_by_id), and adjust type hints from Session
to AsyncSession; alternatively, if you prefer sync flow, ensure you pass a sync
Session into ParseHelper instead of AsyncSession.
---
Nitpick comments:
In `@app/modules/parsing/graph_construction/parsing_router.py`:
- Around line 29-30: Ruff B008 flags are false positives for FastAPI dependency
injection in parsing_router.py; update the dependency argument lines that use
Depends (e.g., the parameters using AsyncSession with get_async_db and
user=Depends(AuthService.check_auth), and the other occurrence around line 38)
to suppress B008 consistently with the codebase by adding an inline noqa comment
(“# noqa: B008”) to those function parameter lines or alternatively add a
project-level ignore for B008 for FastAPI router files so the Depends(...)
patterns are not reported.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/modules/parsing/graph_construction/parsing_controller.pyapp/modules/parsing/graph_construction/parsing_router.py
1 participant
Summary
Fixes #639
fetch_parsing_statuswas declaredasyncbut used a synchronous SQLAlchemySession, which blocks the event loop on every database call. Under concurrent load this serializes requests and degrades API performance.Changes
fetch_parsing_statusinparsing_controller.pyto acceptAsyncSessioninstead ofSessionawaitto thedb.execute(project_query)call inside the method/parsing-status/{project_id}route inparsing_router.pyto injectAsyncSessionviaget_async_dbinstead of the synchronousget_db/parsing-statusPOST route inparsing_router.pythe same way, sincefetch_parsing_status_by_repoalready expectedAsyncSessionReference
The adjacent
fetch_parsing_status_by_repomethod in the same file already implements this pattern correctly and was used as the reference implementation. Both methods now follow the same async database access pattern.Testing
get_async_dbdependency is already defined inapp/core/database.pyand used by other routesSummary by CodeRabbit