fix(native): thread config.db.busyTimeoutMs into NativeDatabase open factories#2021
Open
carlos-alm wants to merge 1 commit into
Conversation
…factories NativeDatabase::open_readonly/open_read_write hardcoded PRAGMA busy_timeout = 5000 in the Rust native DB layer, ignoring config.db.busyTimeoutMs entirely (#1763 only wired the TS-side better-sqlite3 pragma). Add an optional busy_timeout_ms param to both factories, defaulting to 5000 when omitted, and thread the already-resolved value through all six JS call sites: openRepo (via openRepoNative/tryOpenRepoNative) and openReadonlyWithNative in db/connection.ts, reopenNativeDb in native-db-lifecycle.ts, the two NativeDatabase.openReadWrite call sites in native-orchestrator.ts, and branch-compare.ts's fan-metrics helper (which previously did no config resolution at all). Impact: 9 functions changed, 47 affected
Contributor
Greptile SummaryThis PR closes the Rust-side gap deferred from #1763 by threading
Confidence Score: 4/5Safe to merge; all 6 changed call sites correctly forward the resolved timeout and the fallback constant mirrors the TypeScript default. The implementation is mechanically straightforward and consistent across all changed files. The threading test covers tests/unit/native-db-busy-timeout-threading.test.ts — only 3 of the 6 changed call sites have threading assertions; src/domain/graph/builder/stages/native-orchestrator.ts and src/features/branch-compare.ts are the uncovered paths. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant TS as TypeScript call site
participant Cfg as resolveDbSettings / resolveBusyTimeoutMs
participant NAddon as NativeAddon (JS bridge)
participant Rust as NativeDatabase (Rust/napi)
participant SQLite as SQLite
TS->>Cfg: resolve busyTimeoutMs from config
Cfg-->>TS: busyTimeoutMs (e.g. 424242)
TS->>NAddon: NativeDatabase.openReadonly(dbPath, busyTimeoutMs)
NAddon->>Rust: open_readonly(db_path, Some(busy_timeout_ms))
Rust->>SQLite: "PRAGMA busy_timeout = 424242"
SQLite-->>Rust: ok
Rust-->>NAddon: NativeDatabase instance
NAddon-->>TS: nativeDb handle
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant TS as TypeScript call site
participant Cfg as resolveDbSettings / resolveBusyTimeoutMs
participant NAddon as NativeAddon (JS bridge)
participant Rust as NativeDatabase (Rust/napi)
participant SQLite as SQLite
TS->>Cfg: resolve busyTimeoutMs from config
Cfg-->>TS: busyTimeoutMs (e.g. 424242)
TS->>NAddon: NativeDatabase.openReadonly(dbPath, busyTimeoutMs)
NAddon->>Rust: open_readonly(db_path, Some(busy_timeout_ms))
Rust->>SQLite: "PRAGMA busy_timeout = 424242"
SQLite-->>Rust: ok
Rust-->>NAddon: NativeDatabase instance
NAddon-->>TS: nativeDb handle
Reviews (1): Last reviewed commit: "fix(native): thread config.db.busyTimeou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+109
to
+121
| it('passes ctx.config.db.busyTimeoutMs to the native factory', () => { | ||
| openReadWriteCalls.length = 0; | ||
| const ctx = new PipelineContext(); | ||
| ctx.dbPath = dbPath; | ||
| ctx.opts = { engine: 'native' }; | ||
| ctx.config = { db: { busyTimeoutMs: CUSTOM_BUSY_TIMEOUT_MS } } as PipelineContext['config']; | ||
|
|
||
| reopenNativeDb(ctx, 'test'); | ||
|
|
||
| expect(openReadWriteCalls).toHaveLength(1); | ||
| expect(openReadWriteCalls[0]?.[1]).toBe(CUSTOM_BUSY_TIMEOUT_MS); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
The threading test asserts that openRepo, openReadonlyWithNative, and reopenNativeDb all forward busyTimeoutMs. However, three changed call sites are not exercised here: openNativeDatabase (~line 1955 in native-orchestrator.ts), runPostNativeAnalysis (~line 579 in native-orchestrator.ts), and openNativeDbForFanMetrics in branch-compare.ts. A future regression in those paths would go unnoticed by this test suite.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Codegraph Impact Analysis9 functions changed → 47 callers affected across 32 files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NativeDatabase::open_readonly()/open_read_write()incrates/codegraph-core/src/db/connection.rshardcodedPRAGMA busy_timeout = 5000, ignoringconfig.db.busyTimeoutMsentirely — the Rust-side gap deferred from #1763 (which wired every TS-sideopenReadonlyOrFail()/better-sqlite3 call site but explicitly left this to a follow-up, since it requires a native rebuild).What changed
open_readonly()/open_read_write()now take an optionalbusy_timeout_ms: Option<u32>parameter, applied via thebusy_timeoutpragma; falls back to a newDEFAULT_BUSY_TIMEOUT_MS = 5000constant when omitted (mirrorsDEFAULTS.db.busyTimeoutMsinsrc/infrastructure/config.ts).napi build --platform --release) and updatedsrc/types.ts'sNativeAddon.NativeDatabaseinterface to match the new signature (crates/codegraph-core/index.d.tsitself is gitignored/generated).src/db/connection.ts:openRepoNative/tryOpenRepoNative(→openRepo()) andopenReadonlyWithNative()— both already hadbusyTimeoutMsin scope viaresolveDbSettings().src/domain/graph/builder/stages/native-db-lifecycle.ts:reopenNativeDb()— build-path call site, usesctx.config.db.busyTimeoutMs.src/domain/graph/builder/stages/native-orchestrator.ts: bothopenReadWrite()call sites — samectx.config.db.busyTimeoutMspattern.src/features/branch-compare.ts:openNativeDbForFanMetrics()previously did no config resolution at all — added aresolveBusyTimeoutMs(dbPath)call, following the pattern db.busyTimeoutMs: extend config-driven wiring to read-only query paths and Rust connection.rs #1763 established for ad-hoc call sites.Out of scope (filed separately)
While verifying the fix I found two pre-existing, unrelated bugs and filed them rather than fixing them here:
NativeDatabase.pragma()crashes on any PRAGMA returning a non-TEXT value (e.g.busy_timeout,page_count) because it hardcodesrow.get::<_, String>(0). Discovered because I originally tried to verify the fix viapragma('busy_timeout'); had to usequeryGet('PRAGMA busy_timeout', [])instead, which handles all SQLite value types.snapshot.ts,sequence.ts,branch-compare.ts's twoloadSymbolsFromDb/loadCallersFromDbconnections,find-cycles.ts,export-graph.ts,info.ts,native-repository.ts's fallback db) open better-sqlite3 directly vianew Database(dbPath, { readonly: true }), bypassingbusy_timeoutentirely (not even the hardcoded default).Test plan
tests/unit/native-db-busy-timeout.test.ts— exercises the real compiled native addon:openReadWrite/openReadonlyapply a passedbusyTimeoutMs, and default toDEFAULTS.db.busyTimeoutMs(5000) when omitted. Verified viaqueryGet('PRAGMA busy_timeout', [])(notpragma(), see NativeDatabase.pragma() crashes on any PRAGMA that returns a non-TEXT value #2019).tests/unit/native-db-busy-timeout-threading.test.ts— mocksinfrastructure/native.js(same pattern astests/unit/openRepo-busy.test.ts) and assertsopenRepo(),openReadonlyWithNative(), andreopenNativeDb()each pass the project-configuredbusyTimeoutMsthrough to the native factory call.npm test— full suite green (243 test files, 3899 passed, 0 failed, 30 skipped/2 todo pre-existing).npm run lint— clean (only a pre-existing unrelated warning in a fixture file I didn't touch).npx tsc --noEmitandnpm run build— clean.cargo check --releaseandcargo clippy --releaseon the crate — no new warnings on the changed lines;cargo fmt --checkshows no diff in the functions I touched.busyTimeoutMsvia the rebuilt native addon directly and confirmed the pragma value round-trips correctly for bothopenReadWriteandopenReadonly, with and without the parameter.codegraph diff-impact --staged -Tconfirms the blast radius matches exactly the 6 call sites plus their transitive callers — no unexpected symbols touched.Note: the platform-specific prebuilt binaries (
@optave/codegraph-{platform}-{arch}) still need to be rebuilt and republished for this fix to reach installs that use the npm-published native addon rather than a locally compiled one — that rides the normal release cadence per #1882's own scope note, not part of this PR.Fixes #1882