fix(db): standardize SQLite pragmas across all connections#58
fix(db): standardize SQLite pragmas across all connections#58serrrfirat wants to merge 1 commit into
Conversation
getSynapseDb() now configures WAL mode, foreign keys, and busy_timeout, eliminating inconsistent behavior between connections to the same DB file. job-fsm.ts reuses getSynapseDb() instead of duplicating setup. Screenpipe read-only connection gets busy_timeout to prevent spurious lock errors. Closes #10 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @serrrfirat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and consistency of SQLite database interactions by centralizing pragma configurations. It ensures that all connections to the main Synapse database adhere to a standardized set of settings, reducing potential concurrency issues and simplifying database management. Additionally, it addresses specific Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe changes standardize SQLite PRAGMA configurations (WAL mode, foreign key enforcement, busy timeouts) across multiple database initialization entry points and consolidate job-fsm to use a shared database provider instead of creating a local instance. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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 does a great job of centralizing and standardizing SQLite PRAGMA settings across different parts of the application, which improves consistency and maintainability. The changes in job-fsm.ts to use the shared getSynapseDb function are a good example of this. I've added a few comments with suggestions to further improve the implementation, including adding a missing pragma for consistency, and replacing magic numbers with constants.
| db.exec('PRAGMA journal_mode = WAL'); | ||
| db.exec('PRAGMA busy_timeout = 5000'); |
There was a problem hiding this comment.
For consistency with getSynapseDb, you should also set PRAGMA foreign_keys = ON. Omitting it could lead to inconsistent database behavior, especially during a migration. You can also combine these into a single exec call for conciseness.
| db.exec('PRAGMA journal_mode = WAL'); | |
| db.exec('PRAGMA busy_timeout = 5000'); | |
| db.exec('PRAGMA journal_mode = WAL; PRAGMA foreign_keys = ON; PRAGMA busy_timeout = 5000;'); |
| export function getScreenpipeDb(): Database { | ||
| return new Database(SCREENPIPE_DB_PATH, { readonly: true }); | ||
| const db = new Database(SCREENPIPE_DB_PATH, { readonly: true }); | ||
| db.exec('PRAGMA busy_timeout = 3000'); |
There was a problem hiding this comment.
To avoid using a magic number, consider defining 3000 as a constant at the module level, for example: const BUSY_TIMEOUT_MS = 3000;. This improves readability and maintainability by making the value's purpose clear and centralizing its definition.
| db.exec('PRAGMA journal_mode = WAL'); | ||
| db.exec('PRAGMA foreign_keys = ON'); | ||
| db.exec('PRAGMA busy_timeout = 5000'); |
There was a problem hiding this comment.
You can combine these PRAGMA statements into a single exec call for conciseness. Additionally, consider defining 5000 as a constant to avoid a magic number, improving readability and maintainability.
| db.exec('PRAGMA journal_mode = WAL'); | |
| db.exec('PRAGMA foreign_keys = ON'); | |
| db.exec('PRAGMA busy_timeout = 5000'); | |
| db.exec('PRAGMA journal_mode = WAL; PRAGMA foreign_keys = ON; PRAGMA busy_timeout = 5000;'); |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Summary
Clean, low-risk PR that correctly centralizes SQLite pragma configuration. The core changes are sound: getSynapseDb() now sets WAL mode, foreign keys, and busy_timeout in one place, and job-fsm.ts properly delegates to it instead of duplicating the setup. The busy_timeout addition to the Screenpipe read-only connection is a good defensive improvement. The only substantive finding is that the migration script doesn't fully align with the standardization goal (missing PRAGMA foreign_keys = ON), though this may be intentional. Overall, this is a well-scoped improvement with minimal risk.
| console.log(); | ||
|
|
||
| const db = new Database(DB_PATH); | ||
| db.exec('PRAGMA journal_mode = WAL'); |
There was a problem hiding this comment.
The PR's stated goal is to standardize SQLite pragmas across all connections. The centralized getSynapseDb() sets journal_mode=WAL, foreign_keys=ON, and busy_timeout=5000. The migration script adds WAL and busy_timeout but omits foreign_keys=ON. This means FK constraints are not enforced during migration inserts — specifically, the INSERT INTO job_transitions with a job_id FK reference to jobs.id is not validated by SQLite. If lastInsertRowid returned an unexpected value, the orphaned transition row would not be caught. This may be intentional (to avoid ordering issues during migration), but it's inconsistent with the PR's purpose and undocumented.
| export function getScreenpipeDb(): Database { | ||
| return new Database(SCREENPIPE_DB_PATH, { readonly: true }); | ||
| const db = new Database(SCREENPIPE_DB_PATH, { readonly: true }); | ||
| db.exec('PRAGMA busy_timeout = 3000'); |
There was a problem hiding this comment.
The Screenpipe read-only connection uses busy_timeout=3000 (3s) while all Synapse DB connections use busy_timeout=5000 (5s). The difference is reasonable (read-only connections contending with Screenpipe writes need less patience than read-write connections contending with daemon concurrency), but the rationale is not documented anywhere. A future contributor might see the inconsistency and either 'fix' it incorrectly or be confused by it.
| export function getSynapseDb(): Database { | ||
| ensureSynapseDir(); | ||
| const db = new Database(SYNAPSE_DB_PATH); | ||
| db.exec('PRAGMA journal_mode = WAL'); |
There was a problem hiding this comment.
WAL mode is a persistent database property — once set, it survives connection close/reopen. Calling PRAGMA journal_mode = WAL on every getSynapseDb() invocation is harmless but redundant after the first call. Since getSynapseDb() is called ~30 times across index.ts and api-server.ts (each creating a new connection), this pragma is executed ~30 times unnecessarily per daemon lifetime. The overhead is negligible (SQLite returns early if already in WAL), but it's worth noting as a minor inefficiency. The foreign_keys and busy_timeout pragmas are per-connection and must be set every time — those are correct.
1 participant
Summary
getSynapseDb()(WAL mode, foreign keys, busy_timeout) so all connections to~/.synapse/db.sqlitebehave consistentlyjob-fsm.tsby reusinggetSynapseDb()instead of manually creating a connection with duplicate pragma setupbusy_timeoutto Screenpipe read-only connection to prevent spuriousSQLITE_BUSYerrors when Screenpipe is writingTest plan
tsc --noEmitpasses with zero errorsbun test)bun run src/index.ts doctorreports healthy DBbun run src/index.ts statusshows no regressionsCloses #10
🤖 Generated with Claude Code
Summary by CodeRabbit