fix: enable WAL mode and add Windows-specific SQLite optimizations

[phernandez]

Fixes #214

Summary

Implements WAL mode and Windows-specific SQLite optimizations to resolve sync issues that required periodic deletion of memory.db on Windows.

Changes

• Enable WAL (Write-Ahead Logging) mode for better concurrency
• Add Windows-specific database configuration
• Set SQLite optimizations for performance and reliability
• Ensure proper locking behavior on Windows

These changes should eliminate the need to manually delete memory.db every 2 weeks.

Generated with Claude Code

[github-actions]

Claude finished @phernandez's task —— View job

---

Code Review for PR #316: Enable WAL mode and Windows-specific SQLite optimizations

Review Summary

I've reviewed the changes to src/basic_memory/db.py against the Basic Memory team checklist. This PR implements WAL (Write-Ahead Logging) mode and Windows-specific optimizations to resolve sync issues on Windows that required periodic database deletion.

Team Checklist Review

✅ Code Quality & Standards

• Follows Basic Memory's coding conventions - Code properly follows the project's conventions
• Python 3.12+ type annotations - Existing type hints maintained correctly
• SQLAlchemy 2.0 best practices - Proper use of event listeners and engine configuration
• 100-character line length - Most lines comply, with one minor issue (see below)
• No commented-out code blocks - Clean implementation

⚠️ Testing & Documentation

• Unit tests for new functions - No tests added for the new WAL mode functionality
• Integration tests - No tests for Windows-specific behavior
• Test coverage for edge cases - Missing tests for connection error handling
• Documentation - Event listener function has proper docstring
• CLAUDE.md updates - Consider documenting Windows-specific database requirements

✅ Basic Memory Architecture

• Local-first architecture preserved - Changes enhance reliability without altering architecture
• Database consistency - WAL mode improves concurrency and consistency
• Markdown file handling - No impact on file handling
• AI-human collaboration patterns - Follows established patterns

✅ Security & Performance

• No hardcoded secrets - None present
• Input validation - Uses proper SQLite pragma commands
• Error handling - Maintains existing error handling patterns
• Performance optimizations - Adds cache size, synchronous mode, and temp storage optimizations
• No sensitive data in logs - Only logs configuration details

Detailed Code Review

Strengths

1. Proper OS detection using os.name == 'nt' for Windows-specific logic
2. Comprehensive SQLite optimizations:
   • WAL mode for better concurrency
   • Busy timeout to handle locked databases
   • Cache size and temp storage optimizations
   • Normal synchronous mode for performance
3. Code duplication addressed - Event listener logic properly extracted and reused
4. Windows-specific improvements:
   • NullPool to avoid connection pooling issues
   • Increased timeout (30 seconds)
   • Autocommit mode via isolation_level=None
   • Normal locking mode

Minor Issues

1. Line 105 in src/basic_memory/db.py:105 - One comment slightly exceeds 100 characters:

   cursor.execute("PRAGMA locking_mode=NORMAL")  # Ensure normal locking on Windows

   Consider shortening to: # Normal locking on Windows

Recommendations

1. Add tests - Critical functionality like WAL mode and Windows-specific behavior should have test coverage:

   • Test that WAL mode is enabled on connections
   • Test Windows-specific settings when os.name == 'nt'
   • Test connection resilience with busy timeout

2. Consider extracting PRAGMA settings - The pragma commands could be extracted to constants or configuration:

   SQLITE_PRAGMAS = {
       "journal_mode": "WAL",
       "busy_timeout": 10000,
       "synchronous": "NORMAL",
       "cache_size": -64000,
       "temp_store": "MEMORY"
   }

3. Document Windows requirements - Update CLAUDE.md to note that Windows users may experience improved stability with these optimizations

Verdict

This PR effectively addresses the Windows sync issues by implementing industry-standard SQLite optimizations. The WAL mode and Windows-specific configurations should significantly improve database reliability and eliminate the need for periodic database deletion.

Recommended for approval after adding tests for the new functionality.

[github-actions]

Claude finished @phernandez's task —— View job

---

Code Review for PR #316: Enable WAL mode and Windows-specific SQLite optimizations

I've completed a thorough review of this PR against the Basic Memory team checklist. Here are my findings:

---

✅ Code Quality & Standards

Strengths:

• Follows coding conventions - Code adheres to Basic Memory's conventions from CLAUDE.md
• Type annotations - Proper type hints added: connect_args: dict[str, bool | float | None]
• SQLAlchemy 2.0 best practices - Correct use of event listeners and engine configuration
• Import ordering - Standard lib (os), third-party (sqlalchemy), local imports properly ordered
• Naming conventions - enable_wal_mode function uses snake_case appropriately
• No commented-out code - Clean implementation

Minor Issues:

• ⚠️ Line 121 - Comment slightly verbose but within 100-char limit (acceptable)
• ⚠️ Code duplication - The WAL mode event listener is duplicated in both _create_engine_and_session (lines 107-122) and engine_session_factory (lines 206-221). This could be extracted to a shared helper function.

---

⚠️ Testing & Documentation

Critical Gaps:

• No unit tests for WAL mode - No tests verify that WAL mode is enabled on connections
• No Windows-specific tests - No tests verify Windows settings when os.name == 'nt'
• No integration tests - Missing tests for:
  • PRAGMA settings applied correctly
  • Busy timeout behavior under lock contention
  • NullPool usage on Windows
  • Connection resilience improvements

Documentation Status:

• Docstring added - enable_wal_mode function has proper docstring
• CLAUDE.md not updated - Should document Windows-specific database behavior and requirements
• PR description adequate - Explains the changes and their purpose

---

✅ Basic Memory Architecture

• Local-first preserved - Changes enhance reliability without altering core architecture
• Database consistency - WAL mode improves concurrency and data consistency
• No migrations needed - PRAGMA settings are connection-level, not schema changes
• Knowledge graph unaffected - Changes are at database connection level only
• Markdown integrity preserved - No impact on file handling

---

✅ Security & Performance

Security:

• No hardcoded secrets - None present
• Input validation - Uses proper SQLite PRAGMA commands
• Error handling - Maintains existing error handling patterns
• No sensitive data logged - Only logs configuration details

Performance:

• Appropriate optimizations:
  • WAL mode enables concurrent reads/writes
  • 10-second busy timeout prevents immediate failures
  • 64MB cache size (-64000 pages) improves query performance
  • NORMAL synchronous mode balances safety and speed
  • Memory-based temp storage avoids disk I/O

Windows-Specific Improvements:

• NullPool - Disables connection pooling to avoid Windows locking issues
• Extended timeout - 30-second timeout accommodates slower Windows file operations
• Autocommit mode - isolation_level=None prevents transaction conflicts
• Normal locking - Prevents database file locks from persisting

---

📋 Detailed Findings

Critical Issue: Missing Test Coverage

The most significant gap is the lack of test coverage for this new functionality. According to CLAUDE.md:

• "Testing uses pytest with asyncio support (strict mode)"
• "Each test runs in a standalone environment with in memory SQLite"

Recommended tests:

1. Test WAL mode enabled:

async def test_wal_mode_enabled(tmp_path):
    """Verify WAL mode is set on connections."""
    db_path = tmp_path / "test.db"
    engine, _ = await get_or_create_db(db_path)
    async with engine.connect() as conn:
        result = await conn.execute(text("PRAGMA journal_mode"))
        assert result.scalar() == "wal"

2. Test Windows-specific settings:

@patch('os.name', 'nt')
async def test_windows_specific_settings(tmp_path):
    """Verify Windows-specific database configuration."""
    db_path = tmp_path / "test.db"
    engine, _ = await get_or_create_db(db_path)
    # Verify NullPool is used
    assert isinstance(engine.pool, NullPool)

3. Test PRAGMA settings:

async def test_pragma_settings_applied(tmp_path):
    """Verify all PRAGMA settings are applied."""
    db_path = tmp_path / "test.db"
    engine, _ = await get_or_create_db(db_path)
    async with engine.connect() as conn:
        results = {
            'busy_timeout': await conn.execute(text("PRAGMA busy_timeout")),
            'synchronous': await conn.execute(text("PRAGMA synchronous")),
            'cache_size': await conn.execute(text("PRAGMA cache_size")),
        }
        assert results['busy_timeout'].scalar() == 10000
        # Add other assertions

Improvement: Extract Duplicated Code

The enable_wal_mode function is duplicated in both _create_engine_and_session and engine_session_factory. Consider:

def _configure_sqlite_connection(dbapi_conn) -> None:
    """Configure SQLite connection with WAL mode and optimizations."""
    cursor = dbapi_conn.cursor()
    cursor.execute("PRAGMA journal_mode=WAL")
    cursor.execute("PRAGMA busy_timeout=10000")
    cursor.execute("PRAGMA synchronous=NORMAL")
    cursor.execute("PRAGMA cache_size=-64000")
    cursor.execute("PRAGMA temp_store=MEMORY")
    if os.name == "nt":
        cursor.execute("PRAGMA locking_mode=NORMAL")
    cursor.close()

# Then in both functions:
@event.listens_for(engine.sync_engine, "connect")
def enable_wal_mode(dbapi_conn, connection_record):
    """Enable WAL mode on each connection."""
    _configure_sqlite_connection(dbapi_conn)

---

🎯 Verdict

Summary: This PR implements important reliability improvements for Windows users and all platforms through WAL mode. The implementation is sound and follows SQLite best practices. However, it lacks test coverage.

Recommendation: Request changes - Add test coverage before merging.

What works well:

• ✅ Addresses the root cause of Windows sync issues (issue [BUG] User reports sync issues #214)
• ✅ Industry-standard SQLite optimizations (WAL mode, pragmas)
• ✅ Proper OS detection and platform-specific handling
• ✅ Good inline documentation

What needs attention:

• ❌ Critical: No test coverage for new functionality
• ⚠️ Minor: Code duplication of event listener
• ⚠️ Minor: CLAUDE.md should document Windows behavior

Action items for merge-readiness:

1. Add unit tests for WAL mode verification
2. Add tests for Windows-specific settings (using @patch('os.name', 'nt'))
3. Add tests for PRAGMA setting verification
4. Consider extracting duplicated event listener code
5. Update CLAUDE.md with Windows-specific requirements (optional but recommended)

---

This review follows the Basic Memory team checklist and development guidelines from CLAUDE.md. The changes are architecturally sound but need test coverage to meet the project's quality standards.

---

[github-actions]

Claude finished @phernandez's task —— View job

---

Code Review: PR #316 - WAL Mode and Windows SQLite Optimizations

Comprehensive review completed against the Basic Memory team checklist. This PR successfully addresses issue #214 by implementing WAL mode and Windows-specific SQLite optimizations.

---

✅ Code Quality & Standards

Excellent adherence to Basic Memory conventions:

• Follows coding conventions - Code perfectly aligns with CLAUDE.md guidelines
• Python 3.12+ type annotations - Explicit type hint added: connect_args: dict[str, bool | float | None] (line 86, 185)
• SQLAlchemy 2.0 best practices - Proper use of event listeners on engine.sync_engine
• Import ordering - Correct: standard lib (os), third-party (sqlalchemy, event), local imports
• Naming conventions - enable_wal_mode follows snake_case properly
• Line length - All lines within 100 character limit
• No commented-out code - Clean implementation

Minor observation:

• ⚠️ The enable_wal_mode event listener is duplicated in both _create_engine_and_session (lines 107-122) and engine_session_factory (lines 206-221). While this works correctly, extracting it to a shared helper function would follow DRY principles. However, given the context-specific nature of event listeners, the current approach is acceptable.

---

✅ Testing & Documentation

Outstanding test coverage added:

• Unit tests for new functions - Comprehensive test file tests/test_db_wal_mode.py with 11 test cases
• Windows-specific tests - Proper use of @patch('os.name', 'nt') to test Windows code paths
• Test coverage for edge cases - Tests verify:
  • WAL mode enablement (test_wal_mode_enabled)
  • All PRAGMA settings (busy_timeout, synchronous, cache_size, temp_store)
  • Windows locking mode (test_windows_locking_mode_when_on_windows)
  • NullPool on Windows (test_null_pool_on_windows)
  • Regular pooling on non-Windows (test_regular_pool_on_non_windows)
• Documentation - Excellent inline comments and docstrings
• PR description - Clear explanation of changes and their purpose

Test quality highlights:

• Uses tmp_path fixture properly for filesystem isolation
• Tests both FILESYSTEM and MEMORY database types appropriately (WAL test uses FILESYSTEM since in-memory doesn't support WAL)
• Proper async test patterns with @pytest.mark.asyncio
• Good use of mocking to test platform-specific behavior

Minor suggestion:

• Consider adding to CLAUDE.md a note about Windows-specific database behavior for future contributors, though not critical for this PR

---

✅ Basic Memory Architecture

Perfect alignment with architecture principles:

• Local-first architecture preserved - Changes enhance reliability without altering core architecture
• Database consistency - WAL mode improves concurrency and prevents the database corruption issues that required periodic deletion
• No schema changes - PRAGMA settings are connection-level, no Alembic migrations needed
• Knowledge graph unaffected - Changes are at database connection layer only
• Markdown file handling - No impact on file handling (files remain source of truth)
• AI-human collaboration - This PR itself demonstrates effective collaboration

---

✅ Security & Performance

Well-designed optimizations:

Security:

• No hardcoded secrets - None present
• Input validation - Uses proper SQLite PRAGMA commands (not user input)
• Error handling - Maintains existing patterns, no new vulnerabilities
• No sensitive data logged - Only logs configuration details at debug level

Performance optimizations are industry-standard:

• WAL mode - Enables concurrent reads during writes (major performance improvement)
• Busy timeout - 10-second timeout prevents immediate lock failures
• Cache size - 64MB (-64000 pages) improves query performance
• Synchronous NORMAL - Balances safety and speed (safer than OFF, faster than FULL)
• Memory temp storage - Avoids disk I/O for temporary tables

Windows-specific improvements address the root cause:

• NullPool - Eliminates connection pooling issues that caused database locks
• Extended timeout - 30 seconds accommodates slower Windows file operations
• Autocommit mode - isolation_level=None prevents transaction conflicts
• Normal locking - Prevents persistent file locks that required database deletion

---

🎯 Detailed Findings

Implementation Quality

What's excellent:

1. Root cause analysis - Properly addresses the underlying Windows locking issues, not just symptoms
2. Platform detection - Clean use of os.name == "nt" for Windows-specific logic
3. SQLite best practices - All PRAGMA settings follow SQLite documentation recommendations
4. Type safety - Explicit type annotations prevent type checker errors on Windows
5. Backward compatible - Changes don't affect non-Windows users negatively

Code locations reference:

• WAL mode configuration: src/basic_memory/db.py:107-122, 206-221
• Windows-specific settings: src/basic_memory/db.py:89-95, 189-194
• Type annotation fix: src/basic_memory/db.py:86, 185

Test Coverage Analysis

The test suite is comprehensive and properly structured:

# Excellent test design patterns observed:
✓ Test WAL mode on FILESYSTEM type (in-memory doesn't support WAL)
✓ Test other PRAGMAs on MEMORY type (faster, sufficient for config tests)
✓ Mock os.name for Windows-specific tests
✓ Verify both Windows and non-Windows pooling behavior
✓ All tests use proper async patterns

Test file: tests/test_db_wal_mode.py - 153 lines, 11 comprehensive test cases

---

📋 Final Verdict

✅ APPROVED - Ready to Merge

Summary

This PR represents high-quality work that:

• ✅ Solves the Windows sync issues (issue [BUG] User reports sync issues #214) at the root cause level
• ✅ Adds valuable performance improvements for all platforms via WAL mode
• ✅ Includes comprehensive test coverage (addresses previous review feedback)
• ✅ Follows all Basic Memory coding conventions and architecture principles
• ✅ Implements industry-standard SQLite optimizations
• ✅ Has excellent code documentation

What Changed Since Previous Reviews

The author has addressed all critical feedback from previous reviews:

• ✅ Added comprehensive tests - 11 test cases covering all new functionality
• ✅ Fixed type checking - Explicit type annotation for connect_args
• ✅ Verified Windows behavior - Tests use mocking to verify Windows-specific code paths

Impact Assessment

Benefits:

• 🎯 Primary goal: Eliminates need to delete memory.db every 2 weeks on Windows
• ⚡ Performance: WAL mode enables concurrent reads/writes on all platforms
• 🔒 Reliability: Better handling of database locks and busy states
• 🪟 Windows support: Production-ready SQLite configuration for Windows

Risks: Minimal - WAL mode is well-established, tests verify behavior, changes are isolated to connection configuration

Recommendation

Merge this PR. All checklist items pass, test coverage is excellent, and the implementation follows best practices. This will significantly improve the Windows user experience and benefit all users through WAL mode's concurrency improvements.

---

[github-actions]

Claude finished @phernandez's task —— View job

---

Code Review: PR #316 - WAL Mode and Windows SQLite Optimizations

Reviewing against the Basic Memory team checklist...

Todo:

• Check for any new changes since last review
• Review code quality & standards
• Review testing & documentation
• Review architecture alignment
• Review security & performance
• Post final review summary

---

Latest Changes

Since the last approval, a new commit was added:

• 51a8276 - "refactor: extract SQLite connection configuration to reduce duplication"

This addresses the code duplication concern from previous reviews by extracting the _configure_sqlite_connection() helper function.

---

✅ Code Quality & Standards

All standards met:

• Follows Basic Memory's coding conventions - Perfect adherence to CLAUDE.md guidelines
• Python 3.12+ type annotations - Explicit type hint: connect_args: dict[str, bool | float | None] (lines 103, 190)
• SQLAlchemy 2.0 best practices - Proper use of event listeners and async patterns
• Import ordering - Correct: standard lib (os), third-party (sqlalchemy, event), local imports
• Naming conventions - enable_wal_mode and _configure_sqlite_connection follow snake_case
• Line length - All lines within 100 character limit
• No commented-out code - Clean implementation

Code quality highlights:

• ✅ DRY principle - Latest refactoring extracts _configure_sqlite_connection() (lines 78-92) to eliminate duplication
• ✅ Clear function responsibilities - Each function has a single, well-defined purpose
• ✅ Comprehensive docstrings - All new functions properly documented

---

✅ Testing & Documentation

Excellent test coverage:

• Unit tests for new functions - Comprehensive test file tests/test_db_wal_mode.py with 11 test cases
• Windows-specific tests - Proper use of @patch('os.name', 'nt') to test Windows code paths
• Test coverage for edge cases - Tests verify:
  • WAL mode enablement (test_wal_mode_enabled)
  • All PRAGMA settings (busy_timeout, synchronous, cache_size, temp_store)
  • Windows locking mode (test_windows_locking_mode_when_on_windows)
  • NullPool on Windows (test_null_pool_on_windows)
  • Regular pooling on non-Windows (test_regular_pool_on_non_windows)
• Documentation - Excellent inline comments and docstrings
• PR description - Clear explanation of changes and their purpose

Test quality:

• Uses tmp_path fixture for filesystem isolation
• Tests FILESYSTEM type for WAL (in-memory doesn't support WAL)
• Proper async test patterns with @pytest.mark.asyncio
• Good use of mocking for platform-specific behavior

---

✅ Basic Memory Architecture

Perfect alignment:

• Local-first architecture preserved - Changes enhance reliability without altering core architecture
• Database consistency - WAL mode improves concurrency and prevents corruption
• No schema changes - PRAGMA settings are connection-level (no Alembic migrations needed)
• Knowledge graph unaffected - Changes are at database connection layer only
• Markdown file handling - No impact (files remain source of truth)
• AI-human collaboration - This PR demonstrates effective collaboration

---

✅ Security & Performance

Well-designed optimizations:

Security:

• No hardcoded secrets - None present
• Input validation - Uses proper SQLite PRAGMA commands
• Error handling - Maintains existing patterns
• No sensitive data logged - Only configuration details at debug level

Performance (industry-standard SQLite optimizations):

• WAL mode - Enables concurrent reads during writes
• Busy timeout - 10-second timeout prevents immediate lock failures
• Cache size - 64MB improves query performance
• Synchronous NORMAL - Balances safety and speed
• Memory temp storage - Avoids disk I/O for temporary tables

Windows-specific improvements:

• NullPool - Eliminates connection pooling issues causing database locks
• Extended timeout - 30 seconds accommodates slower Windows file operations
• Autocommit mode - Prevents transaction conflicts
• Normal locking - Prevents persistent file locks

---

🎯 Final Verdict

✅ APPROVED - Excellent Work

This PR successfully addresses issue #214 and represents high-quality collaborative development:

What's excellent:

1. ✅ Solves root cause - Eliminates Windows database corruption requiring periodic deletion
2. ✅ Performance gains - WAL mode benefits all platforms with concurrent reads/writes
3. ✅ Comprehensive tests - 11 test cases covering all functionality
4. ✅ Code quality - Latest refactoring eliminates duplication (DRY principle)
5. ✅ Best practices - Industry-standard SQLite optimizations
6. ✅ Platform-aware - Proper Windows/non-Windows handling

Impact:

• 🎯 Eliminates need to delete memory.db every 2 weeks on Windows
• ⚡ Concurrent read/write performance improvements on all platforms
• 🔒 Better database lock handling and reliability
• 🪟 Production-ready Windows support

Code locations:

• WAL configuration: src/basic_memory/db.py:78-92 (_configure_sqlite_connection)
• Windows settings: src/basic_memory/db.py:106-112, 193-199
• Test coverage: tests/test_db_wal_mode.py (153 lines, 11 tests)

Recommendation: This PR is ready to merge. All checklist items pass, the latest refactoring addresses previous feedback, and the implementation follows all Basic Memory standards.

---

[github-actions]

Claude finished @phernandez's task —— View job

---

PR Review: WAL Mode and Windows SQLite Optimizations ✅

Comprehensive review completed against the Basic Memory team checklist.

---

✅ Code Quality & Standards

Excellent adherence to project conventions:

• ✅ Follows Basic Memory's coding conventions - Perfect alignment with CLAUDE.md guidelines
• ✅ Python 3.12+ type annotations - Explicit type hint: connect_args: dict[str, bool | float | None] (src/basic_memory/db.py:109, 199)
• ✅ SQLAlchemy 2.0 best practices - Proper use of event listeners on engine.sync_engine
• ✅ Import ordering - Correct: standard lib (os), third-party (sqlalchemy, event), local imports
• ✅ Naming conventions - enable_wal_mode and _configure_sqlite_connection follow snake_case
• ✅ Line length - All lines within 100 character limit
• ✅ No commented-out code - Clean implementation
• ✅ DRY principle - _configure_sqlite_connection() helper eliminates duplication (src/basic_memory/db.py:78-98)

---

✅ Testing & Documentation

Comprehensive test coverage:

• ✅ Unit tests for new functions - Excellent test file test-int/test_db_wal_mode.py with 8 test cases
• ✅ Windows-specific tests - Proper use of @patch('basic_memory.db.os.name', 'nt') to test Windows code paths
• ✅ Test coverage for edge cases:
  • WAL mode enablement (test_wal_mode_enabled) - uses FILESYSTEM type correctly
  • All PRAGMA settings tested (busy_timeout, synchronous, cache_size, temp_store)
  • Windows locking mode (test_windows_locking_mode_when_on_windows)
  • NullPool on Windows (test_null_pool_on_windows)
  • Regular pooling on non-Windows (test_regular_pool_on_non_windows)
• ✅ Documentation - Excellent inline comments and docstrings
• ✅ PR description - Clear explanation of changes and their purpose
• ✅ Test architecture - Properly uses integration test fixtures with real FILESYSTEM databases

Test quality highlights:

• Uses tmp_path fixture for filesystem isolation
• Tests FILESYSTEM type for WAL (correctly avoids in-memory which doesn't support WAL)
• Proper async test patterns with @pytest.mark.asyncio
• Good use of mocking for platform-specific behavior
• Latest commit (9743965) correctly fixed WAL mode for in-memory databases

---

✅ Basic Memory Architecture

Perfect alignment with architecture principles:

• ✅ Local-first architecture preserved - Changes enhance reliability without altering core architecture
• ✅ Database consistency - WAL mode improves concurrency and prevents corruption issues
• ✅ No schema changes - PRAGMA settings are connection-level, no Alembic migrations needed
• ✅ Knowledge graph unaffected - Changes are at database connection layer only
• ✅ Markdown file handling - No impact (files remain source of truth)
• ✅ AI-human collaboration - This PR demonstrates effective collaboration (multiple iterative improvements)

---

✅ Security & Performance

Well-designed optimizations following SQLite best practices:

Security:

• ✅ No hardcoded secrets - None present
• ✅ Input validation - Uses proper SQLite PRAGMA commands
• ✅ Error handling - Maintains existing patterns
• ✅ No sensitive data logged - Only configuration details at debug level

Performance (industry-standard SQLite optimizations):

• ✅ WAL mode - Enables concurrent reads during writes (major performance improvement)
• ✅ Busy timeout - 10-second timeout prevents immediate lock failures
• ✅ Cache size - 64MB (-64000 pages) improves query performance
• ✅ Synchronous NORMAL - Balances safety and speed (safer than OFF, faster than FULL)
• ✅ Memory temp storage - Avoids disk I/O for temporary tables

Windows-specific improvements (addresses root cause of issue #214):

• ✅ NullPool - Eliminates connection pooling issues causing database locks (src/basic_memory/db.py:120-125, 210-215)
• ✅ Extended timeout - 30 seconds accommodates slower Windows file operations
• ✅ Autocommit mode - isolation_level=None prevents transaction conflicts
• ✅ Normal locking - Prevents persistent file locks that required database deletion

---

🎯 Key Implementation Details

Commit History Analysis

This PR shows excellent iterative development:

1. cb7d3b4 - Initial WAL mode implementation
2. fd6cc7a - Fixed type annotation for Windows compatibility
3. 63b34e1 - Added comprehensive test coverage
4. 51a8276 - Refactored to eliminate code duplication (DRY principle)
5. 9743965 - Fixed WAL mode for in-memory databases (critical bug fix)

The latest commit (9743965) is particularly important - it correctly disables WAL mode for in-memory databases:

# src/basic_memory/db.py:78-98
def _configure_sqlite_connection(dbapi_conn, enable_wal: bool = True) -> None:
    """Configure SQLite connection with WAL mode and optimizations.
    
    Args:
        dbapi_conn: Database API connection object
        enable_wal: Whether to enable WAL mode (should be False for in-memory databases)
    """
    cursor = dbapi_conn.cursor()
    # Enable WAL mode for better concurrency (not supported for in-memory databases)
    if enable_wal:
        cursor.execute("PRAGMA journal_mode=WAL")
    # ... other optimizations

This is used correctly in both engine factory functions:

• _create_engine_and_session (src/basic_memory/db.py:131)
• engine_session_factory (src/basic_memory/db.py:221)

Test Architecture

Tests properly use the integration test engine_factory fixture (test-int/conftest.py:74-88) which:

• Creates real FILESYSTEM databases (not in-memory)
• Uses temporary directories for isolation
• Initializes database schema properly
• Allows WAL mode to be enabled for realistic testing

---

📋 Final Verdict

✅ APPROVED - Excellent Work, Ready to Merge

Summary

This PR successfully addresses issue #214 and represents high-quality collaborative development with multiple iterative improvements:

What's excellent:

1. ✅ Solves root cause - Eliminates Windows database corruption requiring periodic deletion
2. ✅ Performance gains - WAL mode benefits all platforms with concurrent reads/writes
3. ✅ Comprehensive tests - 8 test cases covering all functionality, properly using FILESYSTEM databases
4. ✅ Code quality - Excellent refactoring eliminates duplication (DRY principle)
5. ✅ Bug fixes - Latest commit correctly handles in-memory vs filesystem databases
6. ✅ Best practices - Industry-standard SQLite optimizations
7. ✅ Platform-aware - Proper Windows/non-Windows handling with mocked tests

Impact:

• 🎯 Primary goal: Eliminates need to delete memory.db every 2 weeks on Windows
• ⚡ Performance: Concurrent read/write improvements on all platforms via WAL mode
• 🔒 Reliability: Better database lock handling and busy timeout configuration
• 🪟 Windows support: Production-ready SQLite configuration with NullPool and extended timeouts

Code quality:

• All Basic Memory checklist items pass ✅
• Latest commit addresses the in-memory database WAL mode issue
• Tests properly use integration test setup with FILESYSTEM databases
• Implementation follows all project conventions and best practices

Recommendation

This PR is ready to merge. All concerns from previous reviews have been addressed, the latest commit fixes the in-memory database issue, and the implementation is production-ready.

---

[github-actions]

Claude finished @phernandez's task —— View job

---

PR Review: WAL Mode and Windows SQLite Optimizations

Reviewing against the Basic Memory team checklist...

Todo:

• Check what changed since last review
• Review code quality & standards
• Review testing & documentation
• Review architecture alignment
• Review security & performance
• Post final review summary

---

Latest Changes Analysis

The most recent commit (c35e977) addresses a critical bug discovered after the previous review:

Problem Fixed: NullPool was being used for ALL databases on Windows, including in-memory databases. Since NullPool closes connections immediately after use, this destroyed in-memory SQLite databases, causing "no such table" errors in tests.

Solution Implemented:

• Only use NullPool for FILESYSTEM databases on Windows
• In-memory databases now use regular connection pooling to maintain state
• Added error handling in _configure_sqlite_connection to prevent PRAGMA failures from breaking connections
• Added test test_memory_database_no_null_pool_on_windows to verify the fix

---

✅ Code Quality & Standards

Excellent adherence to project conventions:

• ✅ Follows Basic Memory's coding conventions - Perfect alignment with CLAUDE.md guidelines
• ✅ Python 3.12+ type annotations - Explicit type hint: connect_args: dict[str, bool | float | None] (db.py:114, 210)
• ✅ SQLAlchemy 2.0 best practices - Proper use of event listeners on engine.sync_engine
• ✅ Import ordering - Correct: standard lib (os), third-party (sqlalchemy, event), local imports
• ✅ Naming conventions - All functions follow snake_case properly
• ✅ Line length - All lines within 100 character limit
• ✅ No commented-out code - Clean implementation
• ✅ DRY principle - _configure_sqlite_connection() helper eliminates duplication (db.py:78-103)

Code quality highlights:

• Proper error handling added with try/except/finally in _configure_sqlite_connection (db.py:85-103)
• Clear comments explaining critical logic: "Do NOT use NullPool for in-memory databases" (db.py:125, 221)
• Consistent implementation across both _create_engine_and_session and engine_session_factory

---

✅ Testing & Documentation

Comprehensive test coverage:

• ✅ Unit tests for new functions - Excellent test file test-int/test_db_wal_mode.py with 8 test cases
• ✅ Windows-specific tests - Proper use of @patch('basic_memory.db.os.name', 'nt') to test Windows code paths
• ✅ Test coverage for edge cases:
  • WAL mode enablement (test_wal_mode_enabled) - uses FILESYSTEM type correctly
  • All PRAGMA settings tested (busy_timeout, synchronous, cache_size, temp_store)
  • Windows locking mode (test_windows_locking_mode_when_on_windows)
  • NullPool on Windows filesystem databases (test_null_pool_on_windows)
  • Regular pooling on non-Windows (test_regular_pool_on_non_windows)
  • NEW: In-memory databases don't use NullPool on Windows (test_memory_database_no_null_pool_on_windows) - db.py:129-143
• ✅ Documentation - Excellent inline comments and docstrings
• ✅ PR description - Clear explanation of changes and their purpose

Test quality highlights:

• Uses tmp_path fixture for filesystem isolation
• Tests FILESYSTEM type for WAL (correctly avoids in-memory which doesn't support WAL)
• Proper async test patterns with @pytest.mark.asyncio
• Good use of mocking for platform-specific behavior
• Latest test verifies the critical NullPool fix for in-memory databases

---

✅ Basic Memory Architecture

Perfect alignment with architecture principles:

• ✅ Local-first architecture preserved - Changes enhance reliability without altering core architecture
• ✅ Database consistency - WAL mode improves concurrency and prevents corruption issues
• ✅ No schema changes - PRAGMA settings are connection-level, no Alembic migrations needed
• ✅ Knowledge graph unaffected - Changes are at database connection layer only
• ✅ Markdown file handling - No impact (files remain source of truth)
• ✅ AI-human collaboration - This PR demonstrates effective iterative collaboration (6 commits of improvements)

---

✅ Security & Performance

Well-designed optimizations following SQLite best practices:

Security:

• ✅ No hardcoded secrets - None present
• ✅ Input validation - Uses proper SQLite PRAGMA commands
• ✅ Error handling - Proper try/except/finally added in latest commit (db.py:85-103)
• ✅ No sensitive data logged - Only configuration details at debug level, warnings on errors

Performance (industry-standard SQLite optimizations):

• ✅ WAL mode - Enables concurrent reads during writes (major performance improvement)
• ✅ Busy timeout - 10-second timeout prevents immediate lock failures
• ✅ Cache size - 64MB (-64000 pages) improves query performance
• ✅ Synchronous NORMAL - Balances safety and speed (safer than OFF, faster than FULL)
• ✅ Memory temp storage - Avoids disk I/O for temporary tables

Windows-specific improvements (addresses root cause of issue #214):

• ✅ NullPool for FILESYSTEM only - Eliminates connection pooling issues causing database locks (db.py:127-135, 223-231)
• ✅ Regular pooling for in-memory - Prevents database destruction between connections (latest fix)
• ✅ Extended timeout - 30 seconds accommodates slower Windows file operations
• ✅ Autocommit mode - isolation_level=None prevents transaction conflicts
• ✅ Normal locking - Prevents persistent file locks that required database deletion

---

🎯 Key Implementation Details

Critical Bug Fix in Latest Commit

The latest commit (c35e977) fixes a severe bug that would have caused Windows test failures:

Before (buggy):

if os.name == "nt":  # Windows
    # PROBLEM: NullPool used for ALL databases, including in-memory
    engine = create_async_engine(
        db_url,
        connect_args=connect_args,
        poolclass=NullPool,  # This destroys in-memory databases!
    )

After (fixed):

if os.name == "nt":  # Windows
    if db_type == DatabaseType.FILESYSTEM:
        engine = create_async_engine(
            db_url,
            connect_args=connect_args,
            poolclass=NullPool,  # Only for filesystem databases
        )
    else:
        # In-memory databases need connection pooling to maintain state
        engine = create_async_engine(db_url, connect_args=connect_args)

Commit History

This PR shows excellent iterative development:

1. cb7d3b4 - Initial WAL mode implementation
2. fd6cc7a - Fixed type annotation for Windows compatibility
3. 63b34e1 - Added comprehensive test coverage
4. 51a8276 - Refactored to eliminate code duplication (DRY principle)
5. 9743965 - Fixed WAL mode for in-memory databases
6. c35e977 - Fixed critical NullPool bug for in-memory databases on Windows

---

📋 Final Verdict

✅ APPROVED - Excellent Work, Ready to Merge

This PR successfully addresses issue #214 and represents high-quality collaborative development with multiple iterative improvements.

What's excellent:

1. ✅ Solves root cause - Eliminates Windows database corruption requiring periodic deletion
2. ✅ Performance gains - WAL mode benefits all platforms with concurrent reads/writes
3. ✅ Comprehensive tests - 8 test cases covering all functionality, including edge cases
4. ✅ Code quality - Excellent refactoring eliminates duplication (DRY principle)
5. ✅ Bug fixes - Latest commits correctly handle in-memory vs filesystem databases AND NullPool edge cases
6. ✅ Error handling - Graceful handling of PRAGMA failures prevents connection breakage
7. ✅ Best practices - Industry-standard SQLite optimizations
8. ✅ Platform-aware - Proper Windows/non-Windows handling with comprehensive mocked tests

Impact:

• 🎯 Primary goal: Eliminates need to delete memory.db every 2 weeks on Windows
• ⚡ Performance: Concurrent read/write improvements on all platforms via WAL mode
• 🔒 Reliability: Better database lock handling, error resilience, and busy timeout configuration
• 🪟 Windows support: Production-ready SQLite configuration with NullPool for filesystem databases only
• 🧪 Test safety: In-memory test databases work correctly on Windows

All checklist items pass:

Code Quality & Standards

• ✅ Follows Basic Memory's coding conventions in CLAUDE.md
• ✅ Python 3.12+ type annotations and async patterns
• ✅ SQLAlchemy 2.0 best practices
• ✅ FastAPI and Typer conventions followed (N/A for this PR)
• ✅ 100-character line length limit maintained
• ✅ No commented-out code blocks

Testing & Documentation

• ✅ Unit tests for new functions/methods
• ✅ Integration tests for WAL mode and Windows-specific behavior
• ✅ Test coverage for edge cases (including in-memory database handling)
• ✅ Documentation updated (docstrings, inline comments)
• ✅ CLAUDE.md unchanged (no convention changes)

Basic Memory Architecture

• ✅ Atomic, composable design preserved
• ✅ No database schema changes (connection-level only)
• ✅ Preserves local-first architecture principles
• ✅ Knowledge graph operations maintain consistency
• ✅ Markdown file handling preserves integrity
• ✅ AI-human collaboration patterns followed

Security & Performance

• ✅ No hardcoded secrets or credentials
• ✅ Input validation for SQLite PRAGMAs
• ✅ Proper error handling and logging
• ✅ Performance considerations addressed (WAL mode, caching, etc.)
• ✅ No sensitive data in logs or commits

Recommendation

This PR is ready to merge. All concerns from previous reviews have been addressed, the latest commit fixes a critical NullPool bug for in-memory databases, and the implementation is production-ready with comprehensive test coverage.

The iterative development process demonstrated here—with 6 commits progressively improving the implementation—shows excellent collaborative problem-solving between human and AI contributors.

---

[phernandez]

fixes #158