fix: thread configured busyTimeoutMs through remaining read-only query call sites#1883
Conversation
…y call sites Extends the config-driven db.busyTimeoutMs wiring (started in #1748/#1749 for openDb()/openReadonlyOrFail() and the call sites that already held a loaded config) to the ~30 ad-hoc read-only query call sites across features/*, domain/analysis/*, and domain/search/* that previously called openReadonlyOrFail() directly and silently fell back to DEFAULTS.db.busyTimeoutMs. Adds resolveBusyTimeoutMs() to src/db/connection.ts (exported via db/index.ts), sharing rootDir derivation with resolveDbSettings() via a new deriveRootDirFromDbPath() helper. For call sites that never loaded config in their path, this new call is threaded in directly. Fixing withReadonlyDb() in domain/analysis/query-helpers.ts centrally also covers its callers (exports.ts, dependencies.ts, context.ts) for free. For call sites that already loaded config but did so AFTER opening the DB (diffImpactData, checkData, complexityData, manifestoData, moduleBoundariesData), reordered config-load-before-db-open, mirroring the precedent already established by resolveDbSettings()/openReadonlyWithNative() (see the phase-15 gauntlet handle-leak fix). Verified this reorder is safe: loadConfig() only throws ConfigError for one narrow case (a non-string llm.apiKeyCommand), unrelated to db.busyTimeoutMs and independent of whether the DB exists — no existing test pins the previous error ordering for any of these call sites. Made every new config.db.busyTimeoutMs access optional-chained (config.db?.busyTimeoutMs) after this surfaced a real crash in tests/integration/complexity.test.ts, which mocks loadConfig() to return a partial object without a db key. Verified end-to-end against a real built graph with a custom .codegraphrc.json (db.busyTimeoutMs override) via the CLI (owners, check, structure --modules, map, audit, complexity, dataflow, search, co-change, ast, flow, cfg, roles, where, children), confirming the configured value reaches the PRAGMA busy_timeout call and the default still applies with no config override. Added tests/unit/busy-timeout-query-sites.test.ts covering resolveBusyTimeoutMs directly plus a representative sample of call sites from each category (ownersData, cfgData, manifestoData, hybridSearchData, withReadonlyDb), spying on Database.prototype.pragma to assert the configured value is actually applied. Filed two follow-ups discovered while completing this wiring pass, both intentionally out of scope here: - #1881: several functions (manifestoData, hybridSearchData, moduleBoundariesData, diffImpactData, complexityData, auditData) resolve their config via process.cwd() rather than the resolved --db path, unlike resolveDbSettings()/checkData() — a pre-existing inconsistency, not introduced by this change, that affects cross-repo --db usage for several config fields (not just busyTimeoutMs). - #1882: the Rust native DB layer (NativeDatabase::open_readonly/ open_read_write in crates/codegraph-core/src/db/connection.rs) still hardcodes PRAGMA busy_timeout = 5000. Investigated and confirmed this does NOT require giving Rust config-loading capability (the JS side already resolves the value) — it requires threading an already-resolved number across the napi FFI boundary plus a native rebuild across platforms, which is a heavier-weight change than this wiring pass. Refs #1763 (TS read-only call-site wiring is complete; the Rust portion of that issue's scope is deferred to #1882, so this does not close it). docs check acknowledged: internal config wiring only, no new config keys or CLI flags — db.busyTimeoutMs was already documented by the original phase-7 commit that introduced it. Impact: 36 functions changed, 131 affected
Greptile SummaryThis PR threads the user-configured
Confidence Score: 5/5Safe to merge — the change is a mechanical, well-tested threading of a config value through ~32 call sites with no logic alterations beyond call ordering. All production openReadonlyOrFail call sites now receive a config-derived timeout. The new helpers are thin wrappers around the existing deriveRootDirFromDbPath/loadConfig stack already proven in resolveDbSettings. The withReadonlyDb signature extension is backward-compatible, and the 7 new regression tests verify the pragma reaches SQLite for each fix category. The full suite (3528 tests) is green. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix: resolve config once in withReadonly..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn: (db: BetterSqlite3Database) => T, | ||
| ): T { | ||
| const db = openReadonlyOrFail(customDbPath); | ||
| const db = openReadonlyOrFail(customDbPath, resolveBusyTimeoutMs(customDbPath)); |
There was a problem hiding this comment.
loadConfig for callers that also use resolveAnalysisOpts
withReadonlyDb now calls resolveBusyTimeoutMs(customDbPath) → loadConfig(deriveRootDirFromDbPath(customDbPath)). Callers such as exports.ts, dependencies.ts, and context.ts also call resolveAnalysisOpts inside their callback, which invokes loadConfig() (cwd-rooted) when opts.config is absent. This results in two loadConfig calls per query with different root directories — one DB-path-derived (correct for the timeout) and one cwd-derived (pre-existing behaviour for other settings).
The timeout will be set correctly, but if loadConfig is not cached and involves disk I/O, the extra call adds overhead on every withReadonlyDb-backed query. A simple mitigation would be to expose an overload that accepts an already-resolved busyTimeoutMs when the caller already holds a config, or to cache loadConfig results by rootDir.
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!
There was a problem hiding this comment.
Fixed — added an exported resolveDbConfig() in connection.ts (shared by resolveBusyTimeoutMs), and withReadonlyDb now resolves it once and passes it to the callback. exports.ts/context.ts pass that resolved config into resolveAnalysisOpts as opts.config ?? config, eliminating the second loadConfig() call. Filed #1941 for the same pattern in withRepo/fn-impact.ts — out of scope here since openRepo/resolveDbSettings are shared by every native-engine call site, not just read-only queries.
Codegraph Impact Analysis40 functions changed → 132 callers affected across 84 files
|
1 participant
Summary
loadConfig()only throws for one narrow case — non-stringllm.apiKeyCommand— and no existing test pins error ordering for any of these sites; mirrors the precedent already established byresolveDbSettings()).resolveBusyTimeoutMs(customDbPath?)helper call, includingquery-helpers.ts's sharedwithReadonlyDb()which transitively fixesexports.ts/dependencies.ts/context.tsfor free.PRAGMA busy_timeout = 5000, no config awareness) is genuinely out of scope per the issue's own framing — investigated briefly, doesn't need config-loading capability (JS already resolves the value) but does need a new FFI parameter across 6 call sites + a napi rebuild. Filed as Thread config.db.busyTimeoutMs into the Rust native DB layer (NativeDatabase::open_readonly/open_read_write) #1882 rather than attempted here. UsingRefs #1763rather thanClosessince this issue named both the TS wiring and the Rust layer as its scope — only the former is done.Refs #1763
Test plan
npm test— full suite green (3528 passed, 0 failed) — caught and fixed a real bug during the gate: 26 tests incomplexity.test.tsmockloadConfig()to return{}, and non-optional-chainedconfig.db.busyTimeoutMscrashed on the missing key; fixed withconfig.db?.busyTimeoutMsmatching the existing defensive pattern elsewherenpm run lint/npx tsc --noEmit/npm run build— cleantests/unit/busy-timeout-query-sites.test.ts(7 tests) spies onDatabase.prototype.pragmato assert the configured value is actually applied.codegraphrc.json(db.busyTimeoutMs: 424242), ran 16 different commands, confirmed every one used the configured value; confirmed default (5000) still applies with no overrideFiled #1881 for an out-of-scope finding: several functions resolve config via
process.cwd()rather than the resolved--dbpath (pre-existing, not introduced here — preserved each function's existing resolution mechanism). Filed #1882 for the Rust layer.Stacked on #1880 (base branch
fix/issue-1762-titan-grind-dead-symbol-shape) — only this diff is this issue's change.