mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-05 02:43:32 +00:00
4 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
pcaceres-hazloagil
|
a8736a07d0
|
fix(lbug): checkpoint race in pool-adapter.ts + pin @ladybugdb/core to 0.18.3 (#3189)
* fix(lbug): await evict-then-reopen so it can't race the checkpoint
closeOne() closed the evicted repo's shared Database with a
fire-and-forget `db.close().catch(() => {})` (no await). Both call
sites that evict-then-reopen — evictLRU() right before doInitLbug
opens the new connection, and the "idle & changed" path in initLbug —
proceeded to open the next repo's connection immediately after,
without waiting for the evicted repo's close (and the checkpoint it
triggers) to finish. On the real engine the new open can then collide
with that still-in-flight checkpoint, surfacing on any read as:
Runtime exception: Cannot open database in read-only mode while
checkpoint is in progress. Please retry later.
This reproduces reliably once more than MAX_POOL_SIZE (5) distinct
repos are queried within a short window (self-hosted deployments with
more than a handful of active repos hit it routinely), and gets worse
under genuinely concurrent requests for different repos, since nothing
serialized pool mutations across callers either.
Fix:
- closeOne / evictLRU are now async and await their internal work
(closeOne's own close() call; evictLRU's call to closeOne), closing
the race within a single initLbug call.
- The exported initLbug is wrapped in a small async mutex
(initLbugInner does the real work) so concurrent initLbug calls for
different repos serialize instead of each racing their own
evict-then-reopen against the others.
- closeLbug's two closeOne() calls are now awaited too — closeOne
becoming async meant closeLbug could resolve before pool.delete()
had actually run, which a repo-pinning test caught (isLbugReady()
briefly still true right after a resolved closeLbug()).
- closeOne now deletes the pool entry (and clears its pin, and
notifies pool-close listeners) BEFORE the awaited db.close(), not
after. Review caught that the previous order left a "zombie" entry
reachable via pool.get(repoId) — closed=true, available emptied, but
still present — for the duration of that await; a same-repo
query/init landing in that window would see isLbugReady() as true
and hit a "Connection pool integrity error" in checkout() instead of
just reopening. Deleting first removes the entry entirely, so a
concurrent caller takes the normal fresh-open path instead.
Verified two ways:
- Against the compiled bundle (`ghcr.io/abhigyanpatwari/gitnexus`,
1.6.10/1.6.11 — pool-adapter.js is byte-identical between them): an
A/B docker build with 7 tiny local repos and genuinely concurrent
(parallel, not sequential) /api/graph requests goes from 7/7 failing
to 7/7 succeeding on a freshly-analyzed pool.
- Unit tests here (mocks @ladybugdb/core the same way as
lbug-pool-pinning.test.ts): one asserts the evicted repo's close()
completes before the initLbug call that triggered the eviction
settles; another asserts closeLbug's own promise doesn't resolve
before the underlying close() does. Both gate their mock's close()
on a real short delay and were confirmed to fail against code that
drops the corresponding await.
Note: a second, deeper issue was also observed in the docker A/B
setup — repeated rounds of concurrent access show a repo that has
gone through one evict+reopen cycle can become permanently unable to
reopen for reads, identically with and without this fix. That did not
reproduce with mocks and isn't understood yet; filed separately as
#3186, which stays open and untouched by this PR — this fix closes a
real, root-caused bug on its own but does not resolve #3186 by itself.
Second review round caught a follow-up: the idle-timeout sweep calls
closeOne(repoId) directly, outside of initLbug's poolLock. Now that
closeOne deletes the pool entry before its awaited close(), an
unsynchronized idle close racing a same-repo initLbug could let that
initLbug treat the repo as absent while the idle close (and its
checkpoint) is still in flight — reopening the same class of race this
PR exists to close, just via the idle path instead of LRU eviction.
Routed the idle sweep's closeOne call through withPoolLock too, so it
serializes against initLbug the same way evictLRU already does.
(Tried to add a mocked regression test for this specific interleaving;
dropped it — the mock's dbCache-reuse path masks the difference
regardless of the fix, so it could not be made to discriminate
reliably. Fixed by direct code review instead, same as the note below
already does for the native-engine-specific checkpoint collision.)
Also removed the initLbugInner per-repoId initPromises dedup map: with
every initLbug call now serialized through poolLock, a second call for
a repoId already being initialized cannot observe a pending promise in
initPromises (the first call always fully completes, including its
finally-block cleanup, before the lock releases) — the branch was dead
code the bot correctly flagged twice.
Third review round caught two more follow-ups on the same theme (both
introduced by making the idle sweep route through poolLock):
- closeLbug()'s no-arg ("close everything") branch still calls closeOne
directly in a loop over a snapshotted pool.keys(), without the lock —
an initLbug racing that loop could register a fresh entry the
snapshot never saw, leaving it resident after a call meant to empty
the pool. Wrapped the snapshot+loop in withPoolLock.
- The idle timer callback can now sit queued behind an in-progress
initLbug before its turn arrives, and that init (or a concurrent
touchRepo()) can refresh lastUsed in the meantime — so the pre-lock
idleness check taken when the timer fired can be stale by the time
it actually runs. Re-check lastUsed/checkedOut again inside the lock,
right before closing, instead of trusting the outer snapshot.
* fix(deps): pin @ladybugdb/core back to 0.18.3
Bisected the "checkpoint is in progress" symptom (root cause #2, not
touched by the pool-adapter.ts fix in the previous commit) down to a
single dependency-version-bump commit with zero application code
changes:
|
||
|
azizur100389
|
09322d2d89
|
fix(storage): load VECTOR only when needed (#3045)
* fix(storage): load VECTOR only when needed * test(storage): verify VECTOR reopen lifecycle --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
7f7255aef8
|
fix(analyze): load VECTOR before the incremental writeback touches embedding rows (#2623) (#2624)
* feat(lbug): add ensureEmbeddingRowDmlSafe VECTOR gate for embedding-row DML LadybugDB refuses every mutation of a table carrying an HNSW index while the VECTOR extension is not loaded on that connection: DELETE and CREATE raise a Binder exception, DROP TABLE is refused while the index references it, and SET segfaults the process. Dropping the index is not an available recovery either — CALL DROP_VECTOR_INDEX is itself a VECTOR-extension function and is undefined in exactly that state. Add a single primitive that loads VECTOR under the analyze install policy and, only when that fails, reads CALL SHOW_INDEXES (which works without the extension) to decide whether an index actually exists to trip over. No call sites yet. Refs #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): pin the #2623 VECTOR gate for embedding-row DML Three cases: no index + VECTOR unavailable stays safe (no needless escalation); index present + VECTOR unavailable is reported blocked AND the raw deleteNodesForFiles genuinely throws 'extension is not loaded' (proving the hazard is real, not theoretical); index present + VECTOR loadable is safe, the delete works, and the HNSW index survives — the invariant run-analyze relies on when it keeps the index across a surgical incremental run. Refs #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(analyze): load VECTOR before the incremental writeback touches embedding rows Incremental analyze died on every content change once a repo had built code_embedding_idx: Analysis failed: Binder exception: Trying to delete from an index on table CodeEmbedding but its extension is not loaded. The surgical writeback's first statement is deleteNodesForFiles' CodeEmbedding join-delete, but nothing on that path loaded VECTOR until Phase 4 — so the engine refused the delete. This is an ordering defect, not an environment one: it reproduces on machines where VECTOR loads fine. The dirty-flag recovery then forced a full rebuild on the next run, which is why it read as 'just slow'. Call ensureEmbeddingRowDmlSafe() once, before the escalation gate and before any row is touched — the same 'index lifecycle before row DML' seam dropSearchFTSIndexes occupies for FTS (#2589). Unconditional, because a DB carrying the index from an earlier --embeddings run hits the same wall on a plain incremental run. When VECTOR truly cannot load the table is immutable (the index cannot be dropped without the extension either), so the run falls through to the existing wipe-and-COPY escalation with a message naming cause, consequence and remedy. Fixes #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(analyze): pin the #2623 VECTOR-before-embedding-DML ordering end-to-end Sibling of the #2589 FTS drop-before-delete suite, same shape: drive the real runFullAnalysis incremental path over a real git repo and a real LadybugDB, seed real embedding rows, build the HNSW index, then assert the index state at the exact moment deleteNodesForFiles is invoked. Both cases were confirmed to discriminate — with the run-analyze change reverted they fail with the reported 'Trying to delete from an index on table CodeEmbedding but its extension is not loaded', and pass with it: - surgical path: the run completes, the index is still present AND extension_loaded at delete time, exactly one row per nodeId survives, and the untouched file's rows are preserved - blocked path: with GITNEXUS_LBUG_EXTENSION_INSTALL=never the run escalates to a full DB write and says so, instead of crashing Also applies prettier's reindent to the run-analyze log ternary. Refs #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(lbug): cite the pinned LadybugDB version in the #2623 probe note The probe matrix behind ensureEmbeddingRowDmlSafe was first recorded on 0.18.0, but gitnexus/package-lock.json pins 0.18.2 (#2587). Re-ran every case on 0.18.2: refused DELETE, refused CREATE, SIGSEGV on SET, DROP_VECTOR_INDEX undefined, DROP TABLE refused, SHOW_INDEXES readable with extension_loaded intact. Identical on both, so the design is unchanged — only the citation was wrong. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(analyze): preserve embeddings across the VECTOR-blocked rebuild, and check the catalog before loading Three follow-ups from reviewing the fix itself. 1. Data loss on the blocked path. Escalating wipes the DB files, and Phase 3.5 restores embedding rows from cachedEmbeddings — which deriveEmbeddingMode only populates when meta.stats.embeddings > 0. A DB holding embedding rows that its meta does not account for therefore had every vector destroyed silently by a rebuild it never asked for. Probe on a 3-file repo: 3 rows before, 0 after, no warning. Read the rows before escalating (a plain MATCH, no extension needed) so the existing restore has something to restore, and say so in the log. The blocked-path test now asserts the seeded rows survive exactly once, and that assertion fails without this rescue. 2. Catalog before extension. ensureEmbeddingRowDmlSafe loaded VECTOR first and only read SHOW_INDEXES on failure, so every incremental analyze on a machine without VECTOR paid a bounded out-of-process INSTALL attempt plus an 'extension unavailable' warning — including repos that never built an embedding index and can never hit this bug. One local catalog read settles that case first; the load is attempted only when an index actually gates DML, or when the catalog cannot be read. 3. Dead branch. targetConn is always the module singleton there, so the isSharedSingletonConn ternary could never take its second arm. Collapsed to withConnLock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(doctor): live-probe the VECTOR extension instead of printing the static platform capability Review finding on #2624 (MEDIUM), and exactly what #2623's reporter hit: doctor printed 'VECTOR index: available' — derived from a static platform check — while every incremental analyze on the same machine was dying on an unloaded VECTOR extension. The FTS line was switched to a live LOAD probe for the identical contradiction under #2374; VECTOR now gets the same treatment. probeVectorExtensionLoad shares the FTS probe's implementation (bounded, offline-safe, never runs the installer) and doctor's semantic-mode line now follows the probe, not the platform: without a loadable extension the vector index can be neither built nor queried, so search really is on exact scan. The load-error classifier's remedies are label-parameterized so the VECTOR row stops dispensing FTS-specific advice — 'run analyze --repair-fts' repairs FTS indexes only and was actively wrong for a missing vector extension. Default label stays 'FTS'; every existing caller and pinned remedy string is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lbug): remove the stale Windows VECTOR gate — the extension ships for win_amd64 The codebase categorically refused VECTOR on Windows (platform !== 'win32' in isVectorExtensionSupportedByPlatform, plus a hard early-return in loadVectorExtension) on the strength of an early-era report that in-process INSTALL VECTOR could SIGSEGV (#1365). That belief is stale, verified directly: - the extension server hosts win_amd64 VECTOR artifacts for every 0.18.x extension version — v0.18.0 and v0.18.1 both serve a real 14 MB PE32+ DLL (curl-probed; 'file' confirms PE32+ x86-64) - the pinned 0.18.2 core resolves its extension directory to 0.18.1 (strace-verified LOAD open()), so the pinned version's Windows artifact exists too - INSTALL now runs in a spawned child (installDuckDbExtensionOutOfProcess), so even a crashing installer kills only the child and degrades to unavailable — the original hazard cannot reach the parent process any more Windows now takes the same runtime path as every other OS: try LOAD, install out-of-process when policy allows, degrade to exact scan when it truly fails. The MCP semantic-search lane loses its static platform gate too — it always attempts the vector index and falls back to the exact scan on runtime failure, with a once-per-backend diagnostic naming the real error instead of a platform-policy message. isVectorExtensionSupportedByPlatform is deleted; getRuntimeCapabilities reports the platform capability as available everywhere and defers machine truth to the live probe. Windows CI is the enforcement: the vector suites skip visibly only when the extension genuinely cannot load, so green Windows lanes now actually exercise VECTOR instead of silently skipping by policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): pin the catalog-read-failure fallback in ensureEmbeddingRowDmlSafe Review finding on #2624 (LOW): the one branch where the gate cannot cheaply prove safety — SHOW_INDEXES itself erroring — was exercised only by inference. Force it with a Connection.prototype.query spy over the real DB: the catalog read fails, and the gate must fall through to actually attempting the extension load (asserted via the recorded statement stream) rather than guessing, returning true here because the extension is loadable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mcp): load VECTOR on the pool's shared Database so the semantic vector lane actually works Review finding on #2624 (MEDIUM): extension load scope is per-Database (probe-verified — LOAD on one connection enables QUERY_VECTOR_INDEX on every connection of the same Database), and the pool pre-warm loaded only FTS. So LocalBackend's vector lane has ALWAYS raised 'Catalog exception: function QUERY_VECTOR_INDEX is not defined' through the pool and silently fallen back to the exact scan — repos above the 10k exact-scan cap got empty semantic results. The serve path was unaffected (the embedding pipeline loads the extension itself). Mirror the FTS line at BOTH load sites — doInitLbug's pre-warm and initLbugWithDb's external-Database adoption — under the same load-only contract (the read pool never triggers a network install), tracked by a new SharedDB.vectorLoaded flag reset where ftsLoaded resets. The new pool test is discriminating and deliberately closes the writable core adapter before the pool opens: a shared/injected Database would inherit the VECTOR load from test seeding and pass either way, so the case forces the pool onto its OWN fresh read-only Database where only the pre-warm can make the lane legal. Verified: fails at the pre-fix tree with the exact Catalog exception, passes with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run the #2623 ordering suite on Windows/macOS and pre-install VECTOR alongside FTS Two review findings on #2624, both landing in existing seams: - scripts/cross-platform-tests.ts gains incremental-vector-extension-ordering .test.ts: the win32 VECTOR gate is gone in this PR, so the #2623 drop-ordering + blocked-path escalation must be proven on the windows-latest native addon, not just Ubuntu. (The review's claim that lbug-delete-nodes-for-files.test.ts was also missing was wrong — it has been on the roster since #2409.) - scripts/ensure-fts.ts now pre-installs VECTOR under the same best-effort auto-policy contract, so every sharded CI process LOADs from ~/.lbdb instead of racing its own bounded out-of-process INSTALL; the workflow's extension cache already covers it (path is the whole extension dir — key kept for cache continuity). The cross-platform job sets GITNEXUS_REQUIRE_VECTOR=1 beside GITNEXUS_REQUIRE_FTS so a genuinely unavailable VECTOR is a loud failure, never a silent skip. Windows/macOS cannot be executed locally; the PR's CI lanes are the proof for this commit. Linux smoke: ensure-fts.ts reports both extensions ready; all 79 roster entries resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(pool): register loadVectorExtension in the pool unit-suite mocks The pool adapter's new loadVectorExtension import surfaced in four suites that mock lbug-adapter.js with explicit factories (vitest fails loudly on a missing mocked export). Register the export in each — resolving false where the suite's world assumes no vector, true where it mirrors FTS — and extend lbug-pool-fts-load.test.ts, the suite that owns pre-warm extension loading, with the vector pair: successful load cached per shared Database, failed load retried on the next open, both pinned to policy load-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(analyze): use POSIX literals for graph paths in the #2623 ordering suite First Windows CI run of this suite (it joined the cross-platform roster this PR) failed with 'Parser exception: Invalid input <MATCH (n:Function) WHERE n.filePath = '>' — path.join produces backslashes on Windows, and a backslash inside the seed helper's single-quoted Cypher literal breaks the parser. The graph stores repo-relative filePaths with forward slashes on every OS, so graph-side paths are POSIX literals now (the incremental-orchestration convention); path.join stays only for real filesystem access. The same Windows lane also proved the substance this suite exists for: lbug-vector-extension passed 7/7 on windows-latest — the extension installed, loaded, and built a real HNSW index there — and the pool vector-lane and DML gate suites passed too. This commit fixes the harness, not the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <abhigyan1.patwari@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
azizur100389
|
93e04b46d6
|
fix(lbug): load FTS in Windows read pool (#2040) |