perf(native): defer NativeDatabase.openReadWrite until after change detection

[carlos-alm]

Summary

• Defer NativeDatabase.openReadWrite + initSchema from setupPipeline to tryNativeOrchestrator, saving ~60ms on every incremental build invocation
• Setup now always uses better-sqlite3 (~4ms) for the initial metadata reads (schema mismatch check)
• Native DB opens on-demand only when the Rust orchestrator confirms files need rebuilding
• No-op builds exit before ever opening the native connection

Closes #934

Test plan

• Integration tests pass (564/564)
• Structure tests pass (9/9)
• Verify with codegraph build --engine native --timing — setup phase should drop from ~67ms to ~4ms on incremental builds
• Verify no-op builds (codegraph build with no changes) exit cleanly without opening native connection
• Verify full builds still work correctly (native DB opened on demand)

[claude]

Claude finished @carlos-alm's task —— View job

---

Root Cause Analysis

Problem Being Solved: Setup phase was eagerly opening a NativeDatabase connection (~60ms) before knowing if any files changed. On no-op incremental builds, this expensive operation was pure waste since the build would exit early after change detection.

Why This Approach: Deferring NativeDatabase.openReadWrite until tryNativeOrchestrator is the optimal solution because:

1. Change detection only needs better-sqlite3 for metadata reads (~4ms)
2. No-op builds exit before ever needing the native connection
3. When native orchestrator runs, it opens the connection on-demand
4. Preserves all existing functionality while optimizing the critical path

Risk Assessment: Low risk. This is a pure optimization that maintains identical behavior while reducing wasted work. All error handling and fallback paths are preserved.

Backlog Compliance

• Zero-dep: ✓ — No new dependencies, builds on existing tree-sitter/SQLite/native infrastructure
• Foundation-aligned: ✓ — Directly supports P1 (always-current graph) by making incremental builds faster, enabling more frequent rebuilds
• Problem-fit: 4 — Improves agent experience by reducing rebuild latency, but doesn't directly address the core "lost AI" problem
• Breaking: No — Purely internal optimization, no API/CLI/behavior changes
• Tier: 1

Code Quality Assessment

✅ Proper database lifecycle management - Correctly closes better-sqlite3 before opening rusqlite
✅ Advisory locking - Maintains lock acquisition/release to prevent corruption
✅ Error handling - Graceful fallback to JS pipeline on native DB setup failure
✅ Proxy pattern - Correctly sets up NativeDbProxy for post-native JS phases
✅ Resource cleanup - All cleanup paths preserved, no resource leaks
✅ Comments - Clear explanation of the deferral strategy and performance benefit

The 60-line reduction comes from moving the native DB setup logic from setupPipeline() to tryNativeOrchestrator(), where it logically belongs.

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE WITH CONDITIONS
• Reasoning: Excellent performance optimization that aligns with core principles. Clean implementation with proper error handling. Minor deduction for incomplete test plan verification.

Conditions for approval:

1. ✅ Integration tests passing (564/564)
2. ✅ Structure tests passing (9/9)
3. ⚠️ Manually verify timing claims with codegraph build --engine native --timing
4. ⚠️ Test no-op build behavior to confirm early exit works correctly

This PR directly supports Foundation principle P1 (always-current graph) by making incremental builds significantly faster, enabling more frequent rebuilds without user friction.

[github-actions]

Codegraph Impact Analysis

2 functions changed → 6 callers affected across 5 files

• setupPipeline in src/domain/graph/builder/pipeline.ts:127 (6 transitive callers)
• tryNativeOrchestrator in src/domain/graph/builder/pipeline.ts:562 (5 transitive callers)

[greptile-apps]

Greptile Summary

This PR defers NativeDatabase.openReadWrite + initSchema from setupPipeline to tryNativeOrchestrator, so setup always uses the cheap better-sqlite3 path (~4ms) and the ~60ms native open is paid only when the Rust orchestrator confirms work to do. No-op incremental builds exit before the native connection is ever opened. Previously raised concerns about the advisory-lock transfer (ctx.db.close() intentionally not using closeDb()) and the defensive ctx.nativeFirstProxy = false reset are now documented with inline comments.

Confidence Score: 5/5

Safe to merge — focused optimization with no new correctness issues and prior concerns fully addressed.

All prior P1 concerns (advisory-lock transfer semantics and defensive flag reset) have been resolved with clarifying inline comments. No new P0/P1 issues were found: advisory-lock acquire/release sequencing is consistent across the happy path, error-recovery, and no-op exit; schema initialization via better-sqlite3 is persisted before any raw close so the native connection reopens to a fully-migrated DB; the NativeDbProxy lifecycle (close() no-op, __lockPath release via closeDbPair) is correctly threaded through every exit path. Remaining observations are P2 or below.

No files require special attention.

Important Files Changed

Filename	Overview
src/domain/graph/builder/pipeline.ts	Defers NativeDatabase.openReadWrite + initSchema from setupPipeline to tryNativeOrchestrator; always uses better-sqlite3 (~4ms) for setup/metadata reads, opening native only when Rust orchestrator confirms files need rebuilding. Advisory-lock transfer and defensive flag reset are documented with inline comments addressing prior review concerns.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[buildGraph] --> B[setupPipeline\nopenDb better-sqlite3 ~4ms\ninitSchema\ncheckEngineSchemaMismatch]
    B --> C{tryNativeOrchestrator}
    C -->|shouldSkip| D[return undefined\nfall through to JS pipeline]
    C -->|nativeAvailable| E[ctx.db.close raw\nacquireAdvisoryLock\nNativeDatabase.openReadWrite ~60ms\nnativeDb.initSchema\nNativeDbProxy → ctx.db\nnativeFirstProxy = true]
    E -->|open failed| F[catch: releaseAdvisoryLock\nctx.db = openDb\nnativeFirstProxy = false\nfall through to JS pipeline]
    E -->|open success| G[nativeDb.buildGraph]
    G -->|earlyExit| H[closeDbPair\nreturn early-exit\nno-op build ✓]
    G -->|build complete| I[setBuildMeta\nrunPostNativeStructure?\nrunPostNativeAnalysis?]
    I --> J[closeDbPair\nreturn NativeTimingResult]
    D --> K[runPipelineStages\ncollectFiles / detectChanges]
    F --> K
    K -->|no changes| L[earlyExit]
    K -->|changes| M[parseFiles / insertNodes\nresolveImports / buildEdges\nbuildStructure / runAnalyses\nfinalize]

Loading

_{Reviews (2): Last reviewed commit: "fix: clarify advisory-lock transfer and ..." | Re-trigger Greptile}

[greptile-apps]

Redundant flag reset in error-recovery branch

ctx.nativeFirstProxy = false is set in the catch block, but true is only written on line 588 — which comes after the lines that can throw (openReadWrite, initSchema, and the three proxy assignments above it). If any of those lines throws, the assignment ctx.nativeFirstProxy = true was never reached, so resetting to false here is a no-op. The comment or the assignment should clarify whether this is a defensive guard (in case the ordering ever changes) or dead code.

[carlos-alm]

Fixed — added inline comment clarifying this is a defensive guard in case future refactors move the ctx.nativeFirstProxy = true assignment above the throwing lines.

[greptile-apps]

Advisory-lock ownership gap between close() and proxy creation

ctx.db.close() calls the raw db.close() method, not the closeDb() helper, so the advisory lock that openDb wrote to ctx.db.__lockPath is not released. The lock file continues to hold the current PID. The very next line (acquireAdvisoryLock) then re-writes the same PID — which is a no-op in practice — and the proxy captures the path again.

This is functionally correct (lock is released exactly once when closeDbPair calls releaseAdvisoryLock(proxy.__lockPath) at the end), but the pattern is subtly different from every other place in the codebase that deliberately keeps the lock across a close/reopen pair. A short inline comment at the close() call site would help future readers understand that skipping closeDb here is intentional — the lock is being transferred to the proxy rather than released.

[carlos-alm]

Fixed — added inline comment at the ctx.db.close() call explaining that raw close() is intentional: the advisory lock is kept and transferred to the NativeDbProxy, not released here.

[carlos-alm]

@greptileai