fix(dashboard): keep session DB reads off event loop - #53966
0-CYBERDYNE-SYSTEMS-0 wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean refactoring: moves blocking SQLite SessionDB queries off the async event loop using asyncio.to_thread. The extraction of _count_active_sessions() as a sync helper is the right pattern. The two sync get_sessions / get_profiles_sessions route handlers correctly dropped the async keyword since they do synchronous DB work. Test confirms the event loop is not blocked (version endpoint completes within timeout).
Reviewed by Hermes Agent
Related: #50948 (moves other dashboard sync I/O off the event loop), #45491 (offloads cron profile scans), and the umbrella RFC #48564 (dashboard/TUI event-loop-starvation hardening). This PR is scoped specifically to session/status DB reads, so it complements rather than duplicates those — the cluster as a whole is chipping away at the same desktop-startup event-loop-starvation class. |
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/1bb7b59c5, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
|
Merged via #60884 (rebase) — the residual hunk of this PR (the |
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/6f78e9cd2, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/1bb7b59c5, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/1bb7b59c5, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/1bb7b59c5, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/2229a295a, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
Flip the handler from async def to sync def so FastAPI executes it in its threadpool: the SessionDB open + list_sessions_rich query no longer block the single uvicorn event loop. Residual hunk from PR NousResearch#53966 — that PR's get_profiles_sessions flip already landed via NousResearch#54523/1bb7b59c5, and its get_status offload is superseded by NousResearch#58238's read_only + timeout variant in this branch. (cherry picked from commit 414c12a)
Summary
Fix a dashboard event-loop starvation path caused by session database reads during desktop startup.
/api/statusactive-session counting into a worker thread withasyncio.to_thread(...)./api/sessionsand/api/profiles/sessionsto sync FastAPI handlers so their blockingSessionDBwork runs in FastAPI's worker pool.SessionDB.list_sessions_rich(...)slow and proves another API request can respond before that slow DB read finishes.Root cause
The desktop can see the backend port listening and the dashboard ready signal while later API calls still silently time out. One uncovered cause is synchronous
SessionDBwork running inside async FastAPI handlers, starving the event loop during status/session reads. In the reproduced failure, a slowSessionDB.list_sessions_rich(...)delayed an unrelated API request until the DB read completed.Relation to existing PRs
This complements, but does not duplicate:
This PR keeps the scope to session/status DB reads and their regression coverage.
Verification
python -m pytest tests/hermes_cli/test_web_server_boot_handshake.py -q -o 'addopts=' python -m py_compile hermes_cli/web_server.py tests/hermes_cli/test_web_server_boot_handshake.py git diff --check origin/main...HEADResults: focused pytest passed (
4 passed), py_compile passed, and diff check passed.