diff --git a/src/basic_memory/alembic/env.py b/src/basic_memory/alembic/env.py index 153adb0ce..4b9bb4b2e 100644 --- a/src/basic_memory/alembic/env.py +++ b/src/basic_memory/alembic/env.py @@ -2,37 +2,34 @@ import asyncio import os +import sys from contextlib import suppress from logging.config import fileConfig from loguru import logger +from sqlalchemy import engine_from_config, pool +from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine -# Allow nested event loops (needed for pytest-asyncio and other async contexts) -# Note: nest_asyncio doesn't work with uvloop or Python 3.14+, so we handle those cases separately -import sys +from alembic import context + +from basic_memory.config import ConfigManager +from basic_memory.migration_loop import is_running_loop_error, running_on_uvloop -if sys.version_info < (3, 14): +# Allow nested event loops (needed for pytest-asyncio and other async contexts). +# nest_asyncio cannot patch a uvloop loop or Python 3.14+; in those cases we skip +# it and rely on the thread-based fallback in run_migrations_online() instead +# (see basic_memory.migration_loop for why uvloop must be detected up front). +if sys.version_info < (3, 14) and not running_on_uvloop(): try: import nest_asyncio nest_asyncio.apply() except (ImportError, ValueError) as exc: # Trigger: nest_asyncio is absent (ImportError) or refuses to patch the - # running loop (ValueError, e.g. the uvloop policy installed for the - # Postgres backend - #831/#877). - # Why: the uvloop ValueError is now an *expected* path on every Postgres - # startup, so swallowing it silently hides a routine branch. + # running loop (ValueError). # Outcome: log at DEBUG (observable, not noisy) and fall through to the # thread-based migration fallback. logger.debug(f"nest_asyncio not applied ({exc!r}); using thread-based migration fallback") -# For Python 3.14+, we rely on the thread-based fallback in run_migrations_online() - -from sqlalchemy import engine_from_config, pool -from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine - -from alembic import context - -from basic_memory.config import ConfigManager # Trigger: only set test env when actually running under pytest # Why: alembic/env.py is imported during normal operations (MCP server startup, migrations) @@ -175,7 +172,7 @@ def _run_async_engine_migrations(connectable) -> None: try: _run_async_migrations_with_asyncio_run(connectable) except RuntimeError as e: - if "cannot be called from a running event loop" in str(e): + if is_running_loop_error(e): # We're in a running event loop (likely uvloop or Python 3.14+ tests). # Switch to a dedicated thread so Alembic can finish without nesting loops. _run_async_migrations_in_thread(connectable) diff --git a/src/basic_memory/config.py b/src/basic_memory/config.py index 3da68d183..14f74d8a9 100644 --- a/src/basic_memory/config.py +++ b/src/basic_memory/config.py @@ -648,6 +648,30 @@ def is_test_env(self) -> bool: or os.getenv("PYTEST_CURRENT_TEST") is not None ) + @property + def cloud_mode(self) -> bool: + """Whether this process runs as a cloud deployment. + + In-repo cloud containers build BasicMemoryConfig via ConfigManager (not + for_cloud_tenant), so they signal cloud mode through the environment + rather than skip_initialization_sync. Mirrors the detection in setup_logging. + """ + return os.getenv("BASIC_MEMORY_CLOUD_MODE", "").lower() in ("1", "true") + + @property + def skip_local_initialization(self) -> bool: + """Whether to skip local project seeding / reconciliation / path creation. + + True for any cloud or stateless deployment: for_cloud_tenant sets + skip_initialization_sync, while in-repo cloud containers set + BASIC_MEMORY_CLOUD_MODE. A LOCAL Postgres install matches neither, so it + still initializes like SQLite. Gating these paths on the Postgres *backend* + caught local Postgres (wrong); gating only on skip_initialization_sync + missed BASIC_MEMORY_CLOUD_MODE deployments, letting reconcile delete tenant + project rows (also wrong). + """ + return self.skip_initialization_sync or self.cloud_mode + def get_project_mode(self, project_name: str) -> ProjectMode: """Get the routing mode for a project. @@ -735,9 +759,13 @@ def get_project_path(self, project_name: Optional[str] = None) -> Path: # pragm def model_post_init(self, __context: Any) -> None: """Ensure configuration is valid after initialization.""" - # Skip project initialization in cloud mode - projects are discovered from DB - if self.database_backend == DatabaseBackend.POSTGRES: # pragma: no cover - return # pragma: no cover + # Skip default-project seeding only for cloud/stateless deployments, where + # projects are discovered from the database per tenant. See + # skip_local_initialization for why this is not gated on the Postgres + # backend (caught local Postgres) nor on skip_initialization_sync alone + # (missed BASIC_MEMORY_CLOUD_MODE deployments). + if self.skip_local_initialization: + return # Trigger: no projects configured (fresh install or empty config) # Why: every config needs at least one project to be functional @@ -801,11 +829,14 @@ def project_list(self) -> List[ProjectConfig]: # pragma: no cover def ensure_project_paths_exists(self) -> "BasicMemoryConfig": # pragma: no cover """Ensure project paths exist. - Skips path creation when using Postgres backend (cloud mode) since - cloud tenants don't use local filesystem paths. + Skips path creation for cloud/stateless deployments, whose tenants don't + use local filesystem paths. A local Postgres install still needs its + project directories created like SQLite, so gate on + skip_local_initialization, not the backend — otherwise the seeded default's + directory is never created and the sync/watch path hits a non-existent + directory. """ - # Skip path creation for cloud mode - no local filesystem - if self.database_backend == DatabaseBackend.POSTGRES: + if self.skip_local_initialization: return self for name, entry in self.projects.items(): diff --git a/src/basic_memory/db.py b/src/basic_memory/db.py index 815005113..c2932f210 100644 --- a/src/basic_memory/db.py +++ b/src/basic_memory/db.py @@ -266,12 +266,28 @@ def _create_postgres_engine(db_url: str, config: BasicMemoryConfig) -> AsyncEngi Returns: Configured async engine for Postgres """ - # Use NullPool connection issues. - # Assume connection pooler like PgBouncer handles connection pooling. + # Connection pooling for direct (local / self-hosted) Postgres. + # + # Trigger: this is the default engine factory. The cloud overrides + # get_engine_factory with its own pooled engine (basic_memory_cloud + # tenant_engine_pool), so this path serves the LOCAL runtime — which has no + # PgBouncer in front of Postgres. + # Why: NullPool (a fresh connection per request) was assumed safe because a + # pooler would sit in front, but locally there is none. Under concurrent + # writes — plus each background materialization opening its own connection — + # that stormed max_connections and collapsed (p99 478s, 21% write failures at + # C=32; benchmarks/docs/write-load-benchmark.md). + # Outcome: a real pool bounds in-use connections to db_pool_size (+ + # db_pool_overflow under load) and recycles them (Neon scale-to-zero). + # statement_cache_size=0 stays so the engine also works behind a PgBouncer + # transaction-mode pooler if a user runs one. engine = create_async_engine( db_url, echo=False, - poolclass=NullPool, # No pooling - fresh connection per request + poolclass=AsyncAdaptedQueuePool, + pool_size=config.db_pool_size, + max_overflow=config.db_pool_overflow, + pool_recycle=config.db_pool_recycle, connect_args={ # Disable statement cache to avoid issues with prepared statements on reconnect "statement_cache_size": 0, @@ -286,7 +302,10 @@ def _create_postgres_engine(db_url: str, config: BasicMemoryConfig) -> AsyncEngi }, }, ) - logger.debug("Created Postgres engine with NullPool (no connection pooling)") + logger.debug( + "Created Postgres engine with QueuePool " + f"(pool_size={config.db_pool_size}, max_overflow={config.db_pool_overflow})" + ) return engine diff --git a/src/basic_memory/migration_loop.py b/src/basic_memory/migration_loop.py new file mode 100644 index 000000000..38e01a83e --- /dev/null +++ b/src/basic_memory/migration_loop.py @@ -0,0 +1,45 @@ +"""Pure helpers for how Alembic bridges async migrations across event loops. + +Extracted from ``alembic/env.py`` so these two decisions are unit-testable: +env.py runs migrations at import time and cannot be imported in a test, but the +loop-detection and error-classification logic is exactly what regresses, so it +lives here where it can be covered directly. +""" + +from __future__ import annotations + +import asyncio + + +def running_on_uvloop() -> bool: + """Return True when the active asyncio policy is uvloop. + + The Postgres backend installs the uvloop policy before the event loop starts + (#831/#877). nest_asyncio cannot patch a uvloop loop, but depending on the + nest_asyncio version ``apply()`` may NOT raise the ValueError we expect — it + can silently mis-patch the stdlib loop instead. The later ``asyncio.run()`` + then fails with "this event loop is already running" rather than the standard + "cannot be called from a running event loop", which the thread-based fallback + would not recognize, crashing startup. Detecting uvloop up front lets env.py + skip nest_asyncio entirely and stay on the standard running-loop error path. + """ + try: + import uvloop + except ImportError: + return False + return isinstance(asyncio.get_event_loop_policy(), uvloop.EventLoopPolicy) + + +def is_running_loop_error(exc: BaseException) -> bool: + """Whether an error means ``asyncio.run`` hit an already-running event loop. + + Two spellings reach the migration fallback: the stdlib "cannot be called + from a running event loop", and "this event loop is already running" when + nest_asyncio mis-patched a uvloop loop. Both mean the same thing — retry the + migration in a dedicated thread rather than re-raising. + """ + msg = str(exc) + return ( + "cannot be called from a running event loop" in msg + or "this event loop is already running" in msg + ) diff --git a/src/basic_memory/services/initialization.py b/src/basic_memory/services/initialization.py index 784b54deb..ddbabff21 100644 --- a/src/basic_memory/services/initialization.py +++ b/src/basic_memory/services/initialization.py @@ -13,7 +13,7 @@ from loguru import logger from basic_memory import db -from basic_memory.config import BasicMemoryConfig, DatabaseBackend +from basic_memory.config import BasicMemoryConfig from basic_memory.models import Project from basic_memory.repository import ( ProjectRepository, @@ -191,13 +191,18 @@ async def initialize_app( "permalinks will be written." ) - # Trigger: database backend is Postgres (cloud deployment) - # Why: cloud deployments manage their own projects and migrations via the cloud platform. - # The local MCP server always uses SQLite and needs initialization even when - # projects are configured for cloud routing. - # Outcome: skip initialization only for actual cloud Postgres deployments. - if app_config.database_backend == DatabaseBackend.POSTGRES: - logger.info("Skipping local initialization - Postgres backend manages its own schema") + # Trigger: cloud/stateless deployment (skip_local_initialization — either + # for_cloud_tenant's skip_initialization_sync or BASIC_MEMORY_CLOUD_MODE). + # Why: cloud manages its own schema and per-tenant projects from the database. + # Running reconcile_projects_with_config there would delete tenant project rows + # absent from local config. Gating on the Postgres *backend* was wrong (it + # caught a LOCAL Postgres install, which still needs the seeded default + # reconciled into a projects row, else /v2/projects/resolve rejects it). + # Outcome: skip only for actual cloud/stateless deployments. + if app_config.skip_local_initialization: + logger.info( + "Skipping local initialization - cloud/stateless deployment manages its own schema" + ) return logger.info("Initializing app...") @@ -216,13 +221,17 @@ def ensure_initialization(app_config: BasicMemoryConfig) -> None: This is a wrapper for the async initialize_app function that can be called from synchronous code like CLI entry points. - No-op if database backend is Postgres (cloud deployment manages its own schema). + No-op for cloud/stateless deployments (skip_local_initialization). A LOCAL + Postgres install still needs initialization, so gate on that, not the backend — + matching initialize_app. Args: app_config: The Basic Memory project configuration """ - if app_config.database_backend == DatabaseBackend.POSTGRES: - logger.info("Skipping local initialization - Postgres backend manages its own schema") + if app_config.skip_local_initialization: + logger.info( + "Skipping local initialization - cloud/stateless deployment manages its own schema" + ) return async def _init_and_cleanup(): diff --git a/tests/services/test_initialization_cloud_mode_branches.py b/tests/services/test_initialization_cloud_mode_branches.py index 99a3aa5c6..8d628ec2b 100644 --- a/tests/services/test_initialization_cloud_mode_branches.py +++ b/tests/services/test_initialization_cloud_mode_branches.py @@ -1,6 +1,7 @@ import pytest from basic_memory.config import DatabaseBackend +from basic_memory.services import initialization from basic_memory.services.initialization import ( ensure_initialization, initialize_app, @@ -9,16 +10,95 @@ @pytest.mark.asyncio -async def test_initialize_app_noop_in_postgres_backend(app_config): +async def test_initialize_app_noop_for_stateless_cloud(app_config): + # Stateless/cloud deployments (skip_initialization_sync) manage their own + # schema + per-tenant projects from the DB, so local init is a no-op. app_config.database_backend = DatabaseBackend.POSTGRES + app_config.skip_initialization_sync = True await initialize_app(app_config) -def test_ensure_initialization_noop_in_postgres_backend(app_config): +def test_ensure_initialization_noop_for_stateless_cloud(app_config): app_config.database_backend = DatabaseBackend.POSTGRES + app_config.skip_initialization_sync = True ensure_initialization(app_config) +@pytest.mark.asyncio +async def test_initialize_app_runs_for_local_postgres(app_config, monkeypatch): + """A LOCAL Postgres backend (not stateless) still initializes — migrate + + reconcile — so the seeded default project gets a row in the projects table. + + Gating the skip on the Postgres backend (instead of skip_initialization_sync) + left the seeded default in config only, so /v2/projects/resolve rejected it. + """ + app_config.database_backend = DatabaseBackend.POSTGRES + app_config.skip_initialization_sync = False + monkeypatch.delenv("BASIC_MEMORY_CLOUD_MODE", raising=False) + + calls: list[str] = [] + + async def fake_initialize_database(cfg): + calls.append("initialize_database") + + async def fake_reconcile(cfg): + calls.append("reconcile_projects_with_config") + + monkeypatch.setattr(initialization, "initialize_database", fake_initialize_database) + monkeypatch.setattr(initialization, "reconcile_projects_with_config", fake_reconcile) + + await initialize_app(app_config) + + assert calls == ["initialize_database", "reconcile_projects_with_config"] + + +@pytest.mark.asyncio +async def test_initialize_app_noop_in_cloud_mode(app_config, monkeypatch): + """BASIC_MEMORY_CLOUD_MODE deployments (Postgres, skip_initialization_sync=False) + must still skip — running reconcile_projects_with_config there would delete + tenant project rows absent from local config.""" + app_config.database_backend = DatabaseBackend.POSTGRES + app_config.skip_initialization_sync = False + monkeypatch.setenv("BASIC_MEMORY_CLOUD_MODE", "1") + + calls: list[str] = [] + + async def fake_initialize_database(cfg): + calls.append("initialize_database") + + async def fake_reconcile(cfg): + calls.append("reconcile_projects_with_config") + + monkeypatch.setattr(initialization, "initialize_database", fake_initialize_database) + monkeypatch.setattr(initialization, "reconcile_projects_with_config", fake_reconcile) + + await initialize_app(app_config) + + assert calls == [] + + +def test_ensure_initialization_runs_for_local_postgres(app_config, monkeypatch): + """The sync CLI entrypoint must initialize for local Postgres, not skip — it + runs initialize_app instead of returning early on the Postgres backend.""" + app_config.database_backend = DatabaseBackend.POSTGRES + app_config.skip_initialization_sync = False + + called: list[object] = [] + + async def fake_initialize_app(cfg): + called.append(cfg) + + async def fake_shutdown_db(): + pass + + monkeypatch.setattr(initialization, "initialize_app", fake_initialize_app) + monkeypatch.setattr(initialization.db, "shutdown_db", fake_shutdown_db) + + ensure_initialization(app_config) + + assert called == [app_config] + + @pytest.mark.asyncio async def test_initialize_file_sync_skips_in_test_env(app_config): # app_config fixture uses env="test" diff --git a/tests/test_config.py b/tests/test_config.py index 14516c781..b6bc34913 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -91,6 +91,69 @@ def test_model_post_init_fallback_without_basic_memory_home(self, config_home, m assert Path(config.projects["other"].path) == other_path assert config.default_project == "other" + def test_model_post_init_seeds_default_for_local_postgres(self, config_home, monkeypatch): + """A LOCAL Postgres backend still seeds a default project, like SQLite. + + The seeding skip is for stateless/cloud (skip_initialization_sync), not the + Postgres backend — otherwise a fresh local Postgres has no default project + and create_memory_project raises "No default project configured". + """ + monkeypatch.delenv("BASIC_MEMORY_HOME", raising=False) + monkeypatch.delenv("BASIC_MEMORY_CLOUD_MODE", raising=False) + + config = BasicMemoryConfig(database_backend="postgres") + + assert "main" in config.projects + assert config.default_project == "main" + + def test_model_post_init_skips_seeding_for_stateless_deployments( + self, config_home, monkeypatch + ): + """Stateless/cloud configs discover projects from the DB, so seed nothing.""" + monkeypatch.delenv("BASIC_MEMORY_HOME", raising=False) + + config = BasicMemoryConfig(database_backend="postgres", skip_initialization_sync=True) + + assert config.projects == {} + assert config.default_project is None + + def test_model_post_init_skips_seeding_in_cloud_mode(self, config_home, monkeypatch): + """BASIC_MEMORY_CLOUD_MODE deployments build config via ConfigManager, not + for_cloud_tenant, so skip_initialization_sync is false — they must still skip + seeding (and reconcile) so cloud startup can't delete tenant project rows.""" + monkeypatch.delenv("BASIC_MEMORY_HOME", raising=False) + monkeypatch.setenv("BASIC_MEMORY_CLOUD_MODE", "1") + + config = BasicMemoryConfig(database_backend="postgres", skip_initialization_sync=False) + + assert config.cloud_mode is True + assert config.skip_local_initialization is True + assert config.projects == {} + assert config.default_project is None + + def test_local_postgres_creates_project_directories(self, config_home, tmp_path): + """Local Postgres creates its project directories like SQLite — the + ensure_project_paths_exists skip is gated on skip_initialization_sync.""" + proj = tmp_path / "pg-project" + BasicMemoryConfig( + database_backend="postgres", + skip_initialization_sync=False, + projects={"main": {"path": str(proj)}}, + default_project="main", + ) + assert proj.exists() + + def test_stateless_postgres_skips_project_directories(self, config_home, tmp_path): + """Stateless/cloud deployments don't touch the local filesystem.""" + proj = tmp_path / "cloud-project" + BasicMemoryConfig( + database_backend="postgres", + skip_initialization_sync=True, + projects={"main": {"path": str(proj)}}, + default_project="main", + ) + assert not proj.exists() + def test_basic_memory_home_with_relative_path(self, config_home, monkeypatch): """Test that BASIC_MEMORY_HOME works with relative paths.""" relative_path = "relative/memory/path" diff --git a/tests/test_migration_loop.py b/tests/test_migration_loop.py new file mode 100644 index 000000000..668def3c4 --- /dev/null +++ b/tests/test_migration_loop.py @@ -0,0 +1,49 @@ +"""Unit tests for the Alembic migration loop helpers. + +These guard the exact decisions that broke local Postgres startup: nest_asyncio +mis-patching a uvloop loop, and the fallback failing to recognize the resulting +error. env.py itself can't be imported in a test (it runs migrations at import), +so the logic lives in basic_memory.migration_loop where it can be covered. +""" + +import asyncio +import sys + +import pytest + +from basic_memory import migration_loop + + +def test_is_running_loop_error_matches_stdlib_message(): + err = RuntimeError("asyncio.run() cannot be called from a running event loop") + assert migration_loop.is_running_loop_error(err) is True + + +def test_is_running_loop_error_matches_nest_asyncio_uvloop_message(): + # nest_asyncio mis-patched a uvloop loop -> different wording, same meaning. + err = RuntimeError("this event loop is already running") + assert migration_loop.is_running_loop_error(err) is True + + +def test_is_running_loop_error_rejects_unrelated_runtime_error(): + err = RuntimeError("migration failed: column already exists") + assert migration_loop.is_running_loop_error(err) is False + + +@pytest.mark.skipif(sys.platform == "win32", reason="uvloop is not available on Windows") +def test_running_on_uvloop_true_when_policy_is_uvloop(monkeypatch): + import uvloop + + monkeypatch.setattr(asyncio, "get_event_loop_policy", lambda: uvloop.EventLoopPolicy()) + assert migration_loop.running_on_uvloop() is True + + +def test_running_on_uvloop_false_for_default_policy(monkeypatch): + monkeypatch.setattr(asyncio, "get_event_loop_policy", lambda: asyncio.DefaultEventLoopPolicy()) + assert migration_loop.running_on_uvloop() is False + + +def test_running_on_uvloop_false_when_uvloop_unimportable(monkeypatch): + # Simulate uvloop not installed (e.g. Windows): import raises -> False. + monkeypatch.setitem(sys.modules, "uvloop", None) + assert migration_loop.running_on_uvloop() is False