From 927a17264dfc6fe57dd827227ab5a054608b2b97 Mon Sep 17 00:00:00 2001 From: azizur100389 Date: Fri, 8 May 2026 10:36:20 +0100 Subject: [PATCH 1/4] perf(mcp): parallelize staleness checks in list_repos (#1416) * perf(mcp): parallelize staleness checks in list_repos (#1363) Replace sequential synchronous git spawns with parallel async execFile calls so 200-repo registries resolve in under a second instead of ~50 s. * fix(test): address @claude review findings for parallel staleness PR - Add missing checkStalenessAsync mock to calltool-dispatch.test.ts (BLOCKER: caused 5 CI failures on every list_repos test path) - Add async invalid-commit-hash test for symmetry with sync suite - Document why promisified execFile omits stdio option --- gitnexus/src/core/git-staleness.ts | 38 ++++++++- gitnexus/src/mcp/local/local-backend.ts | 13 +++- gitnexus/test/unit/calltool-dispatch.test.ts | 1 + gitnexus/test/unit/staleness.test.ts | 81 +++++++++++++++++++- 4 files changed, 128 insertions(+), 5 deletions(-) diff --git a/gitnexus/src/core/git-staleness.ts b/gitnexus/src/core/git-staleness.ts index 96f70ddd6..c90cef85e 100644 --- a/gitnexus/src/core/git-staleness.ts +++ b/gitnexus/src/core/git-staleness.ts @@ -3,11 +3,14 @@ * Lives in core/ so application code does not depend on the MCP package layer. */ -import { execFileSync } from 'node:child_process'; +import { execFile, execFileSync } from 'node:child_process'; +import { promisify } from 'node:util'; import path from 'path'; import { readRegistry, type RegistryEntry, type CwdMatch } from '../storage/repo-manager.js'; import { findGitRootByDotGit, getCurrentCommit, getRemoteUrl } from '../storage/git.js'; +const execFileAsync = promisify(execFile); + export interface StalenessInfo { isStale: boolean; commitsBehind: number; @@ -41,6 +44,39 @@ export function checkStaleness(repoPath: string, lastCommit: string): StalenessI } } +/** + * Async variant of {@link checkStaleness} — spawns git as a child process + * instead of blocking the event loop. Used by `listRepos()` to check many + * repos in parallel (issue #1363: 200 repos × sync spawn ≈ 50 s). + */ +export async function checkStalenessAsync( + repoPath: string, + lastCommit: string, +): Promise { + try { + // Note: promisified execFile captures stdout/stderr by default (no stdio option needed, + // unlike the sync variant which requires explicit stdio: ['pipe','pipe','pipe']). + const { stdout } = await execFileAsync('git', ['rev-list', '--count', `${lastCommit}..HEAD`], { + cwd: repoPath, + encoding: 'utf-8', + }); + + const commitsBehind = parseInt(stdout.trim(), 10) || 0; + + if (commitsBehind > 0) { + return { + isStale: true, + commitsBehind, + hint: `⚠️ Index is ${commitsBehind} commit${commitsBehind > 1 ? 's' : ''} behind HEAD. Run analyze tool to update.`, + }; + } + + return { isStale: false, commitsBehind: 0 }; + } catch { + return { isStale: false, commitsBehind: 0 }; + } +} + /** * Compare a sibling-clone HEAD against an indexed `lastCommit`. Returns * `undefined` when the indexed commit is not reachable from the sibling diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index b53034378..34049ab31 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -40,7 +40,7 @@ import { isVectorExtensionSupportedByPlatform, } from '../../core/platform/capabilities.js'; import { PhaseTimer } from '../../core/search/phase-timer.js'; -import { checkStaleness, checkCwdMatch } from '../../core/git-staleness.js'; +import { checkStalenessAsync, checkCwdMatch } from '../../core/git-staleness.js'; import { logger } from '../../core/logger.js'; // AI context generation is CLI-only (gitnexus analyze) // import { generateAIContextFiles } from '../../cli/ai-context.js'; @@ -554,8 +554,15 @@ export class LocalBackend { byRemote.set(h.remoteUrl, list); } - return handles.map((h) => { - const stale = checkStaleness(h.repoPath, h.lastCommit); + // Check staleness for all repos in parallel instead of sequentially. + // Each check spawns an async `git rev-list` — with 200 repos the sync + // variant took ~50 s; parallel async brings it under a second (#1363). + const stalenessResults = await Promise.all( + handles.map((h) => checkStalenessAsync(h.repoPath, h.lastCommit)), + ); + + return handles.map((h, i) => { + const stale = stalenessResults[i]; const selfNorm = norm(h.repoPath); const siblings = h.remoteUrl ? (byRemote.get(h.remoteUrl) ?? []).filter((e) => norm(e.repoPath) !== selfNorm) diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index f8d94d890..6b13cacfa 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -48,6 +48,7 @@ vi.mock('../../src/storage/repo-manager.js', () => ({ // tests don't shell out to git. vi.mock('../../src/core/git-staleness.js', () => ({ checkStaleness: vi.fn().mockReturnValue({ isStale: false, commitsBehind: 0 }), + checkStalenessAsync: vi.fn().mockResolvedValue({ isStale: false, commitsBehind: 0 }), checkCwdMatch: vi.fn().mockResolvedValue({ match: 'none' }), })); diff --git a/gitnexus/test/unit/staleness.test.ts b/gitnexus/test/unit/staleness.test.ts index b8ea398ff..1645d9987 100644 --- a/gitnexus/test/unit/staleness.test.ts +++ b/gitnexus/test/unit/staleness.test.ts @@ -8,7 +8,7 @@ */ import { describe, it, expect } from 'vitest'; import { execFileSync } from 'child_process'; -import { checkStaleness } from '../../src/core/git-staleness.js'; +import { checkStaleness, checkStalenessAsync } from '../../src/core/git-staleness.js'; // We test checkStaleness with a real git repo (the project itself) // since mocking execFileSync across ESM modules is complex. @@ -65,3 +65,82 @@ describe('checkStaleness', () => { expect(result.commitsBehind).toBe(0); }); }); + +describe('checkStalenessAsync', () => { + it('returns not stale when HEAD matches lastCommit', async () => { + let headCommit: string; + try { + headCommit = execFileSync('git', ['rev-parse', 'HEAD'], { + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }).trim(); + } catch { + return; + } + + const result = await checkStalenessAsync(process.cwd(), headCommit); + expect(result.isStale).toBe(false); + expect(result.commitsBehind).toBe(0); + expect(result.hint).toBeUndefined(); + }); + + it('returns stale when lastCommit is behind HEAD', async () => { + let previousCommit: string; + try { + previousCommit = execFileSync('git', ['rev-parse', 'HEAD~1'], { + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }).trim(); + } catch { + return; + } + + if (!previousCommit) return; + + const result = await checkStalenessAsync(process.cwd(), previousCommit); + expect(result.isStale).toBe(true); + expect(result.commitsBehind).toBeGreaterThan(0); + expect(result.hint).toContain('behind HEAD'); + }); + + it('fails open when git command fails (e.g., invalid path)', async () => { + const result = await checkStalenessAsync('/nonexistent/path', 'abc123'); + expect(result.isStale).toBe(false); + expect(result.commitsBehind).toBe(0); + }); + + it('fails open with invalid commit hash', async () => { + const result = await checkStalenessAsync(process.cwd(), 'not-a-real-commit-hash'); + expect(result.isStale).toBe(false); + expect(result.commitsBehind).toBe(0); + }); + + it('parallel calls complete faster than sequential', async () => { + let headCommit: string; + try { + headCommit = execFileSync('git', ['rev-parse', 'HEAD'], { + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }).trim(); + } catch { + return; + } + + const cwd = process.cwd(); + const N = 10; + + // Parallel + const t0 = performance.now(); + await Promise.all(Array.from({ length: N }, () => checkStalenessAsync(cwd, headCommit))); + const parallelMs = performance.now() - t0; + + // Sequential sync + const t1 = performance.now(); + for (let i = 0; i < N; i++) checkStaleness(cwd, headCommit); + const sequentialMs = performance.now() - t1; + + // Parallel should be meaningfully faster than sequential. + // Use a generous ratio to avoid flakiness on slow CI machines. + expect(parallelMs).toBeLessThan(sequentialMs * 1.5); + }); +}); From 8ca9cb1a4d9d80706a4e53266e64ca031326813e Mon Sep 17 00:00:00 2001 From: evolution Date: Fri, 8 May 2026 18:28:19 +0800 Subject: [PATCH 2/4] fix(lbug): recover from WAL corruption by quarantining .wal file (#1402) (#1417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lbug): recover from WAL corruption by quarantining .wal file (#1402) LadybugDB crashes when the WAL file is corrupted — the open fails with an unrecoverable native error. This makes the pool adapter detect WAL corruption errors, quarantine the offending .wal file, and retry the open. MCP tool responses (cypher, context, impact) now include a recoverySuggestion field when WAL corruption is detected. Changes: - Add isWalCorruptionError() regex-based detector in lbug-config.ts - Add throwOnWalReplayFailure and enableChecksums to createLbugDatabase() - Extract openReadOnlyDatabase() with stdout silencing + db.init() - Add tryQuarantineAndReopen() for .wal quarantine + retry in doInitLbug - Wrap cypher/context/impact with WAL recoverySuggestion in MCP responses - Share WAL_RECOVERY_SUGGESTION constant across all MCP error paths - Fix restoreStdout() placement (before db.init() → finally block) - Add unit tests for detection, pool recovery, and MCP feedback * fix(test): remove superfluous argument from LocalBackend constructor (#1402) LocalBackend has no constructor — the { registryPath } argument was ignored. * fix(lbug): address WAL recovery review feedback --------- Co-authored-by: Gergő Magyar --- gitnexus/src/core/lbug/lbug-config.ts | 26 ++- gitnexus/src/core/lbug/pool-adapter.ts | 65 ++++++- gitnexus/src/mcp/local/local-backend.ts | 35 +++- gitnexus/test/unit/lbug-config-wal.test.ts | 59 +++++++ gitnexus/test/unit/mcp-wal-feedback.test.ts | 160 +++++++++++++++++ gitnexus/test/unit/pool-wal-recovery.test.ts | 177 +++++++++++++++++++ 6 files changed, 511 insertions(+), 11 deletions(-) create mode 100644 gitnexus/test/unit/lbug-config-wal.test.ts create mode 100644 gitnexus/test/unit/mcp-wal-feedback.test.ts create mode 100644 gitnexus/test/unit/pool-wal-recovery.test.ts diff --git a/gitnexus/src/core/lbug/lbug-config.ts b/gitnexus/src/core/lbug/lbug-config.ts index 5534f7d8b..a3e90051f 100644 --- a/gitnexus/src/core/lbug/lbug-config.ts +++ b/gitnexus/src/core/lbug/lbug-config.ts @@ -42,10 +42,23 @@ export const LBUG_MAX_DB_SIZE: number = (() => { return 16 * 1024 * 1024 * 1024; })(); +/** Matches WAL corruption errors from the LadybugDB engine. */ +const WAL_CORRUPTION_RE = /corrupt(ed)?\s+wal|invalid\s+wal\s+record|wal.*corrupt|checksum.*wal/i; + +export const WAL_RECOVERY_SUGGESTION = + 'WAL corruption detected. Run `gitnexus analyze` to rebuild the index.'; + +export function isWalCorruptionError(err: unknown): boolean { + if (!err) return false; + const msg = err instanceof Error ? err.message : String(err); + return WAL_CORRUPTION_RE.test(msg); +} + type LbugModule = typeof lbug; export interface LbugDatabaseOptions { readOnly?: boolean; + throwOnWalReplayFailure?: boolean; } export interface LbugConnectionHandle { @@ -58,13 +71,18 @@ export function createLbugDatabase( databasePath: string, options: LbugDatabaseOptions = {}, ): lbug.Database { - return new lbugModule.Database( + // .d.ts declares fewer args than the native constructor accepts. + return new (lbugModule.Database as any)( databasePath, - 0, - false, + 0, // bufferManagerSize + false, // enableCompression (pinned for v0.16.0) options.readOnly ?? false, LBUG_MAX_DB_SIZE, - ); + true, // autoCheckpoint + -1, // checkpointThreshold + options.throwOnWalReplayFailure ?? true, + true, // enableChecksums + ) as lbug.Database; } export async function openLbugConnection( diff --git a/gitnexus/src/core/lbug/pool-adapter.ts b/gitnexus/src/core/lbug/pool-adapter.ts index ca1c45611..ed999907e 100644 --- a/gitnexus/src/core/lbug/pool-adapter.ts +++ b/gitnexus/src/core/lbug/pool-adapter.ts @@ -18,7 +18,7 @@ import fs from 'fs/promises'; import lbug from '@ladybugdb/core'; import { loadFTSExtension } from './lbug-adapter.js'; -import { createLbugDatabase } from './lbug-config.js'; +import { createLbugDatabase, isWalCorruptionError } from './lbug-config.js'; /** Per-repo pool: one Database, many Connections */ interface PoolEntry { @@ -97,7 +97,7 @@ let idleTimer: ReturnType | null = null; // @ladybugdb/core), corrupting stdout in the pre-sentinel window. Routing // through the leaf breaks that chain. export { realStdoutWrite, realStderrWrite, setActiveStdoutWrite } from '../../mcp/stdio-capture.js'; -import { getActiveStdoutWrite } from '../../mcp/stdio-capture.js'; +import { getActiveStdoutWrite, realStderrWrite } from '../../mcp/stdio-capture.js'; let stdoutSilenceCount = 0; /** True while pre-warming connections — prevents watchdog from prematurely restoring stdout */ @@ -263,6 +263,46 @@ const WAITER_TIMEOUT_MS = 15_000; const LOCK_RETRY_ATTEMPTS = 3; const LOCK_RETRY_DELAY_MS = 2000; +async function openReadOnlyDatabase(dbPath: string): Promise { + let db: lbug.Database | undefined; + silenceStdout(); + try { + db = createLbugDatabase(lbug, dbPath, { + readOnly: true, + throwOnWalReplayFailure: false, + }); + await db.init(); + return db; + } catch (err) { + if (db) await db.close().catch(() => {}); + throw err; + } finally { + restoreStdout(); + } +} + +/** + * Quarantine the .wal file and retry opening the database. + * Used when the initial open fails with a WAL corruption error. + */ +async function tryQuarantineAndReopen(dbPath: string, repoId: string): Promise { + const walPath = dbPath + '.wal'; + const quarantineName = `${walPath}.corrupt.${Date.now()}-${Math.random().toString(36).slice(2)}`; + try { + await fs.rename(walPath, quarantineName); + } catch { + throw new Error( + `LadybugDB WAL corruption detected for ${repoId}. ` + + `Run \`gitnexus analyze\` to rebuild the index. (quarantine failed)`, + ); + } + realStderrWrite( + `GitNexus: LadybugDB WAL quarantined for ${repoId}; graph may be stale. ` + + `Run \`gitnexus analyze\` to rebuild the index.\n`, + ); + return await openReadOnlyDatabase(dbPath); +} + /** Deduplicates concurrent initLbug calls for the same repoId */ const initPromises = new Map>(); @@ -319,16 +359,29 @@ async function doInitLbug(repoId: string, dbPath: string): Promise { // avoids lock conflicts when `gitnexus analyze` is writing. let lastError: Error | null = null; for (let attempt = 1; attempt <= LOCK_RETRY_ATTEMPTS; attempt++) { - silenceStdout(); try { - const db = createLbugDatabase(lbug, dbPath, { readOnly: true }); - restoreStdout(); + const db = await openReadOnlyDatabase(dbPath); shared = { db, refCount: 0, ftsLoaded: false }; dbCache.set(dbPath, shared); break; } catch (err: any) { - restoreStdout(); lastError = err instanceof Error ? err : new Error(String(err)); + + if (isWalCorruptionError(lastError)) { + try { + const db = await tryQuarantineAndReopen(dbPath, repoId); + shared = { db, refCount: 0, ftsLoaded: false }; + dbCache.set(dbPath, shared); + break; + } catch (retryErr) { + throw new Error( + `LadybugDB WAL corruption detected for ${repoId}. ` + + `Run \`gitnexus analyze\` to rebuild the index. ` + + `(${retryErr instanceof Error ? retryErr.message : String(retryErr)})`, + ); + } + } + const isLockError = lastError.message.includes('Could not set lock') || lastError.message.includes('lock'); if (!isLockError || attempt === LOCK_RETRY_ATTEMPTS) break; diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 34049ab31..2f16c1fb2 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -16,6 +16,7 @@ import { isLbugReady, isWriteQuery, } from '../../core/lbug/pool-adapter.js'; +import { isWalCorruptionError, WAL_RECOVERY_SUGGESTION } from '../../core/lbug/lbug-config.js'; export { isWriteQuery }; // Embedding imports are lazy (dynamic import) to avoid loading onnxruntime-node // at MCP server startup — crashes on unsupported Node ABI versions (#89) @@ -1225,7 +1226,14 @@ export class LocalBackend { const result = await executeQuery(repo.id, params.query); return result; } catch (err: any) { - return { error: err.message || 'Query failed' }; + const msg = err.message || 'Query failed'; + if (isWalCorruptionError(err)) { + return { + error: msg, + recoverySuggestion: WAL_RECOVERY_SUGGESTION, + }; + } + return { error: msg }; } } @@ -1679,6 +1687,30 @@ export class LocalBackend { kind?: string; include_content?: boolean; }, + ): Promise { + try { + return await this._contextImpl(repo, params); + } catch (err: any) { + const msg = (err instanceof Error ? err.message : String(err)) || 'Context query failed'; + if (isWalCorruptionError(err)) { + return { + error: msg, + recoverySuggestion: WAL_RECOVERY_SUGGESTION, + }; + } + throw err; + } + } + + private async _contextImpl( + repo: RepoHandle, + params: { + name?: string; + uid?: string; + file_path?: string; + kind?: string; + include_content?: boolean; + }, ): Promise { await this.ensureInitialized(repo.id); @@ -2440,6 +2472,7 @@ export class LocalBackend { impactedCount: 0, risk: 'UNKNOWN', suggestion: 'The graph query failed — try gitnexus context as a fallback', + ...(isWalCorruptionError(err) ? { recoverySuggestion: WAL_RECOVERY_SUGGESTION } : {}), }; } } diff --git a/gitnexus/test/unit/lbug-config-wal.test.ts b/gitnexus/test/unit/lbug-config-wal.test.ts new file mode 100644 index 000000000..6baea3621 --- /dev/null +++ b/gitnexus/test/unit/lbug-config-wal.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createLbugDatabase, isWalCorruptionError } from '../../src/core/lbug/lbug-config.js'; + +describe('isWalCorruptionError', () => { + it.each([ + [ + 'Corrupted wal file', + 'Runtime exception: Corrupted wal file. Read out invalid WAL record type.', + ], + ['invalid WAL record', 'Error: invalid WAL record type'], + ['WAL checksum', 'Checksum verification failed, the WAL file is corrupted.'], + ['WAL + corrupt', 'the WAL file is corrupted'], + ])('matches WAL corruption: %s', (_label, msg) => { + expect(isWalCorruptionError(msg)).toBe(true); + expect(isWalCorruptionError(new Error(msg))).toBe(true); + }); + + it.each([ + ['lock error', 'Could not set lock on file : /path/to/db'], + ['generic', 'Query failed'], + ['not found', 'LadybugDB not found at /path'], + ['checksum without WAL', 'Checksum verification failed for parquet file'], + ['permission path with WAL', "EACCES: permission denied '/path/to/wal'"], + ['schema mismatch WAL', 'schema version mismatch in WAL'], + ])('does not match non-WAL error: %s', (_label, msg) => { + expect(isWalCorruptionError(msg)).toBe(false); + }); + + it('handles non-string input', () => { + expect(isWalCorruptionError(undefined)).toBe(false); + expect(isWalCorruptionError(null)).toBe(false); + expect(isWalCorruptionError(42)).toBe(false); + expect(isWalCorruptionError(new Error('ok'))).toBe(false); + }); +}); + +describe('createLbugDatabase WAL replay option', () => { + it('passes throwOnWalReplayFailure and checksum constructor args explicitly', () => { + const Database = vi.fn(function (this: any) {}); + const lbugModule = { Database } as any; + + createLbugDatabase(lbugModule, '/tmp/lbug', { + readOnly: true, + throwOnWalReplayFailure: false, + }); + + expect(Database).toHaveBeenCalledWith( + '/tmp/lbug', + 0, + false, + true, + expect.any(Number), + true, + -1, + false, + true, + ); + }); +}); diff --git a/gitnexus/test/unit/mcp-wal-feedback.test.ts b/gitnexus/test/unit/mcp-wal-feedback.test.ts new file mode 100644 index 000000000..e387e0b97 --- /dev/null +++ b/gitnexus/test/unit/mcp-wal-feedback.test.ts @@ -0,0 +1,160 @@ +/** + * Tests for WAL corruption feedback in MCP error responses (#1402). + */ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { lbugMocks, platformMocks, repoMocks } = vi.hoisted(() => ({ + lbugMocks: { + initLbug: vi.fn().mockResolvedValue(undefined), + executeQuery: vi.fn(), + executeParameterized: vi.fn(), + closeLbug: vi.fn().mockResolvedValue(undefined), + isLbugReady: vi.fn().mockReturnValue(true), + isWriteQuery: vi.fn().mockReturnValue(false), + }, + platformMocks: { + isVectorExtensionSupportedByPlatform: vi.fn().mockReturnValue(true), + }, + repoMocks: { + listRegisteredRepos: vi.fn(), + }, +})); + +vi.mock('../../src/core/lbug/pool-adapter.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, ...lbugMocks }; +}); + +vi.mock('../../src/mcp/core/lbug-adapter.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, ...lbugMocks }; +}); + +vi.mock('../../src/storage/repo-manager.js', () => ({ + listRegisteredRepos: repoMocks.listRegisteredRepos, + cleanupOldKuzuFiles: vi.fn().mockResolvedValue({ found: false, needsReindex: false }), + findSiblingClones: vi.fn().mockResolvedValue([]), +})); + +vi.mock('../../src/core/git-staleness.js', () => ({ + checkStaleness: vi.fn().mockReturnValue({ isStale: false, commitsBehind: 0 }), + checkCwdMatch: vi.fn().mockResolvedValue({ match: 'none' }), +})); + +vi.mock('../../src/core/platform/capabilities.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + isVectorExtensionSupportedByPlatform: platformMocks.isVectorExtensionSupportedByPlatform, + }; +}); + +vi.mock('../../src/core/search/bm25-index.js', () => ({ + searchFTSFromLbug: vi.fn().mockResolvedValue([]), +})); + +vi.mock('../../src/mcp/core/embedder.js', () => ({ + embedQuery: vi.fn().mockResolvedValue([]), + getEmbeddingDims: vi.fn().mockReturnValue(384), +})); + +import { LocalBackend } from '../../src/mcp/local/local-backend.js'; + +const MOCK_REPO_ENTRY = { + name: 'test-repo', + path: '/tmp/test', + storagePath: '/tmp/test/.gitnexus', + indexedAt: '2026-05-01T00:00:00Z', + lastCommit: 'abc1234', +}; + +async function makeBackend(): Promise { + const backend = new LocalBackend(); + await backend.init(); + return backend; +} + +describe('WAL corruption feedback in MCP responses (#1402)', () => { + beforeEach(() => { + vi.clearAllMocks(); + lbugMocks.initLbug.mockResolvedValue(undefined); + lbugMocks.executeQuery.mockResolvedValue([]); + lbugMocks.executeParameterized.mockResolvedValue([]); + lbugMocks.isLbugReady.mockReturnValue(true); + lbugMocks.isWriteQuery.mockReturnValue(false); + repoMocks.listRegisteredRepos.mockResolvedValue([MOCK_REPO_ENTRY]); + }); + + it('impact returns WAL suggestion on corrupted WAL error', async () => { + const backend = await makeBackend(); + lbugMocks.executeParameterized.mockRejectedValueOnce( + new Error('Runtime exception: Corrupted wal file. Read out invalid WAL record type.'), + ); + + const result = await backend.callTool('impact', { + repo: 'test-repo', + target: 'MyClass', + direction: 'upstream', + }); + + expect(result.error).toBeDefined(); + expect(result.suggestion).toBe( + 'The graph query failed — try gitnexus context as a fallback', + ); + expect(result.recoverySuggestion).toBeDefined(); + }); + + it('cypher returns WAL recoverySuggestion on corrupted WAL error', async () => { + const backend = await makeBackend(); + lbugMocks.executeQuery.mockRejectedValueOnce(new Error('Corrupted wal file')); + + const result = await backend.callTool('cypher', { + repo: 'test-repo', + query: 'MATCH (n) RETURN n LIMIT 1', + }); + + expect(result.error).toBe('Corrupted wal file'); + expect(result.recoverySuggestion).toBeDefined(); + }); + + it('context returns WAL recoverySuggestion on corrupted WAL error', async () => { + const backend = await makeBackend(); + lbugMocks.executeParameterized.mockRejectedValueOnce(new Error('Corrupted wal file')); + + const result = await backend.callTool('context', { + repo: 'test-repo', + name: 'MyClass', + }); + + expect(result.error).toBe('Corrupted wal file'); + expect(result.recoverySuggestion).toBeDefined(); + }); + + it('non-WAL errors do not include WAL suggestion', async () => { + const backend = await makeBackend(); + lbugMocks.executeParameterized.mockRejectedValueOnce(new Error('Some other error')); + + const result = await backend.callTool('impact', { + repo: 'test-repo', + target: 'MyClass', + direction: 'upstream', + }); + + expect(result.error).toBeDefined(); + expect(result.suggestion).toBe( + 'The graph query failed — try gitnexus context as a fallback', + ); + }); + + it('context preserves non-WAL throw behavior', async () => { + const backend = await makeBackend(); + lbugMocks.executeParameterized.mockRejectedValueOnce(new Error('Some other error')); + + await expect( + backend.callTool('context', { + repo: 'test-repo', + name: 'MyClass', + }), + ).rejects.toThrow('Some other error'); + }); +}); diff --git a/gitnexus/test/unit/pool-wal-recovery.test.ts b/gitnexus/test/unit/pool-wal-recovery.test.ts new file mode 100644 index 000000000..19b24c583 --- /dev/null +++ b/gitnexus/test/unit/pool-wal-recovery.test.ts @@ -0,0 +1,177 @@ +/** + * Tests for WAL corruption recovery in the connection pool (#1402). + * + * Mocks createLbugDatabase and fs to verify quarantine + retry behavior + * without needing a real LadybugDB instance or corrupted WAL file. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { stderrWriteMock } = vi.hoisted(() => ({ + stderrWriteMock: vi.fn(), +})); + +vi.mock('fs/promises', () => ({ + default: { + stat: vi.fn().mockResolvedValue({}), + unlink: vi.fn().mockResolvedValue(undefined), + rename: vi.fn().mockResolvedValue(undefined), + }, +})); + +vi.mock('@ladybugdb/core', () => ({ + default: { + Database: vi.fn(), + Connection: vi.fn(function (this: any) { + this.close = vi.fn().mockResolvedValue(undefined); + }), + }, +})); + +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + loadFTSExtension: vi.fn().mockResolvedValue(true), +})); + +vi.mock('../../src/core/lbug/lbug-config.js', () => ({ + createLbugDatabase: vi.fn(), + LBUG_MAX_DB_SIZE: 1024, + isWalCorruptionError: vi.fn((err: unknown) => { + const msg = err instanceof Error ? err.message : String(err ?? ''); + return /corrupt(ed)?\s+wal|invalid\s+wal\s+record/i.test(msg); + }), +})); + +vi.mock('../../src/mcp/stdio-capture.js', () => ({ + realStdoutWrite: vi.fn(), + realStderrWrite: stderrWriteMock, + setActiveStdoutWrite: vi.fn(), + getActiveStdoutWrite: vi.fn(() => vi.fn()), +})); + +import fs from 'fs/promises'; +import { createLbugDatabase } from '../../src/core/lbug/lbug-config.js'; + +const { closeLbug } = await import('../../src/core/lbug/pool-adapter.js'); + +const mockInit = vi.fn().mockResolvedValue(undefined); +const mockClose = vi.fn().mockResolvedValue(undefined); + +function makeMockDb() { + return { init: mockInit, close: mockClose, _isClosed: false } as any; +} + +describe('WAL corruption recovery in doInitLbug (#1402)', () => { + beforeEach(() => { + (createLbugDatabase as any).mockReset(); + (fs.stat as any).mockReset(); + (fs.rename as any).mockReset(); + mockInit.mockReset(); + mockClose.mockReset(); + mockInit.mockResolvedValue(undefined); + mockClose.mockResolvedValue(undefined); + (fs.stat as any).mockResolvedValue({}); + (fs.rename as any).mockResolvedValue(undefined); + }); + + afterEach(async () => { + vi.useRealTimers(); + await closeLbug().catch(() => {}); + vi.clearAllMocks(); + }); + + it('retries with WAL quarantine on corrupted WAL init error', async () => { + const { initLbug } = await import('../../src/core/lbug/pool-adapter.js'); + const dbPath = '/tmp/test-wal-recovery/lbug'; + + const badDb = makeMockDb(); + const goodDb = makeMockDb(); + badDb.init = vi.fn().mockRejectedValueOnce(new Error('Corrupted wal file')); + (createLbugDatabase as any).mockReturnValueOnce(badDb).mockReturnValueOnce(goodDb); + + await initLbug('test-repo-init', dbPath); + + expect(badDb.init).toHaveBeenCalledTimes(1); + expect(createLbugDatabase).toHaveBeenCalledTimes(2); + expect(createLbugDatabase).toHaveBeenCalledWith( + expect.anything(), + dbPath, + expect.objectContaining({ + readOnly: true, + throwOnWalReplayFailure: false, + }), + ); + expect(fs.rename).toHaveBeenCalledWith( + dbPath + '.wal', + expect.stringContaining('.wal.corrupt.'), + ); + expect(stderrWriteMock).toHaveBeenCalledWith( + expect.stringContaining('WAL quarantined for test-repo-init'), + ); + }); + + it('does not quarantine on lock error (preserves existing lock retry)', async () => { + const { initLbug } = await import('../../src/core/lbug/pool-adapter.js'); + const setTimeoutSpy = vi.spyOn(global, 'setTimeout').mockImplementation((callback: any) => { + callback(); + return 0 as any; + }); + const dbPath = '/tmp/test-wal-recovery/lbug'; + + (createLbugDatabase as any).mockImplementation(() => { + throw new Error('Could not set lock on file'); + }); + + try { + await expect(initLbug('test-repo-lock', dbPath)).rejects.toThrow(); + } finally { + setTimeoutSpy.mockRestore(); + } + + expect(fs.rename).not.toHaveBeenCalled(); + }); + + it('throws with analyze suggestion after retry also fails', async () => { + const { initLbug } = await import('../../src/core/lbug/pool-adapter.js'); + const dbPath = '/tmp/test-wal-recovery/lbug'; + + (createLbugDatabase as any) + .mockImplementationOnce(() => { + throw new Error('Corrupted wal file'); + }) + .mockImplementationOnce(() => { + throw new Error('Still broken'); + }); + + await expect(initLbug('test-repo-fail', dbPath)).rejects.toThrow(/gitnexus analyze/); + expect(createLbugDatabase).toHaveBeenCalledTimes(2); + }); + + it('does not reuse poisoned state after WAL failure', async () => { + const { initLbug, isLbugReady: ready } = await import('../../src/core/lbug/pool-adapter.js'); + const dbPath = '/tmp/test-wal-recovery/lbug'; + + (createLbugDatabase as any) + .mockImplementationOnce(() => { + throw new Error('Corrupted wal file'); + }) + .mockImplementationOnce(() => { + throw new Error('Still broken'); + }); + + await expect(initLbug('test-repo-nocache', dbPath)).rejects.toThrow(); + + expect(ready('test-repo-nocache')).toBe(false); + }); + + it('handles quarantine gracefully when .wal file does not exist', async () => { + const { initLbug } = await import('../../src/core/lbug/pool-adapter.js'); + const dbPath = '/tmp/test-wal-recovery/lbug'; + + (fs.rename as any).mockRejectedValueOnce(new Error('ENOENT: no such file')); + + (createLbugDatabase as any).mockImplementationOnce(() => { + throw new Error('Corrupted wal file'); + }); + + await expect(initLbug('test-repo-enoent', dbPath)).rejects.toThrow(/gitnexus analyze/); + }); +}); From 1d46200c47f2c9b9e95588b975edde429b289d84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Fri, 8 May 2026 11:58:01 +0100 Subject: [PATCH 3/4] fix(lbug): robust Windows lock acquisition for CI integration tests (#1430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lbug): robust Windows lock acquisition for CI integration tests LadybugDB's `new Database()` raises `Could not set lock on file` from local_file_system.cpp synchronously inside the constructor — before any query is issued, so `withLbugDb`'s query-time retry never sees it. On Windows CI this surfaces as flaky integration tests due to AV-scanner holds, libuv handle-release lag, and stale `.wal` sidecars from aborted prior runs. This change closes the gap at *open time*: - `openLbugConnection` now wraps `new lbug.Database()` in a bounded busy-retry (5x100ms back-off) inside `lbug-config.ts`. Errors that exhaust the budget are tagged via `LBUG_OPEN_RETRY_EXHAUSTED` so `withLbugDb`'s outer 3x retry skips re-retrying a freshly-exhausted path (eliminates the 3x5=15-attempt / ~6s tail latency). - For recognized test fixtures only (immediate-parent dir matches a known prefix AND resolves under `os.tmpdir()`), one final stale- sidecar sweep removes `.wal`/`.lock` and retries once. Production paths never enter this branch. - `safeClose` on Windows runs a bounded `fs.open` probe to absorb native handle-release lag; logs a warning if the probe exhausts so operators can spot AV interference. - `isDbBusyError` is now defined in `lbug-config.ts` as the single source of truth, re-exported from `lbug-adapter.ts` for compatibility. - New tests cover open-time retry (happy/retry/exhaust/non-busy/tag), stale-sidecar sweep (test-fixture-only, production-rejection, preserves-original-error), `isTestFixturePath` direct unit suite (accept/reject/traversal/nested/trailing-sep), and `waitForWindowsHandleRelease` (openable/ENOENT/no-leak). - The two new test files are added to vitest's existing serialized `lbug-db` project (already `fileParallelism: false`). Closes the chronic Windows CI flake on lbug-touching integration tests while preserving the existing single-writable-Database-per-process LadybugDB contract. No public API surface changed. Co-Authored-By: Claude Opus 4.7 (1M context) * refactor(lbug): drop isDbBusyError re-export, import from lbug-config directly The re-export from lbug-adapter.ts was a transitional convenience — with the matcher now living in lbug-config.ts, having two import paths for the same symbol invites future drift. Updated the two real consumers (lbug-lock-retry.test.ts, lbug-open-retry.test.ts) to import from lbug-config directly, removed the re-export equality test (now vacuous), and refreshed the explanatory comment so it no longer references a re-export pattern that doesn't exist. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(lbug): silence benign LadybugDB v0.16.1 schema-init lock warnings on Windows doInitLbug logs "⚠️ Schema creation warning: ... Could not set lock on file" on every CREATE NODE TABLE call after the first init on a given dbPath, on Windows. The lock is internal to LadybugDB v0.16.1 and is resolved before the table is created — same tolerance pattern as the existing "already exists" filter. Genuine cross-process lock contention still surfaces on the next operation through withLbugDb's retry, so filtering at the schema-init catch only suppresses noise, not signal. Also extend the safeClose Windows handle-release probe to cover the .wal sidecar (the previous Database's WAL handle was the slowest to release, surfacing as the schema-query lock contention) and switch the probe back to 'r+' so it actually detects exclusive locks. Test loop in lbug-close-handle-release.test.ts simplified to 10 plain iterations now that the underlying noise is filtered upstream. Co-Authored-By: Claude Opus 4.7 (1M context) * chore(lbug): isDbBusyError review fixes - Drop redundant `could not set lock` term — already subsumed by `lock`. - Document the intentionally-broad matcher: graph-DB lock-shaped errors ("deadlock", "unlock failed", "lock contention", "could not open lock file") are all treated as transient. If a non-transient surfaces, tighten the matcher rather than raise the retry budget. - Add positive test cases covering those lock-shaped strings so the intent is visible and a future tightening would deliberately break these. - Fix the open-retry back-off comment: max sleep is 100+200+300+400 = 1000ms (no sleep after the final attempt), not 1.5s. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- gitnexus/src/core/lbug/lbug-adapter.ts | 56 +++- gitnexus/src/core/lbug/lbug-config.ts | 240 +++++++++++++- gitnexus/test/helpers/test-db.ts | 7 + .../lbug-close-handle-release.test.ts | 41 +++ .../test/integration/lbug-lock-retry.test.ts | 14 +- .../test/integration/lbug-open-retry.test.ts | 310 ++++++++++++++++++ gitnexus/vitest.config.ts | 4 + 7 files changed, 653 insertions(+), 19 deletions(-) create mode 100644 gitnexus/test/integration/lbug-close-handle-release.test.ts create mode 100644 gitnexus/test/integration/lbug-open-retry.test.ts diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index 2fc12cf96..fb4cf76de 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -19,7 +19,10 @@ import type { CachedEmbedding } from '../embeddings/types.js'; import { extensionManager, type ExtensionEnsureOptions } from './extension-loader.js'; import { closeLbugConnection, + isDbBusyError, + isOpenRetryExhausted, openLbugConnection, + waitForWindowsHandleRelease, type LbugConnectionHandle, } from './lbug-config.js'; import { isVectorExtensionSupportedByPlatform } from '../platform/capabilities.js'; @@ -185,21 +188,6 @@ const DB_LOCK_RETRY_ATTEMPTS = 3; /** Base back-off in ms between BUSY retries (multiplied by attempt number). */ const DB_LOCK_RETRY_DELAY_MS = 500; -/** - * Return true when the error message indicates that another process holds - * an exclusive lock on the LadybugDB file (e.g. `gitnexus analyze` or - * `gitnexus serve` running at the same time). - */ -export const isDbBusyError = (err: unknown): boolean => { - const msg = (err instanceof Error ? err.message : String(err)).toLowerCase(); - return ( - msg.includes('busy') || - msg.includes('lock') || - msg.includes('already in use') || - msg.includes('could not set lock') - ); -}; - /** * Return true when the error message indicates a write was attempted against * a read-only LadybugDB connection. The MCP query pool opens DBs read-only, @@ -252,7 +240,11 @@ export const withLbugDb = async (dbPath: string, operation: () => Promise) }); } catch (err) { lastError = err; - if (!isDbBusyError(err) || attempt === DB_LOCK_RETRY_ATTEMPTS) { + // Skip outer retry when the inner open-retry already exhausted: the + // ~1.5s open-time budget was just spent, repeating the full reset+ + // reopen cycle would only add 4-5s of tail latency without changing + // the outcome (both layers consult the same isDbBusyError matcher). + if (!isDbBusyError(err) || isOpenRetryExhausted(err) || attempt === DB_LOCK_RETRY_ATTEMPTS) { throw err; } // Close stale connection inside the session lock to prevent race conditions @@ -330,7 +322,16 @@ const doInitLbug = async (dbPath: string) => { await conn.query(schemaQuery); } catch (err) { const msg = err instanceof Error ? err.message : String(err); - if (!msg.includes('already exists')) { + // Suppression list: + // - "already exists": expected idempotent re-create on existing DBs + // - "could not set lock on file": LadybugDB v0.16.1 emits this on + // Windows when CREATE NODE TABLE runs against a path that was + // just opened (the WAL handle from a fresh Database briefly + // contests the table's first-write lock). The table is created + // anyway and any genuine cross-process lock contention surfaces + // on the next operation via withLbugDb's retry. Logging it here + // would just be noise in CI. + if (!msg.includes('already exists') && !isDbBusyError(err)) { logger.warn(`⚠️ Schema creation warning: ${msg.slice(0, 120)}`); } } @@ -1064,6 +1065,9 @@ export const flushWAL = async (): Promise => { */ export const safeClose = async (): Promise => { await flushWAL(); + // Capture before close — currentDbPath stays set so the Windows post-close + // probe below knows which file to wait on. + const closingDbPath = currentDbPath; if (conn) { try { // eslint-disable-next-line no-restricted-syntax -- sole authorised close site @@ -1082,6 +1086,24 @@ export const safeClose = async (): Promise => { } db = null; } + // Windows: libuv reports `db.close()` resolved before the kernel has + // released the file handle. A subsequent `new Database(samePath)` in + // the same process can race the release. The probe (lbug-config.ts) + // forces any residual lock to surface as EBUSY/EPERM/EACCES so the + // open-time retry absorbs the lag. + if (process.platform === 'win32' && closingDbPath) { + const released = await waitForWindowsHandleRelease(closingDbPath); + if (!released) { + // Probe exhausted with a lock code still in flight. The next + // openLbugConnection will absorb whatever residual lag remains, but + // a chronic warning helps operators spot AV interference (Windows + // Defender holding the file far past the 250ms budget). + logger.warn( + { dbPath: closingDbPath }, + '⚠️ LadybugDB file handle still locked after close (Windows). If this repeats, check antivirus/Defender exclusions for the GitNexus storage directory.', + ); + } + } }; export const closeLbug = async (): Promise => { diff --git a/gitnexus/src/core/lbug/lbug-config.ts b/gitnexus/src/core/lbug/lbug-config.ts index a3e90051f..ceb445693 100644 --- a/gitnexus/src/core/lbug/lbug-config.ts +++ b/gitnexus/src/core/lbug/lbug-config.ts @@ -1,3 +1,6 @@ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; import type lbug from '@ladybugdb/core'; /** @@ -66,6 +69,28 @@ export interface LbugConnectionHandle { conn: lbug.Connection; } +/** + * Return true when the error message indicates that a LadybugDB file lock + * could not be acquired — either at construction time + * (`new lbug.Database(...)` raises from `local_file_system.cpp`) or during + * a query (another writer holds the exclusive lock). + * + * Lives here (not in `lbug-adapter.ts`) so both the construction-time + * retry (`openWithLockRetry` in this file) and the query-time retry + * (`withLbugDb` in `lbug-adapter.ts`) consult the same matcher. Callers + * import directly from this module — no re-export to keep in sync. + */ +export const isDbBusyError = (err: unknown): boolean => { + const msg = (err instanceof Error ? err.message : String(err)).toLowerCase(); + // `lock` already subsumes `could not set lock`; the broader term is kept + // because graph-DB transient errors include "deadlock", "lock contention", + // and the LadybugDB native module's "could not set lock on file" — all of + // which deserve a retry. If a non-transient lock-shaped error ever + // surfaces (e.g., "lock file missing" during recovery), tighten this + // matcher rather than raising the retry budget. + return msg.includes('busy') || msg.includes('lock') || msg.includes('already in use'); +}; + export function createLbugDatabase( lbugModule: LbugModule, databasePath: string, @@ -85,6 +110,159 @@ export function createLbugDatabase( ) as lbug.Database; } +// ─── Lock-busy retry tuning knobs ─────────────────────────────────────────── +// +// All four GitNexus retry pairs that touch native LadybugDB locks live with +// a comment cross-reference here so an SRE tuning Windows flakes finds them +// in one grep: +// +// 1. OPEN_LOCK_RETRY_ATTEMPTS / OPEN_LOCK_RETRY_DELAY_MS (this file) +// → `new lbug.Database()` constructor lock failures +// 2. HANDLE_RELEASE_PROBE_ATTEMPTS / HANDLE_RELEASE_PROBE_DELAY_MS (this file) +// → post-close fs.open probe to absorb Windows handle-release lag +// 3. DB_LOCK_RETRY_ATTEMPTS / DB_LOCK_RETRY_DELAY_MS (lbug-adapter.ts withLbugDb) +// → query-time busy/lock retry around already-open connections +// +// `new lbug.Database()` calls into the native module which performs an +// OS-level exclusive lock on ``. On Windows that lock can fail +// for reasons specific to the OS (Defender briefly opens new files, +// libuv handle release lags the JS-side close). 5 attempts × 100ms +// linear back-off (max sleep 100+200+300+400 = 1s, plus 5 ctor RTTs +// of 10–50ms each = ~1.0–1.2s worst case) clears the typical +// AV-scanner hold without masking real cross-process conflicts. +// +// Source: https://github.com/LadybugDB/ladybug/blob/v0.16.1/src/common/file_system/local_file_system.cpp#L126 +const OPEN_LOCK_RETRY_ATTEMPTS = 5; +const OPEN_LOCK_RETRY_DELAY_MS = 100; + +const HANDLE_RELEASE_PROBE_ATTEMPTS = 5; +const HANDLE_RELEASE_PROBE_DELAY_MS = 50; +const HANDLE_RELEASE_LOCK_CODES = new Set(['EBUSY', 'EPERM', 'EACCES']); + +/** + * Test-fixture directory prefixes recognized by `isTestFixturePath`. + * + * IMPORTANT: this list must stay in sync with the prefixes passed to + * `createTempDir` in `gitnexus/test/helpers/test-db.ts` and the prefixes + * used by `withTestLbugDB` (`gitnexus/test/helpers/test-indexed-db.ts`). + * If you add a new test that passes a custom prefix to `createTempDir`, + * add it here too — otherwise the stale-sidecar sweep silently won't + * fire for that fixture and CI flakes return. + * + * The default `createTempDir('gitnexus-test-')` and the lbug variant + * `'gitnexus-lbug-'` cover today's call sites. + */ +const TEST_FIXTURE_PREFIXES = ['gitnexus-lbug-', 'gitnexus-test-']; + +/** + * Marker symbol attached to lock errors after `openWithLockRetry` exhausts + * its budget. `withLbugDb`'s outer query-time retry consults this so it + * does not re-retry a path that just spent up to ~1.5s in the open-time + * loop — preventing 6s tail latencies (3× outer × 5× inner attempts). + * + * The symbol is internal to GitNexus; consumers should treat the underlying + * error message as the user-visible signal. + */ +export const LBUG_OPEN_RETRY_EXHAUSTED = Symbol.for('gitnexus.lbug.openRetryExhausted'); + +export const isOpenRetryExhausted = (err: unknown): boolean => { + if (err === null || err === undefined || typeof err !== 'object') return false; + return (err as { [LBUG_OPEN_RETRY_EXHAUSTED]?: boolean })[LBUG_OPEN_RETRY_EXHAUSTED] === true; +}; + +const tagOpenRetryExhausted = (err: unknown): unknown => { + if (err && typeof err === 'object') { + (err as { [LBUG_OPEN_RETRY_EXHAUSTED]?: boolean })[LBUG_OPEN_RETRY_EXHAUSTED] = true; + } + return err; +}; + +/** + * True when `dbPath` resolves to a recognized test fixture under the OS + * temp directory. Used to gate the stale-sidecar sweep so production + * paths never have their `.wal` / `.lock` files deleted. + * + * Defensive shape: + * - `path.resolve` normalizes `..` segments before the prefix check, so + * `/gitnexus-lbug-x/../../etc/passwd` is rejected. + * - The tmpRoot check trims any trailing separator returned by some + * Windows TMP configurations (`C:\Users\X\Temp\`) so the startsWith + * comparison stays correct. + * - Only the IMMEDIATE parent directory is matched against the prefix + * list. An ancestor walk would let a tmpdir whose own basename starts + * with `gitnexus-lbug-` accept arbitrary nested paths under it. + */ +const isTestFixturePath = (dbPath: string): boolean => { + const tmpRoot = os.tmpdir().replace(new RegExp(`${path.sep === '\\' ? '\\\\' : path.sep}+$`), ''); + const resolved = path.resolve(dbPath); + if (!resolved.startsWith(tmpRoot + path.sep) && resolved !== tmpRoot) return false; + const parentBase = path.basename(path.dirname(resolved)); + return TEST_FIXTURE_PREFIXES.some((p) => parentBase.startsWith(p)); +}; + +/** Exported only for direct unit testing — production callers use `openWithLockRetry`. */ +export const _isTestFixturePathForTest = isTestFixturePath; + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Attempt to remove stale `.wal` / `.lock` sidecars that a previous aborted + * test run may have left behind. Best-effort: ENOENT is normal, anything + * else is swallowed so the caller's retry can surface the original error. + */ +const sweepStaleSidecars = async (dbPath: string): Promise => { + for (const suffix of ['.wal', '.lock']) { + try { + await fs.unlink(dbPath + suffix); + } catch { + /* missing sidecar or permission error — let the open retry surface it */ + } + } +}; + +/** + * Run `construct` with bounded retries when `new lbug.Database(...)` throws + * a busy/lock error. The original (loop-captured) error is preferred over + * any post-sweep error so triage sees the real LadybugDB lock message. + * On exhaustion the rethrown error is tagged via + * `LBUG_OPEN_RETRY_EXHAUSTED` so the outer query-time retry in + * `withLbugDb` skips re-retrying a freshly-exhausted path. + */ +const openWithLockRetry = async ( + construct: () => lbug.Database, + dbPath: string, +): Promise => { + let originalLockError: unknown; + for (let attempt = 1; attempt <= OPEN_LOCK_RETRY_ATTEMPTS; attempt++) { + try { + return construct(); + } catch (err) { + if (!isDbBusyError(err)) throw err; + originalLockError = err; + if (attempt === OPEN_LOCK_RETRY_ATTEMPTS) break; + await sleep(OPEN_LOCK_RETRY_DELAY_MS * attempt); + } + } + + // Final defense: only for recognized test fixtures, sweep stale sidecars + // (a prior aborted test run can leave a `.wal` lock that survives the + // tmp dir cleanup). Production paths never reach this branch — the guard + // requires the immediate parent dir to match a test prefix AND the + // resolved path to live under the OS temp directory. + if (isTestFixturePath(dbPath)) { + await sweepStaleSidecars(dbPath); + try { + return construct(); + } catch { + // Intentionally do NOT overwrite originalLockError. The user-actionable + // signal is "we exhausted lock retries" — a different error from the + // post-sweep attempt is less useful than the lock failure that drove + // the sweep in the first place. + } + } + throw tagOpenRetryExhausted(originalLockError); +}; + export async function openLbugConnection( lbugModule: LbugModule, databasePath: string, @@ -92,7 +270,10 @@ export async function openLbugConnection( ): Promise { let db: lbug.Database | undefined; try { - db = createLbugDatabase(lbugModule, databasePath, options); + db = await openWithLockRetry( + () => createLbugDatabase(lbugModule, databasePath, options), + databasePath, + ); return { db, conn: new lbugModule.Connection(db) }; } catch (err) { if (db) await db.close().catch(() => {}); @@ -104,3 +285,60 @@ export async function closeLbugConnection(handle: LbugConnectionHandle): Promise await handle.conn.close().catch(() => {}); await handle.db.close().catch(() => {}); } + +/** + * Probe `dbPath` AND its `.wal` sidecar after `db.close()` so any + * residual native file handle surfaces as EBUSY/EPERM/EACCES and the + * bounded retry absorbs the release lag. Windows-only — Linux/macOS do + * not exhibit this race. + * + * Both files matter. Empirically, on rapid open→close→reopen cycles the + * main `dbPath` handle releases first; the `.wal` handle from the + * previous Database lingers and the new Database's first write (CREATE + * NODE TABLE during schema init) fails with "Could not set lock on + * file". Probing both makes safeClose actually return when the kernel + * is fully done with the path. + * + * Returns `true` when both probes succeeded (or skipped on non-lock + * errors / missing files). Returns `false` when either probe exhausted + * its budget with a lock code still in flight. + * + * Defensive shape: + * - Opens read+write (`'r+'`) so the probe actually surfaces exclusive + * locks held by the previous Database. A read-only probe (`'r'`) is + * insufficient — Windows will grant read access while the previous + * handle's exclusive write lock is still in flight, which lets + * `safeClose` return before the next CREATE NODE TABLE can lock the + * file. + * - `try/finally` around `handle.close()` guarantees no fd leak even + * if close itself throws. + */ +export const waitForWindowsHandleRelease = async (dbPath: string): Promise => { + const mainReleased = await probeSinglePath(dbPath); + const walReleased = await probeSinglePath(dbPath + '.wal'); + return mainReleased && walReleased; +}; + +const probeSinglePath = async (filePath: string): Promise => { + for (let attempt = 1; attempt <= HANDLE_RELEASE_PROBE_ATTEMPTS; attempt++) { + let handle: fs.FileHandle | undefined; + try { + handle = await fs.open(filePath, 'r+'); + return true; + } catch (err) { + const code = (err as NodeJS.ErrnoException | undefined)?.code; + if (!code || !HANDLE_RELEASE_LOCK_CODES.has(code)) return true; // ENOENT / unrelated → not our problem + if (attempt === HANDLE_RELEASE_PROBE_ATTEMPTS) return false; + await sleep(HANDLE_RELEASE_PROBE_DELAY_MS * attempt); + } finally { + if (handle) { + try { + await handle.close(); + } catch { + /* swallow — caller cannot do anything useful with a probe-close failure */ + } + } + } + } + return false; +}; diff --git a/gitnexus/test/helpers/test-db.ts b/gitnexus/test/helpers/test-db.ts index 5818fdc8e..37032ebc8 100644 --- a/gitnexus/test/helpers/test-db.ts +++ b/gitnexus/test/helpers/test-db.ts @@ -37,6 +37,13 @@ export async function cleanupTempDir(tmpDir: string): Promise { /** * Create a temporary directory for LadybugDB tests. * Returns the path and a cleanup function. + * + * IMPORTANT: when adding a new test that passes a custom `prefix`, also add + * the prefix to `TEST_FIXTURE_PREFIXES` in + * `gitnexus/src/core/lbug/lbug-config.ts`. The stale-sidecar sweep relies + * on the prefix list to recognize test fixtures; an unknown prefix means + * the sweep silently won't fire for that fixture and Windows CI flakes + * return. */ export async function createTempDir(prefix: string = 'gitnexus-test-'): Promise { const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); diff --git a/gitnexus/test/integration/lbug-close-handle-release.test.ts b/gitnexus/test/integration/lbug-close-handle-release.test.ts new file mode 100644 index 000000000..c0a3b8758 --- /dev/null +++ b/gitnexus/test/integration/lbug-close-handle-release.test.ts @@ -0,0 +1,41 @@ +/** + * Integration test: safeClose's Windows post-close handle-release wait. + * + * On Windows, libuv reports `db.close()` resolved before the kernel has + * released the file handle. A subsequent open of the same path can then + * race the release and surface "Could not set lock on file". `safeClose` + * probes the file with `fs.open` to force the residual lock to surface, + * absorbed by the open-time retry in `lbug-config.ts`. + */ +import path from 'path'; +import { describe, it } from 'vitest'; +import { createTempDir } from '../helpers/test-db.js'; + +describe('safeClose — close + reopen does not surface lock errors', () => { + it('survives 10 sequential open/close/reopen cycles on the same path', async () => { + const tmp = await createTempDir('gitnexus-lbug-close-cycle-'); + const dbPath = path.join(tmp.dbPath, 'lbug'); + try { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + for (let i = 0; i < 10; i++) { + await adapter.initLbug(dbPath); + await adapter.closeLbug(); + } + } finally { + await tmp.cleanup(); + } + }); + + it('safeClose is idempotent — calling twice in a row does not throw', async () => { + const tmp = await createTempDir('gitnexus-lbug-idempotent-'); + const dbPath = path.join(tmp.dbPath, 'lbug'); + try { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + await adapter.initLbug(dbPath); + await adapter.closeLbug(); + await adapter.closeLbug(); + } finally { + await tmp.cleanup(); + } + }); +}); diff --git a/gitnexus/test/integration/lbug-lock-retry.test.ts b/gitnexus/test/integration/lbug-lock-retry.test.ts index 1279c77fb..b95092e2d 100644 --- a/gitnexus/test/integration/lbug-lock-retry.test.ts +++ b/gitnexus/test/integration/lbug-lock-retry.test.ts @@ -14,7 +14,7 @@ import { withTestLbugDB } from '../helpers/test-indexed-db.js'; // Pure-function tests — no DB needed, but grouped here for cohesion // with the retry logic they guard. -import { isDbBusyError } from '../../src/core/lbug/lbug-adapter.js'; +import { isDbBusyError } from '../../src/core/lbug/lbug-config.js'; describe('isDbBusyError', () => { it('returns true for "busy" errors (case-insensitive)', () => { @@ -46,6 +46,18 @@ describe('isDbBusyError', () => { expect(isDbBusyError(undefined)).toBe(false); }); + // Documented behavior for lock-shaped strings: the matcher is intentionally + // broad because in graph-DB contexts these are all transient. If LadybugDB + // ever surfaces a non-transient lock-shaped error (e.g., a recovery-time + // "lock file missing"), tighten the matcher and add a negative test here + // rather than raising the retry budget. + it('treats other lock-shaped errors as transient (current intentional behavior)', () => { + expect(isDbBusyError(new Error('deadlock detected'))).toBe(true); + expect(isDbBusyError(new Error('unlock failed'))).toBe(true); + expect(isDbBusyError(new Error('lock contention'))).toBe(true); + expect(isDbBusyError(new Error('Could not open lock file'))).toBe(true); + }); + it('handles non-Error values gracefully', () => { expect(isDbBusyError('BUSY error')).toBe(true); expect(isDbBusyError(42)).toBe(false); diff --git a/gitnexus/test/integration/lbug-open-retry.test.ts b/gitnexus/test/integration/lbug-open-retry.test.ts new file mode 100644 index 000000000..80e68617d --- /dev/null +++ b/gitnexus/test/integration/lbug-open-retry.test.ts @@ -0,0 +1,310 @@ +/** + * Integration tests: open-time lock-busy retry in `lbug-config.ts`. + * + * The lock IO exception raised by `local_file_system.cpp` happens + * synchronously inside `new lbug.Database(...)`, before any query is + * issued — so `withLbugDb`'s query-time retry cannot see it. These tests + * exercise the construction-time retry wrapper directly by stubbing the + * `Database` constructor. + * + * See: docs/plans/2026-05-08-002-fix-windows-lbug-lock-ci-flakes-plan.md + */ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { + _isTestFixturePathForTest as isTestFixturePath, + isDbBusyError, + isOpenRetryExhausted, + openLbugConnection, + waitForWindowsHandleRelease, +} from '../../src/core/lbug/lbug-config.js'; + +// ─── Minimal stub of the `lbug` module surface used by openLbugConnection ── + +interface StubModuleControl { + /** Errors thrown by sequential `new Database(...)` calls. `null` = success. */ + databaseThrows: Array; + /** Number of times the `Database` constructor was invoked. */ + databaseCallCount: number; + /** Number of times `db.close()` was called. */ + closeCallCount: number; +} + +const makeStubLbug = (control: StubModuleControl) => { + class FakeDatabase { + constructor(_path: string, ..._rest: unknown[]) { + control.databaseCallCount++; + const next = control.databaseThrows.shift(); + if (next instanceof Error) throw next; + } + async close(): Promise { + control.closeCallCount++; + } + } + class FakeConnection { + constructor(_db: FakeDatabase) {} + async close(): Promise {} + } + return { Database: FakeDatabase, Connection: FakeConnection } as any; +}; + +describe('isDbBusyError', () => { + it('matches the documented Windows lock-error wording', () => { + expect(isDbBusyError(new Error('Could not set lock on file foo.lbug'))).toBe(true); + expect(isDbBusyError(new Error('database is locked'))).toBe(true); + }); + it('does not match unrelated errors', () => { + expect(isDbBusyError(new Error('Cypher syntax error'))).toBe(false); + expect(isDbBusyError(null)).toBe(false); + }); +}); + +describe('openLbugConnection — open-time lock-busy retry', () => { + it('returns a handle when the constructor succeeds on the first try', async () => { + const control: StubModuleControl = { + databaseThrows: [null], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + const handle = await openLbugConnection(stub, '/some/path/lbug'); + expect(handle.db).toBeDefined(); + expect(handle.conn).toBeDefined(); + expect(control.databaseCallCount).toBe(1); + }); + + it('retries on busy/lock errors and succeeds on a later attempt', async () => { + const control: StubModuleControl = { + databaseThrows: [new Error('Could not set lock on file'), null], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + const handle = await openLbugConnection(stub, '/some/path/lbug'); + expect(handle.db).toBeDefined(); + expect(control.databaseCallCount).toBe(2); + }); + + it('exhausts the retry budget and rethrows the last error preserving its message', async () => { + const lockErr = new Error('Could not set lock on file foo.lbug'); + const control: StubModuleControl = { + // 5 attempts + production paths get no sweep retry, so 5 throws total. + databaseThrows: [lockErr, lockErr, lockErr, lockErr, lockErr], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + await expect(openLbugConnection(stub, '/var/data/non-test/lbug')).rejects.toThrow( + 'Could not set lock on file foo.lbug', + ); + expect(control.databaseCallCount).toBe(5); + }); + + it('tags the exhausted error so withLbugDb skips its outer retry', async () => { + const lockErr = new Error('Could not set lock on file'); + const control: StubModuleControl = { + databaseThrows: [lockErr, lockErr, lockErr, lockErr, lockErr], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + let caught: unknown; + try { + await openLbugConnection(stub, '/var/data/non-test/lbug'); + } catch (err) { + caught = err; + } + expect(caught).toBeDefined(); + expect(isOpenRetryExhausted(caught)).toBe(true); + expect(isOpenRetryExhausted(new Error('plain error'))).toBe(false); + expect(isOpenRetryExhausted(null)).toBe(false); + expect(isOpenRetryExhausted(undefined)).toBe(false); + }); + + it('does not retry non-busy errors', async () => { + const syntaxErr = new Error('Cypher syntax error'); + const control: StubModuleControl = { + databaseThrows: [syntaxErr], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + await expect(openLbugConnection(stub, '/some/path/lbug')).rejects.toThrow( + 'Cypher syntax error', + ); + expect(control.databaseCallCount).toBe(1); + }); +}); + +describe('openLbugConnection — stale-sidecar sweep (test fixtures only)', () => { + let fixtureDir: string; + let dbPath: string; + + beforeEach(async () => { + fixtureDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gitnexus-lbug-sweep-')); + dbPath = path.join(fixtureDir, 'lbug'); + }); + + afterEach(async () => { + await fs.rm(fixtureDir, { recursive: true, force: true }).catch(() => {}); + }); + + it('sweeps stale .wal/.lock for a recognized test fixture path and retries once', async () => { + await fs.writeFile(dbPath + '.wal', 'stale'); + await fs.writeFile(dbPath + '.lock', 'stale'); + + const lockErr = new Error('Could not set lock on file'); + const control: StubModuleControl = { + // 5 retries throw, then sweep + 1 final attempt succeeds (6 total). + databaseThrows: [lockErr, lockErr, lockErr, lockErr, lockErr, null], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + + const handle = await openLbugConnection(stub, dbPath); + expect(handle.db).toBeDefined(); + expect(control.databaseCallCount).toBe(6); + + // Sidecars removed by the sweep + await expect(fs.access(dbPath + '.wal')).rejects.toThrow(); + await expect(fs.access(dbPath + '.lock')).rejects.toThrow(); + }); + + it('does not sweep production paths even if they share the prefix', async () => { + // A non-tmp dir that *starts* with the prefix must still be rejected. + const lockErr = new Error('Could not set lock on file'); + const control: StubModuleControl = { + databaseThrows: [lockErr, lockErr, lockErr, lockErr, lockErr], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + + // Path is outside os.tmpdir() so the predicate must reject it. + await expect(openLbugConnection(stub, '/var/data/gitnexus-lbug-fake/lbug')).rejects.toThrow( + 'Could not set lock on file', + ); + expect(control.databaseCallCount).toBe(5); // no sweep retry + }); + + it('handles missing sidecars gracefully (ENOENT swallowed, retry runs)', async () => { + // No .wal or .lock pre-created — sweep ENOENTs both, then succeeds. + const lockErr = new Error('Could not set lock on file'); + const control: StubModuleControl = { + databaseThrows: [lockErr, lockErr, lockErr, lockErr, lockErr, null], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + + const handle = await openLbugConnection(stub, dbPath); + expect(handle.db).toBeDefined(); + expect(control.databaseCallCount).toBe(6); + }); + + it('sweep retry that throws a different error preserves the original lock error', async () => { + // 5 lock errors, then sweep fires, then post-sweep throws an unrelated + // error. The user-actionable signal is "lock retries exhausted" — the + // post-sweep error must NOT shadow the original lock message. + const lockErr = new Error('Could not set lock on file foo.lbug'); + const unrelatedErr = new Error('Schema validation error during open'); + const control: StubModuleControl = { + databaseThrows: [lockErr, lockErr, lockErr, lockErr, lockErr, unrelatedErr], + databaseCallCount: 0, + closeCallCount: 0, + }; + const stub = makeStubLbug(control); + + let caught: Error | undefined; + try { + await openLbugConnection(stub, dbPath); + } catch (err) { + caught = err as Error; + } + expect(caught?.message).toBe('Could not set lock on file foo.lbug'); + expect(control.databaseCallCount).toBe(6); // sweep retry did fire + }); +}); + +describe('isTestFixturePath — production-safety guard', () => { + it('accepts a fixture under os.tmpdir with a recognized prefix on the immediate parent', () => { + const tmp = os.tmpdir(); + expect(isTestFixturePath(path.join(tmp, 'gitnexus-lbug-XXX', 'lbug'))).toBe(true); + expect(isTestFixturePath(path.join(tmp, 'gitnexus-test-YYY', 'lbug'))).toBe(true); + }); + + it('rejects production paths even with a matching prefix', () => { + expect(isTestFixturePath('/var/data/gitnexus-lbug-fake/lbug')).toBe(false); + expect(isTestFixturePath('/home/user/gitnexus-test-foo/lbug')).toBe(false); + }); + + it('rejects path traversal attempts that resolve outside tmpdir', () => { + const tmp = os.tmpdir(); + const traversal = path.join(tmp, 'gitnexus-lbug-x', '..', '..', 'etc', 'passwd'); + expect(isTestFixturePath(traversal)).toBe(false); + }); + + it('rejects when the immediate parent does not match even if a deeper ancestor does', () => { + // Tightening: ancestor walk would have allowed nested paths under + // `/gitnexus-lbug-x/inner/lbug` to satisfy the predicate. We + // require the immediate parent to match. + const tmp = os.tmpdir(); + expect(isTestFixturePath(path.join(tmp, 'gitnexus-lbug-x', 'inner', 'lbug'))).toBe(false); + }); + + it('handles tmpdir trailing-separator gracefully', () => { + // Some Windows TMP configs return a trailing separator; the predicate + // strips it before the prefix check so fixtures still match. + const tmp = os.tmpdir(); + const fixture = path.join(tmp, 'gitnexus-lbug-trailing', 'lbug'); + // Whether or not os.tmpdir() itself has a trailing separator, + // the predicate must accept legit fixtures. + expect(isTestFixturePath(fixture)).toBe(true); + }); + + it('rejects unrelated prefixes in tmpdir', () => { + const tmp = os.tmpdir(); + expect(isTestFixturePath(path.join(tmp, 'random-dir', 'lbug'))).toBe(false); + expect(isTestFixturePath(path.join(tmp, 'malicious', 'lbug'))).toBe(false); + }); +}); + +describe('waitForWindowsHandleRelease', () => { + let fixtureDir: string; + let dbPath: string; + + beforeEach(async () => { + fixtureDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gitnexus-lbug-probe-')); + dbPath = path.join(fixtureDir, 'lbug'); + }); + + afterEach(async () => { + await fs.rm(fixtureDir, { recursive: true, force: true }).catch(() => {}); + }); + + it('returns true when the file exists and is openable', async () => { + await fs.writeFile(dbPath, 'fake-db-content'); + const released = await waitForWindowsHandleRelease(dbPath); + expect(released).toBe(true); + }); + + it('returns true when the file does not exist (ENOENT is non-lock)', async () => { + // No fs.writeFile — path does not exist. Probe should bail to true, + // not retry, since ENOENT is not a lock code. + const released = await waitForWindowsHandleRelease(dbPath); + expect(released).toBe(true); + }); + + it('does not leak the file handle when close succeeds', async () => { + // Smoke test: 50 sequential probes with a real file. If close were + // skipped, fd usage would climb. We rely on test process not OOMing + // as the simplest indicator; fd table caps catch egregious leaks. + await fs.writeFile(dbPath, 'fake-db-content'); + for (let i = 0; i < 50; i++) { + await waitForWindowsHandleRelease(dbPath); + } + }); +}); diff --git a/gitnexus/vitest.config.ts b/gitnexus/vitest.config.ts index 9330e86a5..862357668 100644 --- a/gitnexus/vitest.config.ts +++ b/gitnexus/vitest.config.ts @@ -60,6 +60,8 @@ export default defineConfig({ 'test/integration/augmentation.test.ts', 'test/integration/staleness-and-stability.test.ts', 'test/integration/lbug-lock-retry.test.ts', + 'test/integration/lbug-open-retry.test.ts', + 'test/integration/lbug-close-handle-release.test.ts', 'test/integration/api-impact-e2e.test.ts', 'test/integration/shape-check-regression.test.ts', 'test/integration/java-class-impact.test.ts', @@ -87,6 +89,8 @@ export default defineConfig({ 'test/integration/augmentation.test.ts', 'test/integration/staleness-and-stability.test.ts', 'test/integration/lbug-lock-retry.test.ts', + 'test/integration/lbug-open-retry.test.ts', + 'test/integration/lbug-close-handle-release.test.ts', 'test/integration/api-impact-e2e.test.ts', 'test/integration/shape-check-regression.test.ts', 'test/integration/java-class-impact.test.ts', From 5497079ab202d45061bce1b265645f31ec6f19fd Mon Sep 17 00:00:00 2001 From: azizur100389 Date: Fri, 8 May 2026 17:05:18 +0100 Subject: [PATCH 4/4] fix(search): surface warning when FTS indexes are missing (#1418) --- gitnexus/src/core/augmentation/engine.ts | 2 +- gitnexus/src/core/search/bm25-index.ts | 47 ++++++++++++++----- gitnexus/src/core/search/hybrid-search.ts | 11 +++-- gitnexus/src/mcp/local/local-backend.ts | 9 ++-- gitnexus/src/server/api.ts | 23 ++++++--- gitnexus/test/integration/search-core.test.ts | 24 +++++----- gitnexus/test/integration/search-pool.test.ts | 16 +++---- gitnexus/test/unit/bm25-search.test.ts | 26 +++++----- gitnexus/test/unit/calltool-dispatch.test.ts | 23 ++++++++- .../test/unit/mcp/group-repo-routing.test.ts | 2 +- 10 files changed, 121 insertions(+), 62 deletions(-) diff --git a/gitnexus/src/core/augmentation/engine.ts b/gitnexus/src/core/augmentation/engine.ts index 896813c22..f97415cc9 100644 --- a/gitnexus/src/core/augmentation/engine.ts +++ b/gitnexus/src/core/augmentation/engine.ts @@ -104,7 +104,7 @@ export async function augment(pattern: string, cwd?: string): Promise { } // Step 1: BM25 search (fast, no embeddings) - const bm25Results = await searchFTSFromLbug(pattern, 10, repoId); + const { results: bm25Results } = await searchFTSFromLbug(pattern, 10, repoId); if (bm25Results.length === 0) return ''; diff --git a/gitnexus/src/core/search/bm25-index.ts b/gitnexus/src/core/search/bm25-index.ts index 58c0343c9..27a7b9d8d 100644 --- a/gitnexus/src/core/search/bm25-index.ts +++ b/gitnexus/src/core/search/bm25-index.ts @@ -15,9 +15,16 @@ export interface BM25SearchResult { nodeIds?: string[]; } +export interface FTSSearchResponse { + results: BM25SearchResult[]; + /** True when at least one FTS index query succeeded (index exists). */ + ftsAvailable: boolean; +} + /** * Execute a single FTS query via a custom executor (for MCP connection pool). - * Returns the same shape as core queryFTS (from LadybugDB adapter). + * Returns `null` when the query fails (e.g. FTS index does not exist) so the + * caller can distinguish "zero matches" from "index missing". */ async function queryFTSViaExecutor( executor: (cypher: string) => Promise, @@ -25,7 +32,7 @@ async function queryFTSViaExecutor( indexName: string, query: string, limit: number, -): Promise> { +): Promise | null> { // Escape single quotes and backslashes to prevent Cypher injection const escapedQuery = query.replace(/\\/g, '\\\\').replace(/'/g, "''"); const cypher = ` @@ -46,7 +53,7 @@ async function queryFTSViaExecutor( }; }); } catch { - return []; + return null; } } @@ -65,8 +72,9 @@ export const searchFTSFromLbug = async ( query: string, limit: number = 20, repoId?: string, -): Promise => { +): Promise => { const resultsByIndex: any[][] = []; + let queriesSucceeded = 0; if (repoId) { // Use MCP connection pool via dynamic import @@ -77,15 +85,27 @@ export const searchFTSFromLbug = async ( const executor = (cypher: string) => executeQuery(repoId, cypher); for (const { table, indexName } of FTS_INDEXES) { - resultsByIndex.push(await queryFTSViaExecutor(executor, table, indexName, query, limit)); + const result = await queryFTSViaExecutor(executor, table, indexName, query, limit); + if (result !== null) { + queriesSucceeded++; + resultsByIndex.push(result); + } } } else { // Use core lbug adapter (CLI / pipeline context) — also sequential for safety. for (const { table, indexName } of FTS_INDEXES) { - resultsByIndex.push(await queryFTS(table, indexName, query, limit, false).catch(() => [])); + try { + const result = await queryFTS(table, indexName, query, limit, false); + queriesSucceeded++; + resultsByIndex.push(result); + } catch { + // FTS index may not exist — count as failed + } } } + const ftsAvailable = queriesSucceeded > 0; + // Collect all node scores per filePath to track which nodes actually matched const fileNodeScores = new Map>(); @@ -116,10 +136,13 @@ export const searchFTSFromLbug = async ( .sort((a, b) => b.score - a.score) .slice(0, limit); - return sorted.map((r, index) => ({ - filePath: r.filePath, - score: r.score, - rank: index + 1, - nodeIds: r.nodeIds, - })); + return { + results: sorted.map((r, index) => ({ + filePath: r.filePath, + score: r.score, + rank: index + 1, + nodeIds: r.nodeIds, + })), + ftsAvailable, + }; }; diff --git a/gitnexus/src/core/search/hybrid-search.ts b/gitnexus/src/core/search/hybrid-search.ts index 72dd1c9b5..b76a9f5e9 100644 --- a/gitnexus/src/core/search/hybrid-search.ts +++ b/gitnexus/src/core/search/hybrid-search.ts @@ -113,12 +113,13 @@ export const mergeWithRRF = ( }; /** - * Check if hybrid search is available - * LadybugDB FTS is always available once the database is initialized. - * Semantic search is optional - hybrid works with just FTS if embeddings aren't ready. + * Check if hybrid search is available. + * FTS indexes may be missing on read-only MCP connections (see #1403); + * callers should inspect `ftsAvailable` from searchFTSFromLbug for + * per-query availability. This helper is a coarse gate only. */ export const isHybridSearchReady = (): boolean => { - return true; // FTS is always available via LadybugDB when DB is open + return true; // FTS is attempted on every query; ftsAvailable signals actual availability }; /** @@ -160,7 +161,7 @@ export const hybridSearch = async ( ) => Promise, ): Promise => { // Use LadybugDB FTS for always-fresh BM25 results - const bm25Results = await searchFTSFromLbug(query, limit); + const { results: bm25Results } = await searchFTSFromLbug(query, limit); const semanticResults = await semanticSearch(executeQuery, query, limit); return mergeWithRRF(bm25Results, semanticResults, limit); }; diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 2f16c1fb2..6175af875 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -979,7 +979,7 @@ export class LocalBackend { timing, ...(!ftsUsed && { warning: - 'FTS extension unavailable - keyword search degraded. Run: gitnexus analyze --force to rebuild indexes.', + 'FTS indexes missing — keyword search degraded. Run: gitnexus analyze --force to rebuild indexes.', }), }; } @@ -993,9 +993,9 @@ export class LocalBackend { limit: number, ): Promise<{ results: any[]; ftsUsed: boolean }> { const { searchFTSFromLbug } = await import('../../core/search/bm25-index.js'); - let bm25Results; + let ftsResponse; try { - bm25Results = await searchFTSFromLbug(query, limit, repo.id); + ftsResponse = await searchFTSFromLbug(query, limit, repo.id); } catch (err: any) { logger.error( { err: err.message }, @@ -1004,7 +1004,8 @@ export class LocalBackend { return { results: [], ftsUsed: false }; } - const ftsUsed = bm25Results.length === 0 || bm25Results[0]?.ftsUsed !== false; + const bm25Results = ftsResponse.results; + const ftsUsed = ftsResponse.ftsAvailable; const results: any[] = []; diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 773190785..cc65daa3d 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -1060,11 +1060,12 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => const results = await withLbugDb(lbugPath, async () => { let searchResults: any[]; + let ftsAvailable: boolean | undefined; if (mode === 'semantic') { const { isEmbedderReady } = await import('../core/embeddings/embedder.js'); if (!isEmbedderReady()) { - return [] as any[]; + return { searchResults: [] as any[], ftsAvailable: undefined }; } const { semanticSearch: semSearch } = await import('../core/embeddings/embedding-pipeline.js'); @@ -1077,8 +1078,9 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => sources: ['semantic'], })); } else if (mode === 'bm25') { - searchResults = await searchFTSFromLbug(query, limit); - searchResults = searchResults.map((r: any, i: number) => ({ + const ftsResponse = await searchFTSFromLbug(query, limit); + ftsAvailable = ftsResponse.ftsAvailable; + searchResults = ftsResponse.results.map((r: any, i: number) => ({ ...r, rank: i + 1, sources: ['bm25'], @@ -1091,11 +1093,13 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => await import('../core/embeddings/embedding-pipeline.js'); searchResults = await hybridSearch(query, limit, executeQuery, semSearch); } else { - searchResults = await searchFTSFromLbug(query, limit); + const ftsResponse = await searchFTSFromLbug(query, limit); + ftsAvailable = ftsResponse.ftsAvailable; + searchResults = ftsResponse.results; } } - if (!enrich) return searchResults; + if (!enrich) return { searchResults, ftsAvailable }; // Server-side enrichment: add connections, cluster, processes per result // Uses parameterized queries to prevent Cypher injection via nodeId @@ -1177,9 +1181,14 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => }), ); - return enriched; + return { searchResults: enriched, ftsAvailable }; }); - res.json({ results }); + const response: any = { results: results.searchResults ?? results }; + if (results.ftsAvailable === false) { + response.warning = + 'FTS indexes missing — keyword search degraded. Run: gitnexus analyze --force to rebuild indexes.'; + } + res.json(response); } catch (err: any) { res.status(500).json({ error: err.message || 'Search failed' }); } diff --git a/gitnexus/test/integration/search-core.test.ts b/gitnexus/test/integration/search-core.test.ts index 2ccc14706..e49169b30 100644 --- a/gitnexus/test/integration/search-core.test.ts +++ b/gitnexus/test/integration/search-core.test.ts @@ -19,7 +19,7 @@ withTestLbugDB( (_handle) => { describe('searchFTSFromLbug — core adapter (no repoId)', () => { it('returns ranked results for a matching query', async () => { - const results = await searchFTSFromLbug('user authentication', 10); + const { results } = await searchFTSFromLbug('user authentication', 10); expect(results.length).toBeGreaterThan(0); @@ -40,7 +40,7 @@ withTestLbugDB( }); it('results are ordered by descending score', async () => { - const results = await searchFTSFromLbug('user authentication', 10); + const { results } = await searchFTSFromLbug('user authentication', 10); for (let i = 1; i < results.length; i++) { expect(results[i - 1].score).toBeGreaterThanOrEqual(results[i].score); @@ -48,7 +48,7 @@ withTestLbugDB( }); it('auth-related files rank higher than unrelated files', async () => { - const results = await searchFTSFromLbug('user authentication', 10); + const { results } = await searchFTSFromLbug('user authentication', 10); const filePaths = results.map((r) => r.filePath); expect(filePaths).toContain('src/auth.ts'); @@ -61,7 +61,7 @@ withTestLbugDB( }); it('merges scores from multiple node types for the same filePath', async () => { - const results = await searchFTSFromLbug('user authentication', 20); + const { results } = await searchFTSFromLbug('user authentication', 20); const authResult = results.find((r) => r.filePath === 'src/auth.ts'); expect(authResult).toBeDefined(); @@ -73,12 +73,12 @@ withTestLbugDB( }); it('respects limit parameter', async () => { - const results = await searchFTSFromLbug('user authentication', 2); + const { results } = await searchFTSFromLbug('user authentication', 2); expect(results.length).toBeLessThanOrEqual(2); }); it('returns empty array for a non-matching query', async () => { - const results = await searchFTSFromLbug('xyzzyplughtwisty', 10); + const { results } = await searchFTSFromLbug('xyzzyplughtwisty', 10); expect(results).toEqual([]); }); }); @@ -87,32 +87,32 @@ withTestLbugDB( describe('unhappy paths', () => { it('returns empty array for empty query string', async () => { - const results = await searchFTSFromLbug('', 10); + const { results } = await searchFTSFromLbug('', 10); expect(results).toEqual([]); }); it('returns empty array for whitespace-only query', async () => { - const results = await searchFTSFromLbug(' ', 10); + const { results } = await searchFTSFromLbug(' ', 10); expect(results).toEqual([]); }); it('handles special characters in query gracefully', async () => { - const results = await searchFTSFromLbug('user* OR auth+', 10); + const { results } = await searchFTSFromLbug('user* OR auth+', 10); expect(Array.isArray(results)).toBe(true); }); it('handles limit of 0', async () => { - const results = await searchFTSFromLbug('user authentication', 0); + const { results } = await searchFTSFromLbug('user authentication', 0); expect(results).toEqual([]); }); it('handles negative limit gracefully', async () => { - const results = await searchFTSFromLbug('user authentication', -1); + const { results } = await searchFTSFromLbug('user authentication', -1); expect(Array.isArray(results)).toBe(true); }); it('handles very large limit', async () => { - const results = await searchFTSFromLbug('user authentication', 100000); + const { results } = await searchFTSFromLbug('user authentication', 100000); expect(results.length).toBeLessThanOrEqual(100000); expect(results.length).toBeGreaterThan(0); }); diff --git a/gitnexus/test/integration/search-pool.test.ts b/gitnexus/test/integration/search-pool.test.ts index c0943483b..88128fcfa 100644 --- a/gitnexus/test/integration/search-pool.test.ts +++ b/gitnexus/test/integration/search-pool.test.ts @@ -19,7 +19,7 @@ withTestLbugDB( (handle) => { describe('searchFTSFromLbug — MCP pool adapter (with repoId)', () => { it('returns ranked results via pool adapter', async () => { - const results = await searchFTSFromLbug('user authentication', 10, handle.repoId); + const { results } = await searchFTSFromLbug('user authentication', 10, handle.repoId); expect(results.length).toBeGreaterThan(0); @@ -35,7 +35,7 @@ withTestLbugDB( }); it('results are ordered by descending score via pool adapter', async () => { - const results = await searchFTSFromLbug('user authentication', 10, handle.repoId); + const { results } = await searchFTSFromLbug('user authentication', 10, handle.repoId); for (let i = 1; i < results.length; i++) { expect(results[i - 1].score).toBeGreaterThanOrEqual(results[i].score); @@ -43,12 +43,12 @@ withTestLbugDB( }); it('returns empty array for non-matching query via pool adapter', async () => { - const results = await searchFTSFromLbug('xyzzyplughtwisty', 10, handle.repoId); + const { results } = await searchFTSFromLbug('xyzzyplughtwisty', 10, handle.repoId); expect(results).toEqual([]); }); it('respects limit parameter via pool adapter', async () => { - const results = await searchFTSFromLbug('user authentication', 1, handle.repoId); + const { results } = await searchFTSFromLbug('user authentication', 1, handle.repoId); expect(results.length).toBeLessThanOrEqual(1); }); }); @@ -57,22 +57,22 @@ withTestLbugDB( describe('unhappy paths', () => { it('returns empty array for empty query via pool', async () => { - const results = await searchFTSFromLbug('', 10, handle.repoId); + const { results } = await searchFTSFromLbug('', 10, handle.repoId); expect(results).toEqual([]); }); it('returns empty array for whitespace-only query via pool', async () => { - const results = await searchFTSFromLbug(' ', 10, handle.repoId); + const { results } = await searchFTSFromLbug(' ', 10, handle.repoId); expect(results).toEqual([]); }); it('handles special characters in query via pool', async () => { - const results = await searchFTSFromLbug('user* OR auth+', 10, handle.repoId); + const { results } = await searchFTSFromLbug('user* OR auth+', 10, handle.repoId); expect(Array.isArray(results)).toBe(true); }); it('handles limit of 0 via pool', async () => { - const results = await searchFTSFromLbug('user authentication', 0, handle.repoId); + const { results } = await searchFTSFromLbug('user authentication', 0, handle.repoId); expect(results).toEqual([]); }); }); diff --git a/gitnexus/test/unit/bm25-search.test.ts b/gitnexus/test/unit/bm25-search.test.ts index 9b232688f..03a591599 100644 --- a/gitnexus/test/unit/bm25-search.test.ts +++ b/gitnexus/test/unit/bm25-search.test.ts @@ -42,20 +42,24 @@ describe('BM25 search', () => { }); describe('searchFTSFromLbug', () => { - it('returns empty array when LadybugDB is not initialized', async () => { - // Without LadybugDB init, search should return empty (not crash) - const results = await searchFTSFromLbug('test query'); + it('returns empty results when LadybugDB is not initialized', async () => { + // Simulate an uninitialized DB: queryFTS throws instead of returning rows + const { queryFTS } = await import('../../src/core/lbug/lbug-adapter.js'); + vi.mocked(queryFTS).mockRejectedValue(new Error('DB not initialized')); + + const { results, ftsAvailable } = await searchFTSFromLbug('test query'); expect(Array.isArray(results)).toBe(true); expect(results).toHaveLength(0); + expect(ftsAvailable).toBe(false); }); it('handles empty query', async () => { - const results = await searchFTSFromLbug(''); + const { results } = await searchFTSFromLbug(''); expect(Array.isArray(results)).toBe(true); }); it('accepts custom limit parameter', async () => { - const results = await searchFTSFromLbug('test', 5); + const { results } = await searchFTSFromLbug('test', 5); expect(Array.isArray(results)).toBe(true); }); }); @@ -105,7 +109,7 @@ describe('BM25 search', () => { .mockResolvedValueOnce([]) // Method .mockResolvedValueOnce([]); // Interface - const results = await searchFTSFromLbug('queryset'); + const { results } = await searchFTSFromLbug('queryset'); expect(results).toHaveLength(1); expect(results[0].filePath).toBe('src/views.py'); @@ -127,7 +131,7 @@ describe('BM25 search', () => { .mockResolvedValueOnce([]) // Method .mockResolvedValueOnce([]); // Interface - const results = await searchFTSFromLbug('model'); + const { results } = await searchFTSFromLbug('model'); expect(results).toHaveLength(1); expect(results[0].score).toBe(8); // 5+3 @@ -147,7 +151,7 @@ describe('BM25 search', () => { .mockResolvedValueOnce([]) // Method .mockResolvedValueOnce([]); // Interface - const results = await searchFTSFromLbug('util'); + const { results } = await searchFTSFromLbug('util'); expect(results).toHaveLength(1); expect(results[0].nodeIds).toEqual([]); @@ -171,7 +175,7 @@ describe('BM25 search', () => { .mockResolvedValueOnce([]) // Method .mockResolvedValueOnce([]); // Interface - const results = await searchFTSFromLbug('auth'); + const { results } = await searchFTSFromLbug('auth'); expect(results).toHaveLength(1); // All 3 hits (scores 9+7+4=20) — each from a different table, all top-3 @@ -192,7 +196,7 @@ describe('BM25 search', () => { .mockResolvedValueOnce([]) // Method .mockResolvedValueOnce([]); // Interface - const results = await searchFTSFromLbug('fn'); + const { results } = await searchFTSFromLbug('fn'); expect(results[0].filePath).toBe('src/high.py'); expect(results[1].filePath).toBe('src/low.py'); @@ -220,7 +224,7 @@ describe('BM25 search', () => { return []; }); - const results = await searchFTSFromLbug('login', 5, REPO); + const { results } = await searchFTSFromLbug('login', 5, REPO); expect(results).toEqual([ { filePath: 'src/auth.ts', score: 8, rank: 1, nodeIds: ['func:login'] }, diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 6b13cacfa..45e1b71d2 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -62,7 +62,7 @@ vi.mock('../../src/core/platform/capabilities.js', async (importOriginal) => { // Also mock the search modules to avoid loading onnxruntime vi.mock('../../src/core/search/bm25-index.js', () => ({ - searchFTSFromLbug: vi.fn().mockResolvedValue([]), + searchFTSFromLbug: vi.fn().mockResolvedValue({ results: [], ftsAvailable: true }), })); vi.mock('../../src/mcp/core/embedder.js', () => ({ @@ -195,6 +195,27 @@ describe('LocalBackend.callTool', () => { expect(result).toHaveProperty('definitions'); }); + it('includes FTS-unavailable warning when ftsAvailable is false (#1403)', async () => { + const { searchFTSFromLbug } = await import('../../src/core/search/bm25-index.js'); + vi.mocked(searchFTSFromLbug).mockResolvedValueOnce({ results: [], ftsAvailable: false }); + (executeParameterized as any).mockResolvedValue([]); + + const result = await backend.callTool('query', { query: 'ProcessActivity' }); + + expect(result).toHaveProperty('warning'); + expect((result as any).warning).toMatch(/gitnexus analyze --force/); + }); + + it('does not include warning when ftsAvailable is true with zero results', async () => { + const { searchFTSFromLbug } = await import('../../src/core/search/bm25-index.js'); + vi.mocked(searchFTSFromLbug).mockResolvedValueOnce({ results: [], ftsAvailable: true }); + (executeParameterized as any).mockResolvedValue([]); + + const result = await backend.callTool('query', { query: 'nonexistent' }); + + expect(result).not.toHaveProperty('warning'); + }); + it('skips vector index query when VECTOR is unsupported by the platform', async () => { const cap = _captureLogger(); platformMocks.isVectorExtensionSupportedByPlatform.mockReturnValue(false); diff --git a/gitnexus/test/unit/mcp/group-repo-routing.test.ts b/gitnexus/test/unit/mcp/group-repo-routing.test.ts index ddf7d03a5..474abeaa8 100644 --- a/gitnexus/test/unit/mcp/group-repo-routing.test.ts +++ b/gitnexus/test/unit/mcp/group-repo-routing.test.ts @@ -32,7 +32,7 @@ vi.mock('../../../src/storage/repo-manager.js', () => ({ })); vi.mock('../../../src/core/search/bm25-index.js', () => ({ - searchFTSFromLbug: vi.fn().mockResolvedValue([]), + searchFTSFromLbug: vi.fn().mockResolvedValue({ results: [], ftsAvailable: true }), })); vi.mock('../../../src/mcp/core/embedder.js', () => ({