Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Summary
Keeps dashboard status responsive when session db locks by adding a read-only SessionDB connection with its own timeout (_STATUS_ACTIVE_SESSIONS_TIMEOUT = 0.75s) for counting active sessions on /api/status.
Looks Good
- Read-only db connection prevents write contention
- Independent timeout from main gateway health probe
- Best-effort pattern: on timeout, returns 0 gracefully
- Proper executor pattern for async-safe db access
- No security concerns
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (LGTM — token read-only, formal approval deferred to maintainer)
Dashboard status endpoint robustness fix: refactors active session counting into a dedicated async function with a 0.75s timeout and read-only database connection. This prevents /api/status from blocking or competing with write operations during session DB lock contention. The timeout degrades gracefully to 0 with debug logging.
Looks Good
- Read-only SessionDB connection avoids write contention
- Proper timeout on run_in_executor prevents blocking the dashboard response
- Degrades to 0 on timeout or exception (not a hard failure)
- Tests verify read-only flag is passed and lock failures return 0 gracefully
Reviewed by Hermes Agent
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
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)
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
Summary
Context
Addresses the SessionDB lock path reported in #57203. The existing open PRs for that issue focus on the gateway health probe timeout, but the issue follow-up narrowed the observed 21s delay to SessionDB().list_sessions_rich(limit=50) after read_runtime_status(). This PR targets that separate blocking path.
Tests