From ed30a1f1ecb573213cbc19f02a53b97a3ac8fed8 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Wed, 8 Jul 2026 18:41:02 -0600 Subject: [PATCH] fix(perf): resolve config once in withRepo to avoid double loadConfig withRepo() opened its Repository via openRepo(), which independently resolves config for engine/busy-timeout selection, while its two callers that also need analysis-tuning config -- fnImpactData() via resolveAnalysisOpts(), briefData() via a direct loadConfig() call -- triggered a second, independent loadConfig() for the same DB path. Beyond the redundant work, the second call resolved its rootDir from process.cwd() instead of the resolved --db path, so a --db pointing at a different repo than cwd would silently read the wrong project's .codegraphrc.json for analysis.fnImpactDepth/analysis.briefCallerDepth -- the same correctness gap already fixed for withReadonlyDb()'s callers. Widen withRepo() to resolve config once and thread it through to both openRepo() (via a new, additive opts.config -- every other openRepo() caller is unaffected) and the callback, mirroring withReadonlyDb's (db, config) => T signature. fnImpactData and briefData now reuse opts.config ?? dbConfig instead of loading config again. dependencies.ts and implementations.ts, the other two withRepo callers, don't read config, so their existing (repo) => T callbacks are unaffected. --- src/db/connection.ts | 14 +- src/domain/analysis/brief.ts | 5 +- src/domain/analysis/fn-impact.ts | 4 +- src/domain/analysis/query-helpers.ts | 13 +- tests/unit/withrepo-config-resolution.test.ts | 167 ++++++++++++++++++ 5 files changed, 192 insertions(+), 11 deletions(-) create mode 100644 tests/unit/withrepo-config-resolution.test.ts diff --git a/src/db/connection.ts b/src/db/connection.ts index 475a6cde1..e8e74b601 100644 --- a/src/db/connection.ts +++ b/src/db/connection.ts @@ -441,6 +441,9 @@ export function resolveConfigForDbPath(customDbPath?: string): CodegraphConfig { * Derives rootDir from the resolved DB path so loadConfig reads the right project config. * Shared by openRepo() and openReadonlyWithNative() so the two call sites can't drift. * + * Accepts an optional pre-resolved `config` for callers that already loaded it for the + * same `customDbPath` (e.g. withRepo()), avoiding a second findDbPath()/loadConfig() call. + * * MUST be called before opening any DB handle: loadConfig can throw (e.g. ConfigError * via resolveSecrets on a malformed llm.apiKeyCommand config), and an already-open * handle at that point would never be closed. @@ -448,8 +451,8 @@ export function resolveConfigForDbPath(customDbPath?: string): CodegraphConfig { function resolveDbSettings( customDbPath: string | undefined, engineOpt: 'native' | 'wasm' | 'auto' | undefined, + config: CodegraphConfig = resolveConfigForDbPath(customDbPath), ): ResolvedDbSettings { - const config = resolveConfigForDbPath(customDbPath); // config.build.engine is already populated from CODEGRAPH_ENGINE env by applyEnvOverrides, // so this covers both the env-var path and the .codegraphrc.json config-file path. return { @@ -573,10 +576,15 @@ function openRepoSqliteFallback( * When the native engine is available, opens a NativeDatabase (rusqlite) and * wraps it in NativeRepository. Otherwise falls back to better-sqlite3 via * SqliteRepository. + * + * `opts.config`, when supplied, is used in place of a fresh + * resolveConfigForDbPath() call — for callers (e.g. withRepo()) that already + * resolved config for the same `customDbPath` and need to avoid a second + * loadConfig() for it. */ export function openRepo( customDbPath?: string, - opts: { repo?: Repository; engine?: 'native' | 'wasm' | 'auto' } = {}, + opts: { repo?: Repository; engine?: 'native' | 'wasm' | 'auto'; config?: CodegraphConfig } = {}, ): { repo: Repository; close(): void } { if (opts.repo != null) { return wrapInjectedRepo(opts.repo); @@ -584,7 +592,7 @@ export function openRepo( // Respect explicit engine selection: opts.engine > config.build.engine > auto. // This ensures --engine wasm and benchmark workers bypass the native path. - const { engine, busyTimeoutMs } = resolveDbSettings(customDbPath, opts.engine); + const { engine, busyTimeoutMs } = resolveDbSettings(customDbPath, opts.engine, opts.config); const native = tryOpenRepoNative(customDbPath, engine, busyTimeoutMs); if (native) return native; diff --git a/src/domain/analysis/brief.ts b/src/domain/analysis/brief.ts index a4363ad20..dc9059031 100644 --- a/src/domain/analysis/brief.ts +++ b/src/domain/analysis/brief.ts @@ -1,5 +1,4 @@ import type { Repository } from '../../db/index.js'; -import { loadConfig } from '../../infrastructure/config.js'; import { isTestFile } from '../../infrastructure/test-filter.js'; import type { ImportEdgeRow, NodeRow, RelatedNodeRow } from '../../types.js'; import { withRepo } from './query-helpers.js'; @@ -108,9 +107,9 @@ export function briefData( customDbPath: string, opts: { noTests?: boolean; config?: any } = {}, ) { - return withRepo(customDbPath, (repo) => { + return withRepo(customDbPath, (repo, dbConfig) => { const noTests = opts.noTests || false; - const config = opts.config || loadConfig(); + const config = opts.config || dbConfig; const callerDepth = config.analysis?.briefCallerDepth ?? 5; const importerDepth = config.analysis?.briefImporterDepth ?? 5; const highRiskCallers = config.analysis?.briefHighRiskCallers ?? 10; diff --git a/src/domain/analysis/fn-impact.ts b/src/domain/analysis/fn-impact.ts index d7cfc2901..b6c34424f 100644 --- a/src/domain/analysis/fn-impact.ts +++ b/src/domain/analysis/fn-impact.ts @@ -276,8 +276,8 @@ export function fnImpactData( config?: any; } = {}, ) { - return withRepo(customDbPath, (repo) => { - const { noTests, config } = resolveAnalysisOpts(opts); + return withRepo(customDbPath, (repo, dbConfig) => { + const { noTests, config } = resolveAnalysisOpts({ ...opts, config: opts.config ?? dbConfig }); const maxDepth = opts.depth || config.analysis?.fnImpactDepth || 5; const hc = new Map(); diff --git a/src/domain/analysis/query-helpers.ts b/src/domain/analysis/query-helpers.ts index 20c0cd3b0..b9d948356 100644 --- a/src/domain/analysis/query-helpers.ts +++ b/src/domain/analysis/query-helpers.ts @@ -3,6 +3,7 @@ import { openRepo, type Repository, resolveBusyTimeoutMs, + resolveConfigForDbPath, } from '../../db/index.js'; import { loadConfig } from '../../infrastructure/config.js'; import type { BetterSqlite3Database, CodegraphConfig } from '../../types.js'; @@ -28,14 +29,20 @@ export function withReadonlyDb( * Open a Repository (native-first, falling back to better-sqlite3), run `fn`, * and close on completion. Mirrors `withReadonlyDb` but routes queries through * the native Rust engine when available. + * + * Resolves the config once and passes it to both `openRepo` (for engine/ + * busy-timeout selection) and `fn`, so callers that also need + * `resolveAnalysisOpts` can reuse it as `opts.config` instead of triggering a + * second `loadConfig()` for the same rootDir (issue #1941). */ export function withRepo( customDbPath: string | undefined, - fn: (repo: InstanceType) => T, + fn: (repo: InstanceType, config: CodegraphConfig) => T, ): T { - const { repo, close } = openRepo(customDbPath); + const config = resolveConfigForDbPath(customDbPath); + const { repo, close } = openRepo(customDbPath, { config }); try { - return fn(repo); + return fn(repo, config); } finally { close(); } diff --git a/tests/unit/withrepo-config-resolution.test.ts b/tests/unit/withrepo-config-resolution.test.ts new file mode 100644 index 000000000..a01f2793f --- /dev/null +++ b/tests/unit/withrepo-config-resolution.test.ts @@ -0,0 +1,167 @@ +/** + * Regression tests for issue #1941: `withRepo()` and its `resolveAnalysisOpts`- + * using callers (`fnImpactData`, `briefData`) each triggered a *second*, + * independent `loadConfig()` call for the same `--db` path — one implicitly + * via `openRepo()` -> `resolveDbSettings()` (to pick the engine/busy-timeout), + * and a second one via `resolveAnalysisOpts()` / a direct `loadConfig()` call + * inside the `withRepo()` callback (to read analysis-tuning fields like + * `analysis.fnImpactDepth`/`analysis.briefCallerDepth`). + * + * Beyond the redundant work, the second call resolved its rootDir from + * `process.cwd()` instead of the resolved `--db` path, so a `--db` pointing + * at a different repo than cwd would read the WRONG project's + * `.codegraphrc.json` for those fields (mirrors the `withReadonlyDb` fix + * already applied for `exports.ts`/`context.ts`). + * + * The fix: `withRepo()` resolves config once and threads it through to both + * `openRepo()` (via the new `opts.config`) and the callback, so callers can + * pass `opts.config ?? dbConfig` into `resolveAnalysisOpts` instead of + * triggering a second `loadConfig()`. + */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +const loadConfigSpy = vi.hoisted(() => vi.fn()); + +// Delegate to the real loadConfig by default; this only counts invocations. +vi.mock('../../src/infrastructure/config.js', async (importOriginal) => { + const mod = await importOriginal(); + loadConfigSpy.mockImplementation(mod.loadConfig); + return { ...mod, loadConfig: loadConfigSpy }; +}); + +import { initSchema } from '../../src/db/index.js'; +import { briefData } from '../../src/domain/analysis/brief.js'; +import { fnImpactData } from '../../src/domain/analysis/fn-impact.js'; +import { withRepo } from '../../src/domain/analysis/query-helpers.js'; + +const CUSTOM_FN_IMPACT_DEPTH = 2; +const CUSTOM_BRIEF_CALLER_DEPTH = 2; + +function insertNode(db: Database.Database, name: string, kind: string, file: string, line: number) { + return db + .prepare('INSERT INTO nodes (name, kind, file, line) VALUES (?, ?, ?, ?)') + .run(name, kind, file, line).lastInsertRowid; +} + +function insertEdge( + db: Database.Database, + sourceId: number | bigint, + targetId: number | bigint, + kind: string, +) { + db.prepare( + 'INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, ?, 1.0, 0)', + ).run(sourceId, targetId, kind); +} + +let tmpDir: string; +let cwdDir: string; +let dbPath: string; + +beforeAll(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-withrepo-config-')); + fs.mkdirSync(path.join(tmpDir, '.git')); + fs.mkdirSync(path.join(tmpDir, '.codegraph')); + dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + + // Project config lives next to the DB, not at cwd — proves rootDir is + // derived from `--db`, matching resolveConfigForDbPath()'s contract. + fs.writeFileSync( + path.join(tmpDir, '.codegraphrc.json'), + JSON.stringify({ + analysis: { + fnImpactDepth: CUSTOM_FN_IMPACT_DEPTH, + briefCallerDepth: CUSTOM_BRIEF_CALLER_DEPTH, + }, + }), + ); + + const db = new Database(dbPath); + db.pragma('journal_mode = WAL'); + initSchema(db); + + const fBase = insertNode(db, 'lib/base.js', 'file', 'lib/base.js', 0); + const fCaller = insertNode(db, 'lib/caller.js', 'file', 'lib/caller.js', 0); + insertEdge(db, fCaller, fBase, 'imports'); + + const target = insertNode(db, 'target', 'function', 'lib/base.js', 5); + const caller = insertNode(db, 'caller', 'function', 'lib/caller.js', 5); + insertEdge(db, caller, target, 'calls'); + + db.close(); + + // cwd is a *different*, config-less directory — any loadConfig() call that + // resolves from process.cwd() (instead of the --db path) would silently + // fall back to DEFAULTS instead of picking up the custom depth above. + cwdDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-withrepo-cwd-')); +}); + +afterAll(() => { + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); + if (cwdDir) fs.rmSync(cwdDir, { recursive: true, force: true }); +}); + +beforeEach(() => { + loadConfigSpy.mockClear(); +}); + +describe('withRepo config resolution', () => { + it('resolves config exactly once and passes it to the callback', () => { + const config = withRepo(dbPath, (_repo, dbConfig) => dbConfig); + expect(loadConfigSpy).toHaveBeenCalledTimes(1); + expect(config.analysis?.fnImpactDepth).toBe(CUSTOM_FN_IMPACT_DEPTH); + }); +}); + +describe('fnImpactData (issue #1941)', () => { + it('calls loadConfig exactly once per invocation', () => { + fnImpactData('target', dbPath); + expect(loadConfigSpy).toHaveBeenCalledTimes(1); + }); + + it('reads analysis.fnImpactDepth from the --db path rootDir, not process.cwd()', () => { + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(cwdDir); + try { + // depth defaults to config.analysis.fnImpactDepth when opts.depth is unset; + // the BFS at depth 1 already reaches `caller`, so this only distinguishes + // "config resolved" from "config silently defaulted" via the call count above, + // and directly via the resolved config object here. + const config = withRepo(dbPath, (_repo, dbConfig) => dbConfig); + expect(config.analysis?.fnImpactDepth).toBe(CUSTOM_FN_IMPACT_DEPTH); + + const data = fnImpactData('target', dbPath) as { results: Array<{ direct: number }> }; + expect(data.results).toHaveLength(1); + expect(data.results[0]?.direct).toBe(1); + } finally { + cwdSpy.mockRestore(); + } + }); + + it('opts.config still overrides the resolved db config when the caller supplies one', () => { + fnImpactData('target', dbPath, { config: { analysis: { fnImpactDepth: 99 } } as never }); + // The explicit opts.config short-circuits resolveAnalysisOpts's fallback, + // but withRepo's own loadConfig() (for openRepo's engine selection) still runs once. + expect(loadConfigSpy).toHaveBeenCalledTimes(1); + }); +}); + +describe('briefData (issue #1941)', () => { + it('calls loadConfig exactly once per invocation', () => { + briefData('base.js', dbPath); + expect(loadConfigSpy).toHaveBeenCalledTimes(1); + }); + + it('reads analysis.briefCallerDepth from the --db path rootDir, not process.cwd()', () => { + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(cwdDir); + try { + const config = withRepo(dbPath, (_repo, dbConfig) => dbConfig); + expect(config.analysis?.briefCallerDepth).toBe(CUSTOM_BRIEF_CALLER_DEPTH); + } finally { + cwdSpy.mockRestore(); + } + }); +});