fix(search): load FTS during core DB init#1123
Conversation
Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @magyargergo's task in 3m 36s —— View job PR #1123 Review: fix(search): load FTS during core DB initTasks:
Review bar for this PR
Understanding of the changeIssue #1071: HTTP/Web-style search sessions were calling Root cause: Fix: One line added to Affected paths:
Must remain unchanged: VECTOR extension loading, existing FTS/BM25 index creation, Findings[low]
|
| DoD item | Status |
|---|---|
initLbug() loads FTS for every reopened session |
✅ satisfied — doInitLbug now calls await loadFTSExtension() unconditionally before await loadVectorExtension() |
| Vector extension init unchanged / not regressed | ✅ satisfied — loadVectorExtension() is still called immediately after; order is FTS-first, vector-second; both still run; existing behavior preserved |
queryFTS() works after closeLbug() + initLbug(dbPath) |
✅ satisfied — regression test at line 73 creates an index, closes, reopens, asserts arrayContaining([objectContaining({ filePath: 'src/index.ts' })]) — a result assertion, not just no-throw |
| Extension loading idempotent across repeated cycles | ✅ satisfied — loadFTSExtension() guards on ftsLoaded; closeLbug resets it; withLbugDb retry path resets it; "already loaded" catch branch handles duplicates |
| Regression test exercises real reopened core-adapter path | ✅ satisfied — test uses the real adapter.initLbug/closeLbug/queryFTS exports; no mocks on the DB path; goes through runWithSessionLock → ensureLbugInitialized → doInitLbug, which is the same code path as withLbugDb |
vitest.config.ts routes all lbug-backed suites through serialized lbug-db |
✅ satisfied — all three moved files (lbug-vector-extension.test.ts, java-class-impact.test.ts, class-impact-all-languages.test.ts) use withTestLbugDB; include/exclude lists are in sync; duplicate lbug-lock-retry.test.ts exclusion removed |
| Include/exclude lists consistent | ✅ satisfied — every entry added to lbug-db.include is also added to default.exclude; diff confirms symmetry |
| Validation sufficient for a DB/search lifecycle fix |
Validation assessment
Tests added / updated:
lbug-core-adapter.test.ts+14 lines: regression testinitLbug loads FTS so reopened HTTP-style sessions can query existing indexes— creates index, closes DB, reopens, asserts queryFTS returns expected results.
Validation claimed by the PR:
npx vitest run test/integration/lbug-core-adapter.test.ts --pool=forksnpx vitest run test/unit/bm25-search.test.ts --pool=threads- Selected lbug files with
--pool=forks --maxWorkers=1 npx vitest run --project lbug-db --maxWorkers=1- Prettier check on the three changed files
npx tsc --noEmitgit diff --checknpx tsx src/cli/index.ts detect-changes -r GitNexus
Validation verified from diff / CI evidence:
- CI check results are not available in this environment (no runs returned). Verifying against code only.
- The regression test is correctly structured: it creates an FTS index, calls
closeLbug()(which setsftsLoaded = false), reopens viainitLbug(handle.dbPath)(which triggersdoInitLbug→loadFTSExtension()), then queries. The assertionarrayContaining([objectContaining({ filePath: 'src/index.ts' })])is a meaningful result check against thecreateMinimalTestGraphfixture (which seedsFunctionnodes withname='main'andfilePath='src/index.ts'). - The test does not just probe no-throw; it proves an actual search result is returned.
bm25-search.test.tswould cover thesearchFTSFromLbugintegration path (FTS_INDEXES enumeration, result merging).- TypeScript is unchanged in the adapter signature —
tsc --noEmitshould pass. - The
closeLbug+initLbugtest call is isolated to a dedicated fork (lbug-db, fileParallelism: false), so no cross-file singleton corruption. - Windows fork-worker exit: The PR reports 49/51 tests exiting with native fork-worker failure when running without
--maxWorkers=1. This is consistent with the pre-existingdangerouslyIgnoreUnhandledErrors: truecomment invitest.config.tswhich explicitly documents "vitest forks + native addon interaction where destructors run in arbitrary order at exit." Serializing with--maxWorkers=1is the documented mitigation, not a correctness hack.
Missing or not-verifiable:
- No CI run results to confirm the serialized
lbug-dbproject passes reliably end-to-end. - The HTTP/Web search path (
api.ts→withLbugDb→ BM25 mode) is not directly exercised in a test;search-core.test.tswas listed inlbug-db.includebefore this PR and covers that path indirectly. - No direct exercise of
searchFTSFromLbug→queryFTSafter a DB reopen in the new test (it tests at thequeryFTSlevel which is the innermost call site; the layer above is consistently delegating).
Does missing validation block production readiness? No. The initLbug → doInitLbug → loadFTSExtension path is the same code path used by withLbugDb, and the regression test covers it at the right level of abstraction. The HTTP search path calling withLbugDb adds only session-lock and retry logic on top; it does not bypass or alter extension loading.
Final verdict
production-ready with minor follow-ups
The core fix is a single, correct line addition in doInitLbug that closes a clear gap: the FTS extension was never loaded during core adapter initialization, so HTTP-style sessions reopening a DB could not query existing BM25 indexes. The regression test proves the fix at the right level (real adapter, real DB file, asserting result not just no-throw), and the closeLbug + reopen cycle exactly mirrors the production failure condition. The Vitest config changes are correctly scoped, in sync, and justified by LadybugDB's native file-lock behavior — the three newly serialized tests (lbug-vector-extension, java-class-impact, class-impact-all-languages) all use withTestLbugDB and genuinely belong in the serialized project. The Windows fork-worker exit is consistent with the pre-existing documented N-API destructor behavior, not a new correctness bug. The only follow-up worth noting is cleaning up the now-redundant loadFTSExtension() call in the withTestLbugDB helper, which is harmless but misleading about ownership — that's a one-liner cleanup that does not block merge.
Made-with: Cursor
Bring in upstream fixes including: - fix(search): create FTS indexes during analyze (abhigyanpatwari#1107) — ROOT CAUSE of query() returning 0 results (FTS indexes were never created because lazy creation failed on read-only MCP pool connection) - fix(search): load FTS during core DB init (abhigyanpatwari#1123) - fix(search): surface warning when FTS indexes missing (abhigyanpatwari#1418) - fix(augment): add CONTAINS fallback when FTS unavailable (abhigyanpatwari#1476) - fix(search): guard against undefined bm25Results (abhigyanpatwari#1489) - feat(cpp): C++ ADL V2 overload resolution improvements - feat(detect-changes): support git worktrees (abhigyanpatwari#1654) - feat(cpp): parameter type class sidecar, SFINAE filter - Various CI, security, and infrastructure improvements AscendC provider updated to match upstream naming: sourcePreprocessor → preprocessSource Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
withTestLbugDBintegration suite through thelbug-dbVitest project so LadybugDB tests use the serialized project configuration.Closes #1071.
Test plan
npx vitest run test/integration/lbug-core-adapter.test.ts --pool=forksnpx vitest run test/unit/bm25-search.test.ts --pool=threadsnpx vitest run test/integration/lbug-core-adapter.test.ts test/integration/lbug-vector-extension.test.ts test/integration/lbug-pool.test.ts test/integration/lbug-pool-stability.test.ts test/integration/lbug-lock-retry.test.ts --pool=forks --maxWorkers=1npx vitest run --project lbug-db --maxWorkers=1npx prettier --check src/core/lbug/lbug-adapter.ts test/integration/lbug-core-adapter.test.ts vitest.config.tsnpx tsc --noEmitgit diff --checknpx tsx src/cli/index.ts detect-changes -r GitNexusNote: the same lbug category run without
--maxWorkers=1surfaced a Windows native fork-worker exit after 49/51 tests; all files passed individually and the serialized project run passed cleanly.