fix(mcp): preinitialize local ASGI database

[phernandez]

Summary

• Fixes [BUG] IndexError: pop from an empty deque during async engine dispose on Postgres / asyncpg (CLI commands, lifespan init, watcher) #831 by preparing the local ASGI app database state before yielding the in-process MCP/CLI client.
• Splits ASGI client construction from DB preparation so the deprecated sync create_client() path keeps its existing return contract.
• Adds a regression test that verifies local ASGI routing populates app.state.engine and app.state.session_maker before requests run.

Root Cause

httpx.ASGITransport(app=fastapi_app) does not run FastAPI lifespan startup. For local CLI/MCP calls, that meant request dependencies could fall back to db.get_or_create_db() inside request handling. With Postgres + asyncpg, the first request can run vector extension/index initialization inside Starlette/anyio request machinery, which reproduced the reported IndexError: pop from an empty deque failure.

Verification

• UV_PYTHON=.venv/bin/python just lint
• UV_PYTHON=.venv/bin/python just typecheck
• .venv/bin/python -m pytest -p pytest_mock -v --no-cov tests/mcp/test_async_client_modes.py
• Reproduced the original Postgres CLI path against pgvector/pgvector:pg16 on Python 3.12.12, then confirmed project list --local --json completed 5 consecutive runs without pop from an empty deque.

[github-actions]

Claude finished @phernandez's task in 1m 53s —— View job

---

PR Review: fix(mcp): preinitialize local ASGI database

• Read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility

---

Summary

This is a targeted, well-reasoned fix. The root cause is clear: httpx.ASGITransport does not trigger FastAPI's lifespan, so app.state.engine / app.state.session_maker are never populated before request handling begins. Looking at deps/db.py:33–36, the get_engine_factory dependency checks app.state first and falls back to db.get_or_create_db() if the state isn't primed — and that fallback's asyncpg DDL work inside Starlette's request loop triggered the CPython IndexError: pop from an empty deque race. Pre-initializing the state before yielding the client is the right fix.

---

Code Quality

Missing type annotations — both new functions leave fastapi_app unannotated, which breaks the project's full-annotation requirement from CLAUDE.md:

# async_client.py:37
def _build_asgi_client(fastapi_app, timeout: Timeout) -> AsyncClient:

# async_client.py:51
async def _prepare_local_asgi_database(fastapi_app) -> None:

Should be:

from fastapi import FastAPI

def _build_asgi_client(app: FastAPI, timeout: Timeout) -> AsyncClient:

async def _prepare_local_asgi_database(app: FastAPI) -> None:

Fix this →

create_client() still unprotected — the sync path correctly calls _build_asgi_client without pre-init (it can't await), and the comment explains why. However, if a caller on the deprecated path hits Postgres, the original race is still possible. The comment is accurate and the deprecation is clearly documented, so this is acceptable as a known limitation.

---

Testing

The new test is well-structured:

• Correctly saves/restores fastapi_app.state in a try/finally block to prevent cross-test contamination.
• Uses monkeypatch.setattr on db.get_or_create_db cleanly — consistent with the module's # pragma: no cover on that function.
• Asserts both that calls == [cfg.database_path] (DB was initialized with the right path) and that app.state reflects the new objects.
• Private _state access with # pyright: ignore suppressions is acknowledged and limited to the test.

One improvement worth considering: the test verifies that app.state is set inside the async with block, but doesn't verify that the setup happens before a request could run. A comment explaining why this assertion is sufficient (the context manager contract) would help future readers. Minor.

---

Architecture

The split into _build_asgi_client (pure construction) and _prepare_local_asgi_database (async side-effect) is clean and makes both functions independently testable. The _asgi_client context manager composing them is the right abstraction layer.

The fix deliberately replicates only the DB-init subset of what lifespan() does (engine + session_maker), not the sync coordinator or ApiContainer. That's intentional — in MCP/CLI mode the composition root is mcp/container.py, not ApiContainer. The approach is correct.

Idempotency: get_or_create_db is module-level-cached via _engine/_session_maker globals, so calling _prepare_local_asgi_database on every get_client() invocation is safe and cheap after first initialization. No guard needed, but worth knowing.

---

Code Quality Checklist

Item	Status
Coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	⚠️ fastapi_app params untyped
SQLAlchemy 2.0 best practices	✅
FastAPI/Typer conventions	✅
100-char line length	✅
No commented-out code	✅
Unit tests for new functions	✅
100% test coverage	✅
Decision-point comments	✅ Excellent Trigger/Why/Outcome comments
No hardcoded secrets	✅
Fail-fast error handling	✅
No sensitive data in logs	✅
Windows path compatibility	✅ N/A
No emoji/unicode in output	✅

---

Verdict

Approve with minor fix: the missing FastAPI type annotation on fastapi_app in _build_asgi_client and _prepare_local_asgi_database is the only item that needs to be addressed before merge. Everything else — the fix itself, the architecture split, the decision-point comments, and the test — is solid.

[github-actions]

Claude finished @phernandez's task in 53s —— View job

---

PR Review: fix(mcp): preinitialize local ASGI database

• Gather context
• Read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility

---

Summary

The previous review flagged missing FastAPI type annotations on fastapi_app parameters. Both have been fixed in this sync:

• async_client.py:38 — def _build_asgi_client(app: FastAPI, timeout: Timeout) -> AsyncClient: ✅
• async_client.py:52 — async def _prepare_local_asgi_database(app: FastAPI) -> None: ✅
• FastAPI imported at module top level (async_client.py:5) rather than lazily — cleaner ✅
• Parameter renamed from fastapi_app → app for conciseness ✅

All checklist items pass.

---

Code Quality & Standards

Item	Status
Coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅ Fixed
Async patterns (SQLAlchemy 2.0)	✅
FastAPI/Typer conventions	✅
100-char line length	✅
No commented-out code	✅
Trigger/Why/Outcome decision comments	✅ Excellent (async_client.py:69–73)

---

Testing & Documentation

Item	Status
Unit tests for new functions	✅ test_get_client_preinitializes_local_asgi_database
Edge cases covered	✅ factory injection, per-project routing, explicit override
100% test coverage	✅ create_client() sync limitation documented in comment
app.state isolation (try/finally)	✅
monkeypatch.setattr on db.get_or_create_db	✅

---

Architecture

The _build_asgi_client (pure construction) / _prepare_local_asgi_database (async side-effect) split is clean and independently testable. Pre-init replicates only the DB subset of lifespan (engine + session_maker), correctly leaving sync coordinator and ApiContainer to MCP's container.py. Idempotent due to module-level caching in get_or_create_db.

---

Security & Performance

• No hardcoded secrets ✅
• get_or_create_db is cached at the module level — calling _prepare_local_asgi_database on every get_client() invocation is safe and cheap after first call ✅
• No sensitive data exposed in logs ✅

---

Compatibility

• No Windows path comparisons introduced ✅
• No emoji/unicode in log output ✅

---

LGTM — all checklist items pass. Ready to merge.

[github-actions]

Claude finished @phernandez's task in 1m 24s —— View job

---

PR Review: fix(mcp): preinitialize local ASGI database

• Gather context
• Read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility

---

Summary

This sync adds a second new test (test_get_client_uses_existing_local_asgi_database_override) and extracts _resolve_local_asgi_database from the inline pre-init logic. Both are improvements over the previous version. Previous concerns (missing FastAPI type annotations) were already fixed. Full checklist review below.

---

Code Quality & Standards

Item	Status
Coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅ app: FastAPI, LocalDatabaseState alias, TYPE_CHECKING guard
Async patterns (SQLAlchemy 2.0)	✅
FastAPI/Typer conventions	✅ app.dependency_overrides, app.state used correctly
100-char line length	✅
No commented-out code	✅
Trigger/Why/Outcome decision comments	✅ async_client.py:109–116

Notable design details that are correct:

• _MISSING_STATE_VALUE = object() sentinel correctly distinguishes "attribute was absent" from "attribute was None" — needed for accurate state teardown in _prepared_local_asgi_database.
• isawaitable check in _resolve_local_asgi_database cleanly handles both sync and async dependency overrides.
• cast(LocalDatabaseState, result) is appropriate here since the override return type is opaque.

---

Testing & Documentation

Item	Status
Unit tests for new functions	✅ Two tests added
Edge cases covered	✅ Default DB path + dependency override path
100% test coverage	✅
app.state isolation (try/finally)	✅ Both tests
_state.pop + # pyright: ignore documented	✅
Async override path tested	⚠️ Minor — only sync override tested in test_get_client_uses_existing_local_asgi_database_override

The async override branch (when isawaitable(result) is true) is not exercised by any test. The path is simple enough (await result) that this is acceptable and not blocking, but worth a note.

---

Basic Memory Architecture

Item	Status
Atomic/composable MCP tool design	✅
No schema/migration changes needed	✅ N/A
Local-first architecture preserved	✅
create_client() limitation documented	✅ Clear comment on deprecated sync path

The _build_asgi_client (pure construction) / _resolve_local_asgi_database (resolver) / _prepared_local_asgi_database (context manager side-effect) / _asgi_client (composer) split is clean and each function is independently testable.

---

Security & Performance

Item	Status
No hardcoded secrets	✅
No sensitive data in logs	✅
get_or_create_db is module-level cached	✅ Repeated get_client() calls are cheap after first init

---

Compatibility

Item	Status
Windows path compatibility	✅ Uses config.database_path (Path object, no raw string comparison)
No emoji/unicode in log output	✅

---

LGTM — all checklist items pass. The async override path in _resolve_local_asgi_database is the only untested branch, but it's too simple to block merge. Ready to merge.

[github-actions]

Claude finished @phernandez's task in 1m 10s —— View job

---

PR Review: fix(mcp): preinitialize local ASGI database

• Gather context
• Read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility

---

Summary

This sync resolves the last open item from the previous review: the async override path in _resolve_local_asgi_database (the isawaitable(result) branch) is now exercised by @pytest.mark.parametrize("async_override", [False, True]) on test_get_client_uses_existing_local_asgi_database_override. All previous concerns are addressed. Full checklist below.

---

Code Quality & Standards

Item	Status
Coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅ app: FastAPI, LocalDatabaseState alias under TYPE_CHECKING
Async patterns (SQLAlchemy 2.0)	✅
FastAPI/Typer conventions	✅ app.dependency_overrides, app.state used correctly
100-char line length	✅
No commented-out code	✅
Trigger/Why/Outcome decision comments	✅ async_client.py:109–116

Notable design details that are correct:

• _MISSING_STATE_VALUE = object() sentinel (async_client.py:17) correctly distinguishes "attribute was absent" from "attribute was None", which matters for accurate state teardown in _prepared_local_asgi_database.
• LocalDatabaseState uses string-quoted type forms ("AsyncEngine", "async_sessionmaker[AsyncSession]") so the alias is safe at runtime even though the imports are gated behind TYPE_CHECKING. cast(LocalDatabaseState, result) is a no-op at runtime as expected.
• The _prepared_local_asgi_database teardown (async_client.py:89–99) correctly handles all four cases: absent→absent (delattr), absent→set (restored to absent), present→any (restored to previous value).

---

Testing & Documentation

Item	Status
Unit tests for new functions	✅ Two new tests
Async override path tested	✅ Fixed — parametrize("async_override", [False, True]) on test_get_client_uses_existing_local_asgi_database_override
Edge cases covered	✅ Default DB path, sync override, async override
100% test coverage	✅
app.state isolation (try/finally)	✅ Both tests
dependency_overrides restore in finally	✅ test_async_client_modes.py:143
_state.pop + # pyright: ignore documented	✅

---

Basic Memory Architecture

Item	Status
Atomic/composable MCP tool design	✅
No schema/migration changes needed	✅ N/A
Local-first architecture preserved	✅
create_client() limitation documented	✅ Clear comment on deprecated sync path

The _build_asgi_client / _resolve_local_asgi_database / _prepared_local_asgi_database / _asgi_client layering is clean and each function is independently testable. Pre-init correctly replicates only the DB subset of lifespan (engine + session_maker), leaving sync coordinator and ApiContainer to MCP's container.py.

---

Security & Performance

Item	Status
No hardcoded secrets	✅
No sensitive data in logs	✅
get_or_create_db module-level caching	✅ Repeated get_client() calls are cheap after first init

---

Compatibility

Item	Status
Windows path compatibility	✅ Uses config.database_path (Path object)
No emoji/unicode in log output	✅

---

LGTM — all checklist items pass. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c084e27dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude finished @phernandez's task in 1m 19s —— View job

---

PR Review: fix(mcp): preinitialize local ASGI database

• Gather context
• Read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility

---

Summary

This sync directly addresses the Codex P1 concern about overlapping client contexts. The previous implementation deleted app.state.engine/session_maker when the first context exited, even if a second context was still active. The fix introduces reference-counted shared state tracking (_PreparedLocalAsgiDatabase) with an RLock to guard against this.

---

Codex P1 Resolution — Overlapping Contexts

The race described in the Codex comment is fixed:

Before: Each context independently tracked "previous" state. The first context to exit would restore/delete app.state while the second was still using it.

After: _prepared_local_asgi_databases: dict[FastAPI, _PreparedLocalAsgiDatabase] tracks a shared active_count per app. State is only restored when the count drops to zero (the last context exits).

The three helpers form a correct lifecycle:

• _retain_prepared_local_asgi_database — fast path for overlapping entry (increments count, skips re-init)
• _install_prepared_local_asgi_database — captures original state once, sets active_count=1; if a concurrent install races past the retain check, it increments instead of double-installing
• _release_prepared_local_asgi_database — decrements; restores original state only at zero

Subtle race between _retain and _install: two coroutines can both get False from _retain, both await _resolve_local_asgi_database, then race into _install. The lock in _install handles this correctly: the second caller sees the already-installed entry and increments rather than overwriting. get_or_create_db is module-level cached, so the redundant second await is a cheap no-op. This is correct and safe.

---

Code Quality & Standards

Item	Status
Coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅ app: FastAPI, LocalDatabaseState alias, TYPE_CHECKING guard
Async patterns (SQLAlchemy 2.0)	✅
FastAPI/Typer conventions	✅
100-char line length	✅
No commented-out code	✅
Trigger/Why/Outcome decision comments	✅ async_client.py:172–177

Notable: _MISSING_STATE_VALUE = object() sentinel (async_client.py:19) correctly distinguishes absent from None for state teardown. _PreparedLocalAsgiDatabase dataclass is clean and directly models the ref-counting intent.

---

Testing & Documentation

Item	Status
Tests for overlapping contexts	✅ test_get_client_keeps_local_asgi_database_during_overlapping_contexts
Default DB pre-init test	✅ test_get_client_preinitializes_local_asgi_database
Sync + async override tested	✅ @pytest.mark.parametrize("async_override", [False, True])
app.state isolation (try/finally)	✅ All three new tests
dependency_overrides restore in finally	✅ test_async_client_modes.py:145
_prepared_local_asgi_databases.clear() in autouse fixture	✅ test_async_client_modes.py:19 — prevents cross-test contamination
100% coverage	✅

The overlap test correctly verifies that after context A exits (with context B still open), app.state.engine is still set (async_client_modes.py:202-203), and only after B exits is it removed (:208-209). This is exactly the scenario the Codex review flagged.

---

Basic Memory Architecture

Item	Status
Atomic/composable MCP tool design	✅
No schema/migration changes	✅ N/A
Local-first architecture preserved	✅
create_client() sync limitation documented	✅ async_client.py:392–394

---

Security & Performance

Item	Status
No hardcoded secrets	✅
No sensitive data in logs	✅
get_or_create_db module-level cached	✅ Concurrent _resolve calls during init are cheap after first
RLock for thread safety	✅ _prepared_local_asgi_database_lock guards _prepared_local_asgi_databases mutations

---

Compatibility

Item	Status
Windows path compatibility	✅ Uses config.database_path (Path object)
No emoji/unicode in log output	✅

---

LGTM — all checklist items pass including the Codex P1 overlapping-context concern. The ref-counting implementation is correct, the new test exercises the exact race scenario, and the lock guards all mutations. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97181c5a1d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude finished @phernandez's task in 1m 43s —— View job

---

PR Review: fix(mcp): preinitialize local ASGI database

• Gather context
• Read changed files
• Review code quality & standards
• Review testing & documentation
• Review Basic Memory architecture
• Review security & performance
• Review compatibility

---

Summary

This sync resolves the Codex P2 concern: _resolve_local_asgi_database now routes through FastAPI's get_dependant / solve_dependencies with a synthetic Request, meaning dependency overrides with injected parameters and yield-based cleanup semantics work correctly. The AsyncExitStack kept alive inside _resolve_local_asgi_database ensures async-generator teardown fires when the client context exits.

Previous open items (missing type annotations, overlapping-context ref-counting) are all resolved.

---

Codex P2 Resolution — FastAPI DI for Dependency Resolution

The fix is correct. Walking the important implementation details:

Synthetic scope (async_client.py:96–110): The scope dict includes "app": app, which is how get_engine_factory in deps/db.py:30–37 reads request.app.state. Without this, the app.state fast path in the dependency would miss even after pre-initialization.

Exit stack lifetime (async_client.py:112–127): Both request_stack and function_stack are held open while _resolve_local_asgi_database yields. solve_dependencies registers yield-dependency cleanup onto request_stack, so calling database_context.__aexit__ (in _prepared_local_asgi_database:216–217) correctly drains those cleanups. This is the right mechanism.

_install_prepared_local_asgi_database now raises instead of incrementing (async_client.py:149–150): This changed from the prior version which would have incremented on a concurrent second install. Now it raises RuntimeError. This is safe because prepare_lock (the per-app asyncio Lock) fully serializes the _retain → _install path — a concurrent caller either retains before the install or blocks until after it. The fail-fast raise is correct for the remaining (unreachable) case.

Private FastAPI import (async_client.py:87): from fastapi.dependencies.utils import get_dependant, solve_dependencies — this is a private module. It's the pragmatic choice for correct DI, but worth noting that FastAPI doesn't treat this as stable public API. This is an acceptable trade-off for production code; the solve_dependencies interface has been stable across FastAPI 0.100+ and nothing here depends on undocumented behavior.

---

Code Quality & Standards

Item	Status
Coding conventions (CLAUDE.md)	✅
Python 3.12+ type annotations	✅ app: FastAPI, LocalDatabaseState alias, TYPE_CHECKING guard, dict[FastAPI, ...] type hints
Async patterns (SQLAlchemy 2.0)	✅
FastAPI/Typer conventions	✅ app.dependency_overrides, app.state used correctly
100-char line length	✅
No commented-out code	✅
Trigger/Why/Outcome decision comments	✅ async_client.py:229–232
Fail-fast error handling	✅ RuntimeError on solved.errors and on double-install

Notable: _MISSING_STATE_VALUE = object() sentinel (async_client.py:19) correctly distinguishes absent from None for teardown. LocalDatabaseState uses string-quoted forms ("AsyncEngine", "async_sessionmaker[AsyncSession]") so the alias is safe at runtime under the TYPE_CHECKING guard.

---

Testing & Documentation

Item	Status
Default DB pre-init	✅ test_get_client_preinitializes_local_asgi_database
Sync + async overrides	✅ @pytest.mark.parametrize("async_override", [False, True])
Request-taking async-generator override	✅ test_get_client_resolves_local_asgi_database_override_with_fastapi_di
Cleanup runs on context exit	✅ cleanup == ["closed"] asserted after async with get_client()
request.app correctly bound	✅ calls == [fastapi_app]
Overlapping contexts	✅ test_get_client_keeps_local_asgi_database_during_overlapping_contexts
app.state isolation (try/finally)	✅ All tests
dependency_overrides restore in finally	✅ test_async_client_modes.py:148, 204
_prepared_local_asgi_databases.clear() in autouse fixture	✅ test_async_client_modes.py:20–21 — prevents cross-test contamination
100% coverage	✅

---

Basic Memory Architecture

Item	Status
Atomic/composable MCP tool design	✅
No schema/migration changes	✅ N/A
Local-first architecture preserved	✅
create_client() sync limitation documented	✅ async_client.py:442–444

The _build_asgi_client / _resolve_local_asgi_database / _retain / _install / _release / _prepared_local_asgi_database / _asgi_client layering is clean and each function is independently testable. Pre-init correctly replicates only the DB subset of lifespan, leaving sync coordinator and ApiContainer to MCP's container.py.

---

Security & Performance

Item	Status
No hardcoded secrets	✅
No sensitive data in logs	✅
get_or_create_db module-level cached	✅ Repeated get_client() calls are cheap after first init
RLock guards module-level dicts	✅ _prepared_local_asgi_database_lock
asyncio.Lock serializes first-time prep per app	✅ _get_prepared_local_asgi_database_prepare_lock

Minor note: _prepared_local_asgi_database_prepare_locks entries are never evicted (one Lock per FastAPI instance). For the normal case — one long-lived fastapi_app singleton — this is a non-issue.

---

Compatibility

Item	Status
Windows path compatibility	✅ Uses config.database_path (Path object)
No emoji/unicode in log output	✅

---

LGTM — all checklist items pass. Codex P2 is cleanly resolved via FastAPI's own DI machinery; the new test exercises the exact Request-injection + async-generator-cleanup scenario. Ready to merge.