ACE-048: offload blocking work off the event loop + uvicorn worker factory - #104
Conversation
… event loop New leaf module async_offload.run_blocking(fn,*args,**kwargs) (functools.partial over anyio.to_thread.run_sync; positional-only otherwise). The two login handlers (oauth authorize, admin_login) now offload the credential check so a ~50-100ms argon2 verify never freezes the single asyncio worker. Design correction found on first run: offloading the *whole* authenticate(store,...) failed — a SQLite connection is thread-bound and can't be used from the worker thread. Fix: a sync user_store.authenticate_with_own_store() that opens/uses/closes its OWN Store inside the worker thread; the handler's loop-thread Store is never shared. authenticate stays sync (20+ test callers unchanged). 231 auth tests green; ruff clean.
Slice 2: offload the OIDC token exchange + JWKS-backed id-token verification (oidc_callback) and the discovery-resolving authorize_url (oidc_start/admin_oidc_start) and the per-tool-call audit INSERT (mcp_http _call_tool finally) via run_blocking, so a slow IdP / DB write never freezes the loop. The query-subprocess block itself stays on-loop (known issue, parked ACE-028). Slice 3: main() binds uvicorn to the import-string factory 'mcp_http:build_app' (factory=True) with WORKERS env (default 1) so --workers=N can fork. Multi-worker is safe: stateless JWT + Postgres, boot migrations already guarded by store.run_migrations' pg_advisory_lock (Cloud-Run multi-instance), admin seed tolerates the boot race. (No new migration lock needed - it already exists.) Tests: run_blocking args+kwargs off-thread + exception propagation; factory import string resolves to a Starlette app. Full gate green (ruff + all tests + gitleaks + lib-drift).
There was a problem hiding this comment.
Pull request overview
This PR addresses event-loop blocking in the hosted HTTP MCP server by offloading synchronous/IO-heavy operations (password verification, OIDC flows, and per-tool-call audit writes) to worker threads, and updates the server entrypoint to use a uvicorn factory import string so --workers can spawn multiple processes.
Changes:
- Add
async_offload.run_blocking()(anyio thread offload) and use it in OAuth/admin login, OIDC start/callback, and tool-call audit logging. - Add
user_store.authenticate_with_own_store()to safely run credential checks in worker threads without sharing loop-thread DB connections. - Switch
mcp_http.main()touvicorn.run("mcp_http:build_app", factory=True, workers=WORKERS)and add tests covering offload + factory resolution.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_async_offload.py | Adds coverage for run_blocking semantics and uvicorn factory import resolution. |
| packages/agami-core/src/async_offload.py | Introduces the run_blocking() helper wrapping anyio thread offload with kwargs support. |
| packages/agami-core/src/user_store.py | Adds authenticate_with_own_store() to open/close a Store within the worker thread. |
| packages/agami-core/src/oauth_server.py | Offloads password auth and OIDC network/verification work off the event loop. |
| packages/agami-core/src/admin.py | Offloads admin password auth off the event loop via run_blocking. |
| packages/agami-core/src/mcp_http.py | Offloads tool-call audit INSERTs; switches uvicorn to a multi-worker-capable factory import string. |
| packages/agami-core/pyproject.toml | Adds async_offload to the packaged flat module list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await run_blocking( | ||
| record_tool_call, | ||
| name=name, | ||
| arguments=arguments, | ||
| result_text=result_text, |
There was a problem hiding this comment.
Declining this one — the premise is incorrect for this code. asyncio.CancelledError subclasses BaseException, not Exception (since Python 3.8), so except Exception: pass does not catch it: cancellation propagates out of the finally normally. The audit write is intentionally best-effort and the runtime is 3.11+. Leaving this thread open for a human call rather than self-resolving a disagreement with the bot.
- declare anyio>=4 in server deps (async_offload imports it; was only transitive via mcp) - test_async_offload adds packages/agami-core/src to sys.path so it imports async_offload/mcp_http when run in isolation (matches other server tests)
|
Thanks @copilot — dispositioned all three: Fixed (2):
Declined (1) — the `finally`/cancellation concern: |
Both fixes are confirmed in commit
On the cancellation point — agreed, |
Summary
Behaviour-preserving availability fix (audit P3, feature F12, high/auth lane). The single asyncio worker ran argon2 verify (~50–100 ms), OIDC token exchange (up to a 10 s timeout), and per-tool-call audit INSERTs synchronously on the loop → one login / callback / tool call froze all concurrent users. And
uvicorn.run(build_app(), …)passed an app instance, so--workers>1was impossible. Now the blocking calls run off-loop, and uvicorn binds a factory import-string so--workers=Nworks.Changes
async_offload.run_blocking(fn, *args, **kwargs)(functools.partialoveranyio.to_thread.run_sync).oauth authorize,admin_login) offload the credential check via a newuser_store.authenticate_with_own_store()that opens its own Store in the worker thread — a loop-thread SQLite connection is thread-bound and can't cross threads.authenticatestays sync (20+ existing test callers unchanged).oidc_callbackoffloadsexchange_code+verify_id_token;oidc_start/admin_oidc_startoffloadauthorize_url._call_tool'sfinallyoffloadsrecord_tool_call(actorread on-loop, passed in). Still best-effort.main()→uvicorn.run("mcp_http:build_app", factory=True, workers=WORKERS)(WORKERSenv, default 1). The boot-migration race is already handled bystore.run_migrations'pg_advisory_lock(Cloud-Run multi-instance), and the admin seed tolerates the boot race — so no new lock was needed.Test plan
run_blockingruns a sync fn with args and kwargs on a worker thread + propagates exceptions; the factory import-stringmcp_http:build_appresolves to a Starlette app.uv run dev.py check).Out of scope (per spec)
The query-subprocess event-loop block (known issue, parked ACE-028); a pooled/queued audit writer (the fresh-Store-per-call churn stays — follow-up); rate limiting (ACE-049, depends on this).
Spec: ACE-048