Improve SQLite connection stability with timeout, WAL mode, and row_factory #866
Conversation
📝 WalkthroughWalkthroughDatabase connection configuration is enhanced to improve concurrency and error resilience. A 30-second timeout is added to the connection, row factory is configured to return Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/database/connection.py (1)
28-29: Consider WAL mode persistence and deployment implications.WAL mode is persistent—once enabled, it remains active even after the connection closes and affects all future connections to this database. Consider:
- Filesystem compatibility: WAL requires a filesystem that supports shared memory; it won't work on network filesystems (e.g., NFS).
- Additional files: WAL creates
-waland-shmfiles alongside the database.- Backup implications: Backups should use
PRAGMA wal_checkpointor backup tools that handle WAL.Ensure your deployment environment and backup strategy accommodate WAL mode.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/connection.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 570
File: backend/app/database/connection.py:16-24
Timestamp: 2025-10-31T17:00:50.132Z
Learning: In PictoPy backend, the user prefers not to use database connection retry logic or extended busy timeouts in the centralized get_db_connection() context manager, even though the app has concurrent access patterns via ProcessPoolExecutor and FastAPI.
🔇 Additional comments (3)
backend/app/database/connection.py (3)
1-38: Critical mismatch between PR changes and linked issue.The PR description states this targets issue #856, which is about fixing a "Home" button navigation bug in the navbar. However, all changes in this PR are database connection improvements with no UI or navigation code. This suggests either:
- The wrong issue number is linked, or
- This PR should be split and the database changes should target a different issue
Please verify the correct issue number and update the PR description accordingly.
16-16: Timeout contradicts previous preference.The 30-second timeout contradicts a previous preference documented in the learnings, where you indicated not wanting extended busy timeouts in this connection manager, even with concurrent access patterns.
Has your approach changed, or should this be reconsidered?
Based on learnings from PR #570 on the same file.
18-19: The row_factory change is safe for this codebase and will not cause breaking changes.While tuple unpacking patterns exist in
backend/app/database/face_clusters.pyandbackend/app/database/faces.py(e.g.,cluster_id, cluster_name, face_count, face_image_base64 = row), these files use directsqlite3.connect()rather thanget_db_connection(), so they are unaffected by the row_factory change.Code that actually uses
get_db_connection()(likebackend/app/database/albums.pyandbackend/app/utils/face_clusters.py) consistently uses compatible patterns:
- Integer indexing:
row[0]- List comprehensions:
[row[0] for row in ...]No tuple unpacking occurs in files using
get_db_connection(), so this change introduces no breaking changes.Likely an incorrect or invalid review comment.
2 participants
Summary
This PR improves the robustness and reliability of the SQLite database connection by configuring important connection settings that were previously missing. These changes help prevent avoidable runtime issues under concurrent access and improve overall developer experience.
What changed
The following improvements were made in
get_db_connection():timeoutto reducedatabase is lockederrors under concurrent accessrow_factoryto returnsqlite3.Rowobjects for safer and more readable query resultsWhy this change is important
SQLite is sensitive to concurrent access patterns. Without these settings:
Making these configurations explicit hardens the database layer and improves stability as the application scales.
Testing
PRAGMA journal_modesqlite3.RowChecklist
row_factoryRelated Issue
Fixes #863
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.