fix(lbug): prevent DuckDB extension install hangs#1129
Conversation
…#1128) `gitnexus analyze` could hang indefinitely (60% / 85% on Windows) when DuckDB's `INSTALL fts` or `INSTALL VECTOR` was unable to reach `extensions.duckdb.org`. The DuckDB driver's INSTALL is a synchronous network call, so any blocked egress would block the Node event loop forever. Replace the ad-hoc, in-process INSTALL/LOAD scattered across `lbug-adapter.ts` and `pool-adapter.ts` with a single `ExtensionManager` that owns the lifecycle of optional DuckDB extensions: * `LOAD` is always tried first — per-connection, idempotent, no network. * If `LOAD` fails and policy permits, INSTALL runs in a short-lived child Node process bounded by `GITNEXUS_LBUG_EXTENSION_INSTALL_TIMEOUT_MS` (default 15s). The parent loop keeps spinning; on timeout the child is killed with SIGKILL and the capability is flagged unavailable. * Capabilities and install attempts are cached per process, so a single bounded install per extension covers every subsequent call. Install policy is now an explicit, per-context decision: * `auto` (default for analyze) — try LOAD, fall back to bounded INSTALL. * `load-only` — used by `pool-adapter` (serve / MCP read paths) so user queries never block on a network install. * `never` — operator escape hatch for offline / airgapped environments. `createFTSIndex` and `createVectorIndex` now check the boolean return value before issuing the index DDL, so missing extensions degrade BM25 and semantic search gracefully without ever throwing during analyze. Tests: - New unit suite for `ExtensionManager` covering LOAD-first behavior, all three policies, install caching, observability, and warn dedup. - Existing vector-extension integration tests pass against the new boolean return type. - Existing embedding-pipeline mocks updated to return `true`. Docs: `gitnexus/README.md` documents `GITNEXUS_LBUG_EXTENSION_INSTALL` and `GITNEXUS_LBUG_EXTENSION_INSTALL_TIMEOUT_MS` with examples for offline and slow-network environments. Made-with: Cursor
Keep the bounded out-of-process INSTALL behavior, but replace the inline child code with a stable packaged ESM script. This makes the child process directly runnable and gives debuggable stack traces without source-vs-dist branching or a runtime transpiler. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @magyargergo's task in 5m 1s —— View job PR #1129 Adversarial Production Readiness ReviewTasks:
Review bar for this PR
Understanding of the changeWhat hang #1128 was: Why DuckDB What lifecycle boundary this PR introduces: The installer now runs in a short-lived child Which paths may install: Only Which paths must only load: Pool adapter ( What must remain unchanged for BM25/vector users: Pre-installed extensions still load normally and search works as before. Findings[high] No automated test proving pool adapter enforces
|
| DoD item | Status |
|---|---|
Analyze never hangs indefinitely when extension downloads are blocked — bounded child process with SIGKILL + timedOut resolution |
satisfied |
Serve/MCP/read-pool paths never run INSTALL — load-only policy enforced in both initLbug and initLbugWithDb in pool-adapter.ts |
satisfied (code correct; test coverage gap noted in Findings) |
ExtensionManager policy precedence correct: per-call → constructor → env → default |
satisfied |
auto, load-only, never policies behave distinctly and are tested |
partially satisfied — never policy test is present; load-only and auto are covered as manager-level unit tests; env var parsing for policy is untested |
loadFTSExtension() / loadVectorExtension() return accurate booleans; all callers handle false |
partially satisfied — callers handle false correctly in production code; the false path in createVectorIndex lacks a unit test |
| Out-of-process installer: no shell interpolation, validated names, bounded timeout, timer cleared, scratch DB cleaned | satisfied — parent uses process.execPath + argv array; validation in both parent and child; settled flag prevents double-resolve; finally block in child cleans tmpdir |
| Capability/install-attempt caching correct across repeated calls and reconnects | satisfied — installAttempted prevents re-install; module-level flags and shared.ftsLoaded handle reconnect; closeLbug resets flags correctly |
| Documentation accurately describes env controls and defaults | satisfied — README table matches implementation defaults (15000 ms, auto, correct policy descriptions, offline guidance) |
Validation assessment
Tests added/updated:
test/unit/lbug-extension-loader.test.ts(new, 252 lines): covers LOAD-first, already-loaded, auto/load-only/never policies, per-call override, timeout, caching, warning deduplication, name validation, child process args, env timeout parsing.test/unit/embedding-pipeline.test.ts(updated, +4/-4): mockloadVectorExtensionupdated tomockResolvedValue(true)to match newbooleanreturn; no new degradation path tests.test/integration/lbug-vector-extension.test.ts(updated, +4/-4): assertions updated toresolves.toBe(true)matching new return type; idempotency and close/reinit tested against real native module.
Validation claimed by PR:
npx tsc --noEmit— typecheck passes.- Unit and integration test runs for affected files.
node scripts/install-duckdb-extension.mjs "bad;name"— non-zero exit verifies name validation in child.npm run build— distribution build.npx tsx src/cli/index.ts detect-changes -r GitNexus— smoke test.- Full
npm testhas pre-existing unrelated Swift failures.
Verified from diff/code:
- SIGKILL on timeout: confirmed (
child.kill('SIGKILL')in timer callback). - Timer cleared on success/error: confirmed (
clearTimeout(timer)in both handlers). - No shell interpolation: confirmed (
process.execPath+ argv array). - Name validated in both parent (line 80
extension-loader.ts) and child (line 10install-duckdb-extension.mjs). scripts/is inpackage.jsonfilesarray — distribution path covered.- Path from compiled
dist/core/lbug/extension-loader.js→../../../scripts/install-duckdb-extension.mjsresolves to<package-root>/scripts/install-duckdb-extension.mjs. ✓ - Pool adapter both
doInitLbugandinitLbugWithDbuse{ policy: 'load-only' }. CYPHER_WRITE_REinpool-adapter.tsblocksINSTALLandLOADin user Cypher as defense-in-depth.
Missing or not-verifiable validation:
- No blocked-install / timeout scenario tested end-to-end (only mocked installer). The real
installDuckDbExtensionOutOfProcesstimeout/kill path is not exercised. - No test proving pool init cannot trigger a child process spawn under any condition.
- No
serve/mcpsmoke test showing no installer is spawned under query load. - Policy env var parsing behavior not verified in any test.
- Windows SIGKILL / cleanup behavior not verified (noted by PR as known noise, not blocking).
Does missing validation block production readiness? The high-severity test gap (pool load-only invariant) is not blocking in the sense that the code is clearly correct, but it is a meaningful regression risk for the core safety claim. The two medium gaps are genuine coverage holes but not showstoppers individually.
Final verdict
production-ready with minor follow-ups
The core mechanism is sound: the out-of-process installer is correctly bounded, the parent kill-on-timeout path has no timer leak, extension names are validated in both parent and child, the pool adapter enforces load-only in both init paths, and graceful degradation is wired through the whole stack. The documentation accurately reflects the implementation. The three test gaps (pool load-only invariant, policy env var parsing, vector skip-on-false path) do not make the current code incorrect — they make future regressions in the key safety properties undetectable by CI. The pool load-only invariant test is the most important follow-up given it covers the central claim of this PR; the other two are straightforward additions.
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 7541 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
Summary
gitnexus analyzefrom hanging when DuckDBfts/VECTORextension downloads are blocked by movingINSTALLinto a bounded child process.ExtensionManagerwith explicitauto,load-only, andneverpolicies so analyze can install while serve/MCP read paths only load preinstalled extensions.gitnexus/scripts/install-duckdb-extension.mjs, update vector/FTS callers to degrade gracefully, and document the new environment controls.Closes #1128.
Test plan
npx tsc --noEmitnpx vitest run test/unit/lbug-extension-loader.test.ts --pool=threadsnpx vitest run test/unit/embedding-pipeline.test.ts --pool=threadsnode scripts/install-duckdb-extension.mjs "bad;name"(expected non-zero, verifies debuggable script stack trace)npx vitest run --project=lbug-db test/integration/lbug-core-adapter.test.ts --pool=forks --maxWorkers=1npx vitest run --project=lbug-db test/integration/lbug-vector-extension.test.ts --pool=forks --maxWorkers=1(assertions pass; Windows may still emit the known native Vitest worker-exit noise after teardown)npm run buildnpx tsx src/cli/index.ts detect-changes -r GitNexusNotes
npm teststill shows unrelated pre-existing Swift failures inmethod-extraction,type-env, and Swift resolver coverage.