Repository navigation
fix(dashboard): offload remaining SessionDB read endpoints (eventloop PRD A2) - #182
Conversation
Offloads the remaining dashboard SessionDB read sites from async handlers through sync helpers and adds Semaphore(2) bounding for heavyweight reads. Adds thread-assert coverage plus an AST-aware regression lint with bare/lambda/partial/inner-def/to_thread self-tests. Verified: scripts/run_tests.sh tests/test_web_server_sessiondb_eventloop.py tests/test_web_server.py tests/test_hermes_state.py -q; scripts/run_tests.sh tests/hermes_cli/test_dashboard_auth_status_endpoint.py -q.
|
| Filename | Overview |
|---|---|
| hermes_cli/web_server.py | Adds _session_db_read / _session_db_heavy_read_semaphore helpers and refactors five endpoints to offload blocking DB work to the executor; semaphore dict is never pruned, and _rename_session_record raises HTTPException from an executor thread (works but is unusual). |
| tests/test_web_server_sessiondb_eventloop.py | New test file with an AST-based lint asserting no bare SessionDB reads remain in async handlers, plus thread-identity and semaphore concurrency tests; SESSIONDB_READ_METHODS is missing get_compression_tip, resolve_session_id, and a few write-path methods, leaving gaps in the lint's coverage. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant EventLoop as asyncio Event Loop
participant SessionDbRead as _session_db_read()
participant Semaphore as Semaphore(2) heavy only
participant Executor as ThreadPoolExecutor
participant SyncHelper as _read_* blocking helper
participant DB as SessionDB SQLite
Client->>EventLoop: HTTP request
EventLoop->>SessionDbRead: "await _session_db_read(fn, args, heavy=...)"
alt "heavy=True"
SessionDbRead->>Semaphore: async with semaphore max 2 concurrent
Semaphore-->>SessionDbRead: acquired
end
SessionDbRead->>Executor: await _blocking_io loop.run_in_executor
Executor->>SyncHelper: fn args in thread
SyncHelper->>DB: _open_session_db_for_profile
DB-->>SyncHelper: db handle
SyncHelper->>DB: list_sessions_rich session_count etc
DB-->>SyncHelper: result
SyncHelper->>DB: db.close
SyncHelper-->>Executor: return result
Executor-->>SessionDbRead: result future resolved
alt "heavy=True"
SessionDbRead->>Semaphore: release
end
SessionDbRead-->>EventLoop: result
EventLoop-->>Client: JSON response
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant EventLoop as asyncio Event Loop
participant SessionDbRead as _session_db_read()
participant Semaphore as Semaphore(2) heavy only
participant Executor as ThreadPoolExecutor
participant SyncHelper as _read_* blocking helper
participant DB as SessionDB SQLite
Client->>EventLoop: HTTP request
EventLoop->>SessionDbRead: "await _session_db_read(fn, args, heavy=...)"
alt "heavy=True"
SessionDbRead->>Semaphore: async with semaphore max 2 concurrent
Semaphore-->>SessionDbRead: acquired
end
SessionDbRead->>Executor: await _blocking_io loop.run_in_executor
Executor->>SyncHelper: fn args in thread
SyncHelper->>DB: _open_session_db_for_profile
DB-->>SyncHelper: db handle
SyncHelper->>DB: list_sessions_rich session_count etc
DB-->>SyncHelper: result
SyncHelper->>DB: db.close
SyncHelper-->>Executor: return result
Executor-->>SessionDbRead: result future resolved
alt "heavy=True"
SessionDbRead->>Semaphore: release
end
SessionDbRead-->>EventLoop: result
EventLoop-->>Client: JSON response
Comments Outside Diff (1)
-
tests/test_web_server_sessiondb_eventloop.py, line 608-617 (link)SESSIONDB_READ_METHODSis missing several blocking DB methodsThe set currently omits
get_compression_tip,resolve_session_id,resolve_resume_session_id,set_session_title,set_session_archived,get_session_title, anddelete_session, all of which appear in_read_*helpers or remain in async handlers elsewhere in the file. If a future change callsdb.get_compression_tip(...)ordb.resolve_session_id(...)directly from an async function, the AST lint intest_web_server_has_no_bare_sessiondb_reads_in_async_handlerswill not catch it — defeating the purpose of the regression guard for those entry points.
Reviews (1): Last reviewed commit: "fix(dashboard): offload session read end..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _SESSION_DB_HEAVY_READ_SEMAPHORES: Dict[int, asyncio.Semaphore] = {} | ||
|
|
||
|
|
||
| def _session_db_heavy_read_semaphore() -> asyncio.Semaphore: | ||
| loop = asyncio.get_running_loop() | ||
| key = id(loop) | ||
| semaphore = _SESSION_DB_HEAVY_READ_SEMAPHORES.get(key) | ||
| if semaphore is None: | ||
| semaphore = asyncio.Semaphore(2) | ||
| _SESSION_DB_HEAVY_READ_SEMAPHORES[key] = semaphore | ||
| return semaphore |
There was a problem hiding this comment.
_SESSION_DB_HEAVY_READ_SEMAPHORES is keyed by id(loop), but entries are never removed. Every call to asyncio.run() (e.g., in the test suite) creates a new event loop, adds an entry, and then closes the loop — the entry stays forever. CPython's id() recycles memory addresses after GC, so a new loop can receive the same integer key as a closed one and inherit its stale asyncio.Semaphore object. If the semaphore held waiters when the previous loop was cancelled mid-flight, those stale Future handles remain in _waiters, and the next loop's acquire/release cycle will encounter them. A weakref.WeakValueDictionary or registering a loop-close callback to remove the entry are standard remedies. In a production process with a single, long-lived event loop this is dormant; it surfaces in test suites or multiprocessing workers that repeatedly call asyncio.run().
| def _rename_session_record( | ||
| session_id: str, | ||
| profile: Optional[str], | ||
| title: Optional[str], | ||
| archived: Optional[bool], | ||
| ): | ||
| """Blocking session update/read; callers must offload it.""" | ||
| db = _open_session_db_for_profile(profile) | ||
| try: | ||
| sid = db.resolve_session_id(session_id) | ||
| if not sid: | ||
| raise HTTPException(status_code=404, detail="Session not found") | ||
| if body.title is None and body.archived is None: | ||
| if title is None and archived is None: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Nothing to update; provide 'title' and/or 'archived'.", | ||
| ) | ||
| if body.title is not None: | ||
| if title is not None: | ||
| try: | ||
| db.set_session_title(sid, body.title or "") | ||
| db.set_session_title(sid, title or "") | ||
| except ValueError as e: | ||
| # Title too long, invalid characters, or already in use. | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| if body.archived is not None: | ||
| db.set_session_archived(sid, body.archived) | ||
| if archived is not None: | ||
| db.set_session_archived(sid, archived) | ||
| result = {"ok": True, "title": db.get_session_title(sid) or ""} | ||
| if body.archived is not None: | ||
| result["archived"] = bool(body.archived) | ||
| if archived is not None: | ||
| result["archived"] = bool(archived) | ||
| return result | ||
| finally: | ||
| db.close() |
There was a problem hiding this comment.
HTTPException from an executor-thread function is surprising and fragile
_rename_session_record (and _read_session_detail) raise HTTPException — a Starlette/FastAPI framework class — from inside a plain blocking function that is dispatched via run_in_executor. This works today because asyncio propagates the exception through the future back to the awaiting coroutine, and FastAPI's exception handler catches it. However, it tightly couples a framework-level exception to a function documented as a portable sync helper, makes the helper untestable without a running ASGI app, and can confuse future callers who expect only ValueError/RuntimeError from a DB-layer function. The conventional pattern is to return a sentinel (e.g., None) or raise a domain-specific exception and convert to HTTPException in the async caller.
1 participant
Lane A2 of docs/desktop/2026-07-02-dashboard-eventloop-starvation-PRD.md (v1.0). Remaining 8 sites offloaded; Semaphore(2) for heavy reads (D-3); thread-assert tests per site; AST-aware regression lint with bare/lambda/partial/inner-def/to_thread self-test matrix (AC-4); 9-site affinity record (pass-4 note). 313 tests pass on rebased fork/main. Reviewed by Apollo.