perf(db): NativeDatabase napi-rs class for rusqlite connection lifecycle (6.13)#666
perf(db): NativeDatabase napi-rs class for rusqlite connection lifecycle (6.13)#666carlos-alm merged 4 commits intomainfrom
Conversation
…fecycle (6.13) Foundation for moving all DB operations to rusqlite on the native engine path. Creates a persistent rusqlite::Connection holder exposed to JS, handling schema migrations and build metadata KV — eliminating redundant per-call connection open/close in the native build pipeline.
|
Claude finished @carlos-alm's task —— View job PR Review: NativeDatabase napi-rs Class ImplementationReviewing this PR according to the codegraph review framework: Progress
Phase 1: Root Cause Analysis ✅Problem Being Solved: This PR addresses the dual-SQLite-in-one-process inefficiency by creating a foundation for migrating all database operations to rusqlite on the native engine path. Currently, both the native Rust engine and JavaScript use separate SQLite connections (rusqlite + better-sqlite3), creating potential consistency issues and performance overhead. Why This Approach: The PR implements a strategic foundation approach rather than a big-bang migration. It introduces Risk Assessment: Low risk - this is an additive change that maintains full backward compatibility. The native database is only used when available and gracefully falls back to the existing better-sqlite3 path on any errors. Phase 2: Configuration Change Detection ✅No High-Risk Configuration Changes Detected:
Phase 3: Test Integrity ✅No Test Integrity Issues:
Phase 4: Code Quality Review ✅Rust Code Quality (native_db.rs:1-565):
Schema Migration Mirroring (lines 21-267):
TypeScript Integration (pipeline.ts:50-52, 78-89, 194-195):
Interface Contract (types.ts):
Minor Observations:
Phase 5: Backlog Compliance Assessment ✅Backlog Compliance
Final Assessment |
Greptile SummaryThis PR introduces Key implementation points:
Confidence Score: 4/5Safe to merge for Phase 6.13 scope; the single remaining finding is a misleading doc comment, not a runtime defect All prior P1 concerns (migration atomicity, pragma safety, schema_version determinism) are resolved. The one remaining finding is a doc comment on open_read_write that claims parent-directory creation but doesn't implement it — a P2 documentation inaccuracy. In production the directory always pre-exists (openDb runs first), so no runtime failure is expected. Score is 4 rather than 5 only to flag the comment fix before this API is used standalone in 6.14+. crates/codegraph-core/src/native_db.rs — doc comment on open_read_write should either add std::fs::create_dir_all or remove the directory-creation promise Important Files Changed
Sequence DiagramsequenceDiagram
participant P as pipeline.ts (setupPipeline)
participant BS as better-sqlite3 (ctx.db)
participant NDB as NativeDatabase (ctx.nativeDb)
participant F as finalize.ts
P->>BS: openDb(dbPath) — always opened
P->>NDB: NativeDatabase.openReadWrite(dbPath) [native only]
alt native available
NDB-->>P: connection opened
P->>NDB: initSchema() — runs all 16 migrations
else native unavailable or error
P->>BS: initSchema(ctx.db) — JS fallback
end
Note over P,F: Pipeline stages run (queries still via better-sqlite3)
F->>NDB: getBuildMeta('node_count') [native]
F->>NDB: getBuildMeta('edge_count') [native]
F->>NDB: setBuildMeta([engine, schema_version, built_at, ...]) [native]
F->>NDB: close() — NativeDatabase closed first
F->>BS: closeDb(ctx.db) — better-sqlite3 closed second
Reviews (2): Last reviewed commit: "fix(build): remove redundant String() co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| // Legacy column compat — add columns that may be missing from pre-migration DBs. | ||
| // Mirrors the post-migration block in src/db/migrations.ts initSchema(). | ||
| if has_table(conn, "nodes") { | ||
| if !has_column(conn, "nodes", "end_line") { | ||
| let _ = conn.execute_batch("ALTER TABLE nodes ADD COLUMN end_line INTEGER"); | ||
| } | ||
| if !has_column(conn, "nodes", "role") { | ||
| let _ = conn.execute_batch("ALTER TABLE nodes ADD COLUMN role TEXT"); | ||
| } | ||
| let _ = conn.execute_batch("CREATE INDEX IF NOT EXISTS idx_nodes_role ON nodes(role)"); | ||
| if !has_column(conn, "nodes", "parent_id") { | ||
| let _ = conn.execute_batch( | ||
| "ALTER TABLE nodes ADD COLUMN parent_id INTEGER REFERENCES nodes(id)", | ||
| ); | ||
| } | ||
| let _ = conn | ||
| .execute_batch("CREATE INDEX IF NOT EXISTS idx_nodes_parent ON nodes(parent_id)"); |
There was a problem hiding this comment.
Migrations that contain multiple ALTER TABLE statements (e.g. v9 with 14 ALTER TABLE statements, v11, v14, and v15) are executed via conn.execute_batch(migration.up) without an enclosing transaction. When SQLite is not inside an explicit transaction, every statement is individually auto-committed.
If the process crashes or rusqlite returns an error partway through migration v9 (e.g. after 7 of 14 ALTER TABLE statements), those 7 columns are already committed to disk but schema_version is still 8. The next run will attempt to re-apply all 14 ALTER TABLE statements and will fail immediately with "duplicate column name: loc" (or whichever column was already added). This leaves the database permanently unrunnable — the user must delete the entire .codegraph/graph.db file.
The TypeScript counterpart (db.exec(migration.up)) has the same flaw, but this is a brand-new Rust implementation and the fix is straightforward with rusqlite:
for migration in MIGRATIONS {
if migration.version > current_version {
let tx = conn
.unchecked_transaction()
.map_err(|e| napi::Error::from_reason(format!("begin migration tx failed: {e}")))?;
tx.execute_batch(migration.up).map_err(|e| {
napi::Error::from_reason(format!("migration v{} failed: {e}", migration.version))
})?;
tx.execute(
"UPDATE schema_version SET version = ?1",
params![migration.version],
)
.map_err(|e| {
napi::Error::from_reason(format!("update schema_version failed: {e}"))
})?;
tx.commit().map_err(|e| {
napi::Error::from_reason(format!("commit migration v{} failed: {e}", migration.version))
})?;
current_version = migration.version;
}
}Note: SQLite DDL is fully transactional (unlike PostgreSQL), so wrapping ALTER TABLE + UPDATE schema_version in a single transaction is both correct and idempotent on retry.
There was a problem hiding this comment.
Fixed — each migration is now wrapped in an explicit conn.unchecked_transaction() / tx.commit() block, so DDL + schema_version update are atomic. A crash mid-migration rolls back cleanly and the next run retries the same version.
| } | ||
| } | ||
|
|
||
| /// Run all schema migrations. Mirrors `initSchema()` from `src/db/migrations.ts`. | ||
| #[napi] | ||
| pub fn init_schema(&self) -> napi::Result<()> { | ||
| let conn = self.conn()?; | ||
|
|
||
| conn.execute_batch( | ||
| "CREATE TABLE IF NOT EXISTS schema_version (version INTEGER NOT NULL DEFAULT 0)", | ||
| ) | ||
| .map_err(|e| napi::Error::from_reason(format!("create schema_version failed: {e}")))?; | ||
|
|
||
| let mut current_version: u32 = conn | ||
| .query_row("SELECT version FROM schema_version", [], |row| row.get(0)) | ||
| .unwrap_or(0); | ||
|
|
||
| // Insert version 0 if table was just created (empty) | ||
| let count: u32 = conn | ||
| .query_row("SELECT COUNT(*) FROM schema_version", [], |row| row.get(0)) |
There was a problem hiding this comment.
pragma() accepts write PRAGMAs without restriction
The sql argument is directly interpolated into PRAGMA {sql} and passed to conn.prepare(). While prepare() prevents multi-statement injection (SQLite rejects statements with a semicolon), it still allows callers to pass write-mode PRAGMAs such as:
nativeDb.pragma('journal_mode = DELETE') // switches off WAL for both connections
nativeDb.pragma('wal_autocheckpoint = 0') // disables WAL checkpointing
nativeDb.pragma('user_version = 99') // corrupts schema version signalSince this mirrors the better-sqlite3 pragma API and the callers are all trusted internal code today, this is not an immediate security issue. But as more callers are added in Phase 6.14+, a write-PRAGMA that silently downgrades journal_mode while ctx.db (better-sqlite3) is open on the same file could cause subtle corruption or lock errors.
Consider adding a guard that rejects setter syntax:
if sql.contains('=') || sql.to_ascii_lowercase().contains("checkpoint") {
return Err(napi::Error::from_reason(
"pragma(): use exec() for write PRAGMAs",
));
}Or at minimum document the write-semantics expectation in the method's doc-comment.
There was a problem hiding this comment.
Fixed — added doc-comment clarifying that pragma() is intended for read-only PRAGMAs and that write-mode PRAGMAs should use exec() instead. Since all callers are trusted internal code today, a runtime guard would add overhead without benefit, but the documentation will prevent misuse as more callers are added in Phase 6.14+.
| } | ||
|
|
||
| for migration in MIGRATIONS { | ||
| if migration.version > current_version { | ||
| conn.execute_batch(migration.up).map_err(|e| { | ||
| napi::Error::from_reason(format!( | ||
| "migration v{} failed: {e}", | ||
| migration.version | ||
| )) | ||
| })?; | ||
| conn.execute( | ||
| "UPDATE schema_version SET version = ?1", | ||
| params![migration.version], | ||
| ) | ||
| .map_err(|e| { | ||
| napi::Error::from_reason(format!("update schema_version failed: {e}")) | ||
| })?; | ||
| current_version = migration.version; | ||
| } |
There was a problem hiding this comment.
schema_version table has no UNIQUE constraint and allows multiple rows
The schema_version table is created as:
CREATE TABLE IF NOT EXISTS schema_version (version INTEGER NOT NULL DEFAULT 0)There is no PRIMARY KEY or UNIQUE constraint, so multiple rows can accumulate. The migration loop uses:
conn.query_row("SELECT version FROM schema_version", [], |row| row.get(0))
.unwrap_or(0);which returns an arbitrary row if more than one exists (SQLite does not guarantee row order without ORDER BY). In a healthy DB there will only ever be one row, but a partial run that inserted before crashing could create duplicates.
The TypeScript version has the same issue, but a minimal guard would help:
conn.query_row(
"SELECT version FROM schema_version ORDER BY rowid DESC LIMIT 1",
[],
|row| row.get(0),
)
.unwrap_or(0);There was a problem hiding this comment.
Fixed — the schema_version query now uses ORDER BY rowid DESC LIMIT 1 to deterministically return the latest version even if duplicate rows exist.
…chema (#666) - Fix E0507 in close(): use (*self.conn).take() to deref through SendWrapper and call Option::take instead of SendWrapper::take which consumes self - Wrap each migration in an explicit transaction so a mid-migration crash rolls back cleanly instead of leaving the DB in an unrecoverable state - Add ORDER BY rowid DESC LIMIT 1 to schema_version query to handle potential duplicate rows deterministically - Document pragma() as read-only with guidance to use exec() for write PRAGMAs
All values in the metadata object are already strings (explicitly coerced via String() in the object literal), so the outer .map() String() wrapper was a no-op.
|
Addressed all Greptile review feedback:
|
|
Claude finished @carlos-alm's task —— View job PR Review: NativeDatabase Implementation After Greptile FixesTodo List
Review SummaryAll Greptile Issues Successfully Addressed ✅ P1 Migration Atomicity Fix: ✅ VERIFIED P2 Schema Version Uniqueness: ✅ VERIFIED P2 Pragma Write Semantics: ✅ VERIFIED P2 Redundant String(): ✅ VERIFIED Architecture & Code Quality AssessmentSecurity & Safety: ✅ EXCELLENT
Resource Management: ✅ ROBUST
TypeScript Integration: ✅ EXCELLENT
Schema Migration Integrity: ✅ VERIFIED
Performance Design: ✅ STRATEGIC
Minor Observations
Final RecommendationRating: ⭐⭐⭐⭐⭐ (5/5) - APPROVE This is exemplary foundation work. All critical issues identified by Greptile have been properly resolved. The implementation demonstrates excellent security practices, robust error handling, and thoughtful architectural design that enables future native migration phases. The code quality is high, TypeScript integration is seamless, and the dual-engine fallback strategy ensures zero risk to existing functionality. Ready to merge - this provides a solid foundation for Phase 6.14+ native database migration while maintaining complete backward compatibility. |
1 participant
Summary
NativeDatabasenapi-rs class incrates/codegraph-core/src/native_db.rsholding a persistentrusqlite::Connectionwith factory methods (openReadWrite/openReadonly), lifecycle (close/exec/pragma), schema migrations (initSchemawith all 16 migrations embedded), and build metadata KV (getBuildMeta/setBuildMeta)NativeDatabasehandles schema init and metadata reads/writes —better-sqlite3remains open for queries and stages not yet migratedTest plan
rustfmtcheckcargo buildcompilesnative_db.rs(local Windows linker env issue prevents local verification)codegraph build --engine nativeuses NativeDatabase for schema+metadatacodegraph build --engine wasmunchanged behavior (falls back to better-sqlite3)codegraph buildtwice — second is incremental, no schema errors