feat: migrate PostgreSQL to SQLite#95
Conversation
- Replace asyncpg with aiosqlite for all storage modules - Add SqlStorage base class with helper methods - Update container to use single connection with shared lock - Enable WAL mode for better SQLite concurrency - Remove Postgres wait logic from entrypoint - Update CI/CD to remove DATABASE_URL secret - Add tests with real in-memory SQLite databases
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the application's persistent storage from PostgreSQL to SQLite using aiosqlite. Key changes include the introduction of a generic SqlStorage base class, updates to the AppContainer for connection and lock management, and the refactoring of all storage-backed tools. Feedback highlights a critical bug where UPDATE statements in the reminder storage will fail because they use a helper method that strictly requires a lastrowid, which SQLite only provides for INSERTs. It is recommended to refactor this helper for more generic execution and to consolidate duplicated query logic in the reminder service.
| await self._execute( | ||
| "UPDATE reminders SET is_sent = 1 WHERE id = ?", | ||
| (reminder_id,), | ||
| ) |
There was a problem hiding this comment.
This UPDATE operation will fail with a RuntimeError because it uses the _execute helper, which expects a lastrowid to be returned. In SQLite, UPDATE statements do not populate lastrowid. You should use self._conn.execute directly within the shared lock, similar to the implementation of delete_reminder.
| await self._execute( | |
| "UPDATE reminders SET is_sent = 1 WHERE id = ?", | |
| (reminder_id,), | |
| ) | |
| async with self._lock: | |
| await self._conn.execute( | |
| "UPDATE reminders SET is_sent = 1 WHERE id = ?", | |
| (reminder_id,), | |
| ) | |
| logger.info("Marked reminder %s as sent (SQLite)", reminder_id) |
| await self._execute( | ||
| """ | ||
| UPDATE reminders | ||
| SET trigger_time = $1, is_sent = FALSE | ||
| WHERE id = $2 | ||
| SET trigger_time = ?, is_sent = 0 | ||
| WHERE id = ? | ||
| """, | ||
| next_trigger_time, | ||
| reminder_id, | ||
| (next_trigger_time, reminder_id), | ||
| ) |
There was a problem hiding this comment.
Similar to mark_sent, this UPDATE query will trigger a RuntimeError due to the lastrowid check in the _execute helper. It must be refactored to use the connection directly with the write lock.
| await self._execute( | |
| """ | |
| UPDATE reminders | |
| SET trigger_time = $1, is_sent = FALSE | |
| WHERE id = $2 | |
| SET trigger_time = ?, is_sent = 0 | |
| WHERE id = ? | |
| """, | |
| next_trigger_time, | |
| reminder_id, | |
| (next_trigger_time, reminder_id), | |
| ) | |
| async with self._lock: | |
| await self._conn.execute( | |
| """ | |
| UPDATE reminders | |
| SET trigger_time = ?, is_sent = 0 | |
| WHERE id = ? | |
| """, | |
| (next_trigger_time, reminder_id), | |
| ) | |
| logger.info( | |
| "Rescheduled recurring reminder %s for %s (SQLite)", | |
| reminder_id, | |
| next_trigger_time, | |
| ) |
| async def _execute( | ||
| self, | ||
| query: str, | ||
| params: tuple[Any, ...] = (), | ||
| *, | ||
| use_lock: bool = True, | ||
| ) -> int: | ||
| """Execute a write query and return lastrowid. | ||
|
|
||
| Args: | ||
| query: SQL query with ? placeholders. | ||
| params: Query parameters. | ||
| use_lock: Whether to acquire the write lock. | ||
|
|
||
| Returns: | ||
| The last inserted row ID. | ||
|
|
||
| Raises: | ||
| RuntimeError: If lastrowid is None after insert. | ||
| """ | ||
| if use_lock: | ||
| async with self._lock: # noqa: SIM117 | ||
| async with self._conn.execute(query, params) as cursor: | ||
| if cursor.lastrowid is None: | ||
| raise RuntimeError("Failed to get lastrowid after insert") | ||
| return cursor.lastrowid | ||
| else: | ||
| async with self._conn.execute(query, params) as cursor: | ||
| if cursor.lastrowid is None: | ||
| raise RuntimeError("Failed to get lastrowid after insert") | ||
| return cursor.lastrowid | ||
|
|
There was a problem hiding this comment.
The _execute helper is strictly designed for INSERT operations that return an auto-incrementing ID. The explicit check for cursor.lastrowid and the RuntimeError on line 96 (and 101) make this method unsuitable for UPDATE or DELETE statements, which do not set lastrowid in SQLite.
Currently, this is causing critical bugs in SqliteReminderStorage where UPDATE queries are being passed to this helper. Consider renaming this to _insert and providing a more generic _execute method that returns the cursor or rowcount.
| query = """ | ||
| SELECT | ||
| id, | ||
| user_id, | ||
| message, | ||
| trigger_time, | ||
| is_sent, | ||
| recurrence_rule, | ||
| recurrence_text, | ||
| timezone_name, | ||
| created_at | ||
| FROM reminders WHERE user_id = ? | ||
| ORDER BY trigger_time ASC | ||
| """ | ||
| rows = await self._fetch_all(query, (user_id,)) | ||
| else: | ||
| query = """ | ||
| SELECT | ||
| id, | ||
| user_id, | ||
| message, | ||
| trigger_time, | ||
| is_sent, | ||
| recurrence_rule, | ||
| recurrence_text, | ||
| timezone_name, | ||
| created_at | ||
| FROM reminders WHERE user_id = ? AND is_sent = 0 | ||
| ORDER BY trigger_time ASC | ||
| """ | ||
| rows = await self._fetch_all(query, (user_id,)) |
There was a problem hiding this comment.
The query logic is heavily duplicated between the two branches of the if include_sent condition. This can be refactored into a single query construction to improve maintainability and reduce the risk of schema mismatches in future updates.
query = """
SELECT
id, user_id, message, trigger_time, is_sent,
recurrence_rule, recurrence_text, timezone_name, created_at
FROM reminders WHERE user_id = ?
"""
params: list[Any] = [user_id]
if not include_sent:
query += " AND is_sent = 0"
query += " ORDER BY trigger_time ASC"
rows = await self._fetch_all(query, tuple(params))- Fix mark_sent to use direct execute with lock - Fix reschedule_reminder to use direct execute with lock - Fix preferences.set to use direct execute with lock - Refactor get_user_reminders to reduce query duplication The _execute helper raises RuntimeError for UPDATE/DELETE statements because lastrowid is None for non-INSERT operations.
1 participant
What
Replace PostgreSQL with SQLite for all storage modules (reminders, calories, workouts, preferences). This eliminates PostgreSQL as an external service dependency, aligning with the project philosophy of running on cheap self-hosted infrastructure.
Why
tools.dbfile in the same volume as ADK sessions is trivial to back upHow
src/blacki/storage/sqlite.py— SQLite connection manager with WAL modesrc/blacki/storage/base.py—SqlStoragebase class with_execute,_fetch_one,_fetch_allhelpersSqliteReminderStorage,SqliteCalorieStorage,SqliteWorkoutStorage,SqlitePreferencesStorage)container.py— Singleaiosqlite.Connectionwith sharedasyncio.Lockserver.py— UseSQLITE_PATH(default:{AGENT_DIR}/.adk/tools.db)asyncpgandgreenletdependencies, addaiosqliteentrypoint.shDATABASE_URLsecretTests
uv run mypy .passes with no errorsuv run ruff checkpassesuv run ruff formatpassesuv run pytest --cov=srcpasses with 100% coverageasyncpgorgreenletimports remain in source codeRelated Issues
Closes #86