Skip to content

Commit 34758cd

Browse files
phernandezclaude
andcommitted
fix: handle Windows subprocess incompatibility and Postgres test isolation
Two root cause fixes for CI failures: 1. Windows subprocess incompatibility: - SelectorEventLoop (used on Windows for aiosqlite stability) doesn't support asyncio.create_subprocess_shell() - Added Windows detection to _quick_count_files() and _scan_directory_modified_since() to use Python fallbacks - Updated db.py comment to document this limitation 2. Postgres integration test isolation: - Test fixtures created their own engine but MCP lifespan created a separate global engine via get_or_create_db() - This caused DuplicateTableError when MCP tried to run migrations - Fixed by setting db._engine and db._session_maker in test fixture so get_or_create_db() skips re-initialization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent f4dbcc0 commit 34758cd

6 files changed

Lines changed: 42 additions & 5 deletions

File tree

src/basic_memory/api/app.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ async def lifespan(app: FastAPI): # pragma: no cover
6262
)
6363
if app_config.sync_changes and not is_test_env:
6464
logger.info(f"Sync changes enabled: {app_config.sync_changes}")
65+
6566
# start file sync task in background
6667
async def _file_sync_runner() -> None:
6768
await initialize_file_sync(app_config)

src/basic_memory/db.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@
3333
# - "RuntimeError: Event loop is closed"
3434
# - "IndexError: pop from an empty deque"
3535
#
36-
# We don't rely on Proactor-only features (like subprocess pipes), so use the
37-
# more stable SelectorEventLoopPolicy everywhere on Windows.
36+
# The SelectorEventLoop doesn't support subprocess operations, so code that uses
37+
# asyncio.create_subprocess_shell() (like sync_service._quick_count_files) must
38+
# detect Windows and use fallback implementations.
3839
if sys.platform == "win32": # pragma: no cover
3940
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
4041

src/basic_memory/mcp/server.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ async def lifespan(app: FastMCP):
2626
app_config = ConfigManager().config
2727
logger.info("Starting Basic Memory MCP server")
2828

29+
# Track if we created the engine (vs test fixtures providing it)
30+
# This prevents disposing an engine provided by test fixtures when
31+
# multiple Client connections are made in the same test
32+
engine_was_none = db._engine is None
33+
2934
# Initialize app (runs migrations, reconciles projects)
3035
await initialize_app(app_config)
3136

@@ -40,6 +45,7 @@ async def lifespan(app: FastMCP):
4045
logger.info("Test environment detected - skipping local file sync")
4146
elif app_config.sync_changes and not app_config.cloud_mode_enabled:
4247
logger.info("Starting file sync in background")
48+
4349
async def _file_sync_runner() -> None:
4450
await initialize_file_sync(app_config)
4551

@@ -61,8 +67,12 @@ async def _file_sync_runner() -> None:
6167
except asyncio.CancelledError:
6268
logger.info("File sync task cancelled")
6369

64-
await db.shutdown_db()
65-
logger.info("Database connections closed")
70+
# Only shutdown DB if we created it (not if test fixture provided it)
71+
if engine_was_none:
72+
await db.shutdown_db()
73+
logger.info("Database connections closed")
74+
else:
75+
logger.debug("Skipping DB shutdown - engine provided externally")
6676

6777

6878
mcp = FastMCP(

src/basic_memory/sync/sync_service.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import asyncio
44
import os
5+
import sys
56
import time
67
from collections import OrderedDict
78
from dataclasses import dataclass, field
@@ -1027,12 +1028,22 @@ async def _quick_count_files(self, directory: Path) -> int:
10271028
Uses subprocess to leverage OS-level file counting which is much faster
10281029
than Python iteration, especially on network filesystems like TigrisFS.
10291030
1031+
On Windows, subprocess is not supported with SelectorEventLoop (which we use
1032+
to avoid aiosqlite cleanup issues), so we fall back to Python-based counting.
1033+
10301034
Args:
10311035
directory: Directory to count files in
10321036
10331037
Returns:
10341038
Number of files in directory (recursive)
10351039
"""
1040+
# Windows with SelectorEventLoop doesn't support subprocess
1041+
if sys.platform == "win32":
1042+
count = 0
1043+
async for _ in self.scan_directory(directory):
1044+
count += 1
1045+
return count
1046+
10361047
process = await asyncio.create_subprocess_shell(
10371048
f'find "{directory}" -type f | wc -l',
10381049
stdout=asyncio.subprocess.PIPE,
@@ -1063,13 +1074,20 @@ async def _scan_directory_modified_since(
10631074
This is dramatically faster than scanning all files and comparing mtimes,
10641075
especially on network filesystems like TigrisFS where stat operations are expensive.
10651076
1077+
On Windows, subprocess is not supported with SelectorEventLoop (which we use
1078+
to avoid aiosqlite cleanup issues), so we fall back to a full directory scan.
1079+
10661080
Args:
10671081
directory: Directory to scan
10681082
since_timestamp: Unix timestamp to find files newer than
10691083
10701084
Returns:
10711085
List of relative file paths modified since the timestamp (respects .bmignore)
10721086
"""
1087+
# Windows with SelectorEventLoop doesn't support subprocess
1088+
if sys.platform == "win32":
1089+
return await self._scan_directory_full(directory)
1090+
10731091
# Convert timestamp to find-compatible format
10741092
since_date = datetime.fromtimestamp(since_timestamp).strftime("%Y-%m-%d %H:%M:%S")
10751093

test-int/conftest.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,11 @@ async def engine_factory(
146146
autoflush=False,
147147
)
148148

149+
# Set module-level state to prevent MCP lifespan from re-initializing
150+
# This ensures get_or_create_db() sees an existing engine and skips initialization
151+
db._engine = engine
152+
db._session_maker = session_maker
153+
149154
# Drop and recreate all tables for test isolation
150155
async with engine.begin() as conn:
151156
await conn.execute(text("DROP TABLE IF EXISTS search_index CASCADE"))
@@ -158,7 +163,10 @@ async def engine_factory(
158163

159164
yield engine, session_maker
160165

166+
# Clean up module-level state
161167
await engine.dispose()
168+
db._engine = None
169+
db._session_maker = None
162170

163171
else:
164172
# SQLite: Create fresh database (fast with tmp files)

tests/services/test_initialization.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
ensure_initialization,
99
initialize_database,
1010
reconcile_projects_with_config,
11-
initialize_file_sync,
1211
)
1312

1413

0 commit comments

Comments
 (0)