Skip to content

Commit 078086b

Browse files
phernandezclaude
andcommitted
fix: eliminate redundant database migration initialization
- Add global migration state tracking to prevent duplicate migrations - Make run_migrations() idempotent with completion state checking - Update get_or_create_db() to handle migrations centrally - Remove redundant migration calls from initialization services - Add comprehensive unit tests for migration deduplication This resolves the issue where MCP startup triggered 3x database initialization sequences, which could interfere with tool registration and cause "Method not found" errors in MCP clients. Fixes: #142 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent b4c26a6 commit 078086b

4 files changed

Lines changed: 256 additions & 28 deletions

File tree

src/basic_memory/db.py

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
# Module level state
2424
_engine: Optional[AsyncEngine] = None
2525
_session_maker: Optional[async_sessionmaker[AsyncSession]] = None
26+
_migrations_completed: bool = False
2627

2728

2829
class DatabaseType(Enum):
@@ -72,18 +73,35 @@ async def scoped_session(
7273
await factory.remove()
7374

7475

76+
def _create_engine_and_session(
77+
db_path: Path, db_type: DatabaseType = DatabaseType.FILESYSTEM
78+
) -> tuple[AsyncEngine, async_sessionmaker[AsyncSession]]:
79+
"""Internal helper to create engine and session maker."""
80+
db_url = DatabaseType.get_db_url(db_path, db_type)
81+
logger.debug(f"Creating engine for db_url: {db_url}")
82+
engine = create_async_engine(db_url, connect_args={"check_same_thread": False})
83+
session_maker = async_sessionmaker(engine, expire_on_commit=False)
84+
return engine, session_maker
85+
86+
7587
async def get_or_create_db(
7688
db_path: Path,
7789
db_type: DatabaseType = DatabaseType.FILESYSTEM,
90+
ensure_migrations: bool = True,
91+
app_config: Optional["BasicMemoryConfig"] = None,
7892
) -> tuple[AsyncEngine, async_sessionmaker[AsyncSession]]: # pragma: no cover
7993
"""Get or create database engine and session maker."""
8094
global _engine, _session_maker
8195

8296
if _engine is None:
83-
db_url = DatabaseType.get_db_url(db_path, db_type)
84-
logger.debug(f"Creating engine for db_url: {db_url}")
85-
_engine = create_async_engine(db_url, connect_args={"check_same_thread": False})
86-
_session_maker = async_sessionmaker(_engine, expire_on_commit=False)
97+
_engine, _session_maker = _create_engine_and_session(db_path, db_type)
98+
99+
# Run migrations automatically unless explicitly disabled
100+
if ensure_migrations:
101+
if app_config is None:
102+
from basic_memory.config import get_basic_memory_config
103+
app_config = get_basic_memory_config()
104+
await run_migrations(app_config, db_type)
87105

88106
# These checks should never fail since we just created the engine and session maker
89107
# if they were None, but we'll check anyway for the type checker
@@ -100,12 +118,13 @@ async def get_or_create_db(
100118

101119
async def shutdown_db() -> None: # pragma: no cover
102120
"""Clean up database connections."""
103-
global _engine, _session_maker
121+
global _engine, _session_maker, _migrations_completed
104122

105123
if _engine:
106124
await _engine.dispose()
107125
_engine = None
108126
_session_maker = None
127+
_migrations_completed = False
109128

110129

111130
@asynccontextmanager
@@ -119,7 +138,7 @@ async def engine_session_factory(
119138
for each test. For production use, use get_or_create_db() instead.
120139
"""
121140

122-
global _engine, _session_maker
141+
global _engine, _session_maker, _migrations_completed
123142

124143
db_url = DatabaseType.get_db_url(db_path, db_type)
125144
logger.debug(f"Creating engine for db_url: {db_url}")
@@ -143,12 +162,20 @@ async def engine_session_factory(
143162
await _engine.dispose()
144163
_engine = None
145164
_session_maker = None
165+
_migrations_completed = False
146166

147167

148168
async def run_migrations(
149-
app_config: BasicMemoryConfig, database_type=DatabaseType.FILESYSTEM
169+
app_config: BasicMemoryConfig, database_type=DatabaseType.FILESYSTEM, force: bool = False
150170
): # pragma: no cover
151171
"""Run any pending alembic migrations."""
172+
global _migrations_completed
173+
174+
# Skip if migrations already completed unless forced
175+
if _migrations_completed and not force:
176+
logger.debug("Migrations already completed in this session, skipping")
177+
return
178+
152179
logger.info("Running database migrations...")
153180
try:
154181
# Get the absolute path to the alembic directory relative to this file
@@ -170,11 +197,18 @@ async def run_migrations(
170197
command.upgrade(config, "head")
171198
logger.info("Migrations completed successfully")
172199

173-
_, session_maker = await get_or_create_db(app_config.database_path, database_type)
200+
# Get session maker - ensure we don't trigger recursive migration calls
201+
if _session_maker is None:
202+
_, session_maker = _create_engine_and_session(app_config.database_path, database_type)
203+
else:
204+
session_maker = _session_maker
174205

175206
# initialize the search Index schema
176207
# the project_id is not used for init_search_index, so we pass a dummy value
177208
await SearchRepository(session_maker, 1).init_search_index()
209+
210+
# Mark migrations as completed
211+
_migrations_completed = True
178212
except Exception as e: # pragma: no cover
179213
logger.error(f"Error running migrations: {e}")
180214
raise

src/basic_memory/services/initialization.py

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,21 @@
1717

1818

1919
async def initialize_database(app_config: BasicMemoryConfig) -> None:
20-
"""Run database migrations to ensure schema is up to date.
20+
"""Initialize database with migrations handled automatically by get_or_create_db.
2121
2222
Args:
2323
app_config: The Basic Memory project configuration
24+
25+
Note:
26+
Database migrations are now handled automatically when the database
27+
connection is first established via get_or_create_db().
2428
"""
29+
# Trigger database initialization and migrations by getting the database connection
2530
try:
26-
logger.info("Running database migrations...")
27-
await db.run_migrations(app_config)
28-
logger.info("Migrations completed successfully")
31+
await db.get_or_create_db(app_config.database_path)
32+
logger.info("Database initialization completed")
2933
except Exception as e:
30-
logger.error(f"Error running migrations: {e}")
34+
logger.error(f"Error initializing database: {e}")
3135
# Allow application to continue - it might still work
3236
# depending on what the error was, and will fail with a
3337
# more specific error if the database is actually unusable
@@ -44,9 +48,9 @@ async def reconcile_projects_with_config(app_config: BasicMemoryConfig):
4448
"""
4549
logger.info("Reconciling projects from config with database...")
4650

47-
# Get database session
51+
# Get database session - migrations handled centrally
4852
_, session_maker = await db.get_or_create_db(
49-
db_path=app_config.database_path, db_type=db.DatabaseType.FILESYSTEM
53+
db_path=app_config.database_path, db_type=db.DatabaseType.FILESYSTEM, ensure_migrations=False
5054
)
5155
project_repository = ProjectRepository(session_maker)
5256

@@ -65,9 +69,9 @@ async def reconcile_projects_with_config(app_config: BasicMemoryConfig):
6569

6670

6771
async def migrate_legacy_projects(app_config: BasicMemoryConfig):
68-
# Get database session
72+
# Get database session - migrations handled centrally
6973
_, session_maker = await db.get_or_create_db(
70-
db_path=app_config.database_path, db_type=db.DatabaseType.FILESYSTEM
74+
db_path=app_config.database_path, db_type=db.DatabaseType.FILESYSTEM, ensure_migrations=False
7175
)
7276
logger.info("Migrating legacy projects...")
7377
project_repository = ProjectRepository(session_maker)
@@ -134,9 +138,9 @@ async def initialize_file_sync(
134138
# delay import
135139
from basic_memory.sync import WatchService
136140

137-
# Load app configuration
141+
# Load app configuration - migrations handled centrally
138142
_, session_maker = await db.get_or_create_db(
139-
db_path=app_config.database_path, db_type=db.DatabaseType.FILESYSTEM
143+
db_path=app_config.database_path, db_type=db.DatabaseType.FILESYSTEM, ensure_migrations=False
140144
)
141145
project_repository = ProjectRepository(session_maker)
142146

tests/services/test_initialization.py

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,21 @@
1717

1818

1919
@pytest.mark.asyncio
20-
@patch("basic_memory.services.initialization.db.run_migrations")
21-
async def test_initialize_database(mock_run_migrations, project_config):
20+
@patch("basic_memory.services.initialization.db.get_or_create_db")
21+
async def test_initialize_database(mock_get_or_create_db, app_config):
2222
"""Test initializing the database."""
23-
await initialize_database(project_config)
24-
mock_run_migrations.assert_called_once_with(project_config)
23+
mock_get_or_create_db.return_value = (MagicMock(), MagicMock())
24+
await initialize_database(app_config)
25+
mock_get_or_create_db.assert_called_once_with(app_config.database_path)
2526

2627

2728
@pytest.mark.asyncio
28-
@patch("basic_memory.services.initialization.db.run_migrations")
29-
async def test_initialize_database_error(mock_run_migrations, project_config):
29+
@patch("basic_memory.services.initialization.db.get_or_create_db")
30+
async def test_initialize_database_error(mock_get_or_create_db, app_config):
3031
"""Test handling errors during database initialization."""
31-
mock_run_migrations.side_effect = Exception("Test error")
32-
await initialize_database(project_config)
33-
mock_run_migrations.assert_called_once_with(project_config)
32+
mock_get_or_create_db.side_effect = Exception("Test error")
33+
await initialize_database(app_config)
34+
mock_get_or_create_db.assert_called_once_with(app_config.database_path)
3435

3536

3637
@pytest.mark.asyncio
Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
"""Tests for database migration deduplication functionality."""
2+
3+
import pytest
4+
from pathlib import Path
5+
from unittest.mock import patch, AsyncMock, call, MagicMock
6+
7+
from basic_memory.config import BasicMemoryConfig
8+
from basic_memory import db
9+
10+
11+
@pytest.fixture
12+
def mock_alembic_config():
13+
"""Mock Alembic config to avoid actual migration runs."""
14+
with patch("basic_memory.db.Config") as mock_config_class:
15+
mock_config = MagicMock()
16+
mock_config_class.return_value = mock_config
17+
yield mock_config
18+
19+
20+
@pytest.fixture
21+
def mock_alembic_command():
22+
"""Mock Alembic command to avoid actual migration runs."""
23+
with patch("basic_memory.db.command") as mock_command:
24+
yield mock_command
25+
26+
27+
@pytest.fixture
28+
def mock_search_repository():
29+
"""Mock SearchRepository to avoid database dependencies."""
30+
with patch("basic_memory.db.SearchRepository") as mock_repo_class:
31+
mock_repo = AsyncMock()
32+
mock_repo_class.return_value = mock_repo
33+
yield mock_repo
34+
35+
36+
# Use the app_config fixture from conftest.py
37+
38+
39+
@pytest.mark.asyncio
40+
async def test_migration_deduplication_single_call(
41+
app_config, mock_alembic_config, mock_alembic_command, mock_search_repository
42+
):
43+
"""Test that migrations are only run once when called multiple times."""
44+
# Reset module state
45+
db._migrations_completed = False
46+
db._engine = None
47+
db._session_maker = None
48+
49+
# First call should run migrations
50+
await db.run_migrations(app_config)
51+
52+
# Verify migrations were called
53+
mock_alembic_command.upgrade.assert_called_once_with(mock_alembic_config, "head")
54+
mock_search_repository.init_search_index.assert_called_once()
55+
56+
# Reset mocks for second call
57+
mock_alembic_command.reset_mock()
58+
mock_search_repository.reset_mock()
59+
60+
# Second call should skip migrations
61+
await db.run_migrations(app_config)
62+
63+
# Verify migrations were NOT called again
64+
mock_alembic_command.upgrade.assert_not_called()
65+
mock_search_repository.init_search_index.assert_not_called()
66+
67+
68+
@pytest.mark.asyncio
69+
async def test_migration_force_parameter(
70+
app_config, mock_alembic_config, mock_alembic_command, mock_search_repository
71+
):
72+
"""Test that migrations can be forced to run even if already completed."""
73+
# Reset module state
74+
db._migrations_completed = False
75+
db._engine = None
76+
db._session_maker = None
77+
78+
# First call should run migrations
79+
await db.run_migrations(app_config)
80+
81+
# Verify migrations were called
82+
mock_alembic_command.upgrade.assert_called_once_with(mock_alembic_config, "head")
83+
mock_search_repository.init_search_index.assert_called_once()
84+
85+
# Reset mocks for forced call
86+
mock_alembic_command.reset_mock()
87+
mock_search_repository.reset_mock()
88+
89+
# Forced call should run migrations again
90+
await db.run_migrations(app_config, force=True)
91+
92+
# Verify migrations were called again
93+
mock_alembic_command.upgrade.assert_called_once_with(mock_alembic_config, "head")
94+
mock_search_repository.init_search_index.assert_called_once()
95+
96+
97+
@pytest.mark.asyncio
98+
async def test_migration_state_reset_on_shutdown():
99+
"""Test that migration state is reset when database is shut down."""
100+
# Set up completed state
101+
db._migrations_completed = True
102+
db._engine = AsyncMock()
103+
db._session_maker = AsyncMock()
104+
105+
# Shutdown should reset state
106+
await db.shutdown_db()
107+
108+
# Verify state was reset
109+
assert db._migrations_completed is False
110+
assert db._engine is None
111+
assert db._session_maker is None
112+
113+
114+
@pytest.mark.asyncio
115+
async def test_get_or_create_db_runs_migrations_automatically(
116+
app_config, mock_alembic_config, mock_alembic_command, mock_search_repository
117+
):
118+
"""Test that get_or_create_db runs migrations automatically."""
119+
# Reset module state
120+
db._migrations_completed = False
121+
db._engine = None
122+
db._session_maker = None
123+
124+
# First call should create engine and run migrations
125+
engine, session_maker = await db.get_or_create_db(
126+
app_config.database_path, app_config=app_config
127+
)
128+
129+
# Verify we got valid objects
130+
assert engine is not None
131+
assert session_maker is not None
132+
133+
# Verify migrations were called
134+
mock_alembic_command.upgrade.assert_called_once_with(mock_alembic_config, "head")
135+
mock_search_repository.init_search_index.assert_called_once()
136+
137+
138+
@pytest.mark.asyncio
139+
async def test_get_or_create_db_skips_migrations_when_disabled(
140+
app_config, mock_alembic_config, mock_alembic_command, mock_search_repository
141+
):
142+
"""Test that get_or_create_db can skip migrations when ensure_migrations=False."""
143+
# Reset module state
144+
db._migrations_completed = False
145+
db._engine = None
146+
db._session_maker = None
147+
148+
# Call with ensure_migrations=False should skip migrations
149+
engine, session_maker = await db.get_or_create_db(
150+
app_config.database_path, ensure_migrations=False
151+
)
152+
153+
# Verify we got valid objects
154+
assert engine is not None
155+
assert session_maker is not None
156+
157+
# Verify migrations were NOT called
158+
mock_alembic_command.upgrade.assert_not_called()
159+
mock_search_repository.init_search_index.assert_not_called()
160+
161+
162+
@pytest.mark.asyncio
163+
async def test_multiple_get_or_create_db_calls_deduplicated(
164+
app_config, mock_alembic_config, mock_alembic_command, mock_search_repository
165+
):
166+
"""Test that multiple get_or_create_db calls only run migrations once."""
167+
# Reset module state
168+
db._migrations_completed = False
169+
db._engine = None
170+
db._session_maker = None
171+
172+
# First call should create engine and run migrations
173+
await db.get_or_create_db(app_config.database_path, app_config=app_config)
174+
175+
# Verify migrations were called
176+
mock_alembic_command.upgrade.assert_called_once_with(mock_alembic_config, "head")
177+
mock_search_repository.init_search_index.assert_called_once()
178+
179+
# Reset mocks for subsequent calls
180+
mock_alembic_command.reset_mock()
181+
mock_search_repository.reset_mock()
182+
183+
# Subsequent calls should not run migrations again
184+
await db.get_or_create_db(app_config.database_path, app_config=app_config)
185+
await db.get_or_create_db(app_config.database_path, app_config=app_config)
186+
187+
# Verify migrations were NOT called again
188+
mock_alembic_command.upgrade.assert_not_called()
189+
mock_search_repository.init_search_index.assert_not_called()

0 commit comments

Comments
 (0)