From c1a1b2a553e4e5ea62ef9e7be7f60a29d8bcef02 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 30 Jun 2026 07:46:38 +0100 Subject: [PATCH 1/5] chore(deps)(deps): bump onnxruntime-common in /gitnexus (#2320) Bumps [onnxruntime-common](https://github.com/Microsoft/onnxruntime) from 1.26.0 to 1.27.0. - [Release notes](https://github.com/Microsoft/onnxruntime/releases) - [Changelog](https://github.com/microsoft/onnxruntime/blob/main/docs/ReleaseManagement.md) - [Commits](https://github.com/Microsoft/onnxruntime/compare/v1.26.0...v1.27.0) --- updated-dependencies: - dependency-name: onnxruntime-common dependency-version: 1.27.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- gitnexus/package-lock.json | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index 9feeab39b..9dbd48476 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -4200,9 +4200,9 @@ } }, "node_modules/onnxruntime-common": { - "version": "1.26.0", - "resolved": "https://registry.npmjs.org/onnxruntime-common/-/onnxruntime-common-1.26.0.tgz", - "integrity": "sha512-qVyMR4lcWgbkc4getFV+GQijsTnbg/siteoqcDwa3sI/LxbrMSNw4ePyvCq/ymdQaRomCA7YuWmhzsswxvymdw==", + "version": "1.27.0", + "resolved": "https://registry.npmjs.org/onnxruntime-common/-/onnxruntime-common-1.27.0.tgz", + "integrity": "sha512-3KxL5wIVqa8Ex08jxSzncm9CMgw8CjOFyOQ7SxvG9o0cVLlhTNKXyIQuTbtX4tGPJEf73OER2xrjt4HJSBL4ow==", "license": "MIT" }, "node_modules/onnxruntime-node": { @@ -4222,6 +4222,12 @@ "onnxruntime-common": "1.26.0" } }, + "node_modules/onnxruntime-node/node_modules/onnxruntime-common": { + "version": "1.26.0", + "resolved": "https://registry.npmjs.org/onnxruntime-common/-/onnxruntime-common-1.26.0.tgz", + "integrity": "sha512-qVyMR4lcWgbkc4getFV+GQijsTnbg/siteoqcDwa3sI/LxbrMSNw4ePyvCq/ymdQaRomCA7YuWmhzsswxvymdw==", + "license": "MIT" + }, "node_modules/onnxruntime-web": { "version": "1.26.0-dev.20260416-b7804b056c", "resolved": "https://registry.npmjs.org/onnxruntime-web/-/onnxruntime-web-1.26.0-dev.20260416-b7804b056c.tgz", From 028bd110537e41d0b2c159af3234f872a17c40ea Mon Sep 17 00:00:00 2001 From: Sparsh <73558748+prajapatisparsh@users.noreply.github.com> Date: Tue, 30 Jun 2026 12:19:47 +0530 Subject: [PATCH 2/5] fix(group): cache read-only bridge handle to fix Windows @group reopen (#2274) (#2313) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(group): cache read-only bridge handle to fix Windows @group reopen (#2274) A long-lived MCP server opened bridge.lbug read-only, queried, and closed it on every @group trace/impact call. On Windows the in-process reopen of the same file fails (the OS handle is not fully released before the next open races in), so repeated @group calls broke. #2269 fixed Linux/macOS by skipping CHECKPOINT on read-only handles; Windows stayed broken. Instead of fighting LadybugDB's Windows close/reopen timing: cache one read-only handle per groupDir and reuse it across calls (open-once-per-process already works on Windows). getCachedBridgeReadOnly: - reuses a single handle keyed by resolved groupDir, - invalidates on mtime change (external writer / re-sync), - invalidates explicitly before same-process writes (writeBridge), - guards concurrent first-open with an in-flight promise (no handle leak), - closes all handles on process exit. closeBridgeDb now no-ops for the cached handle (cache owns its lifetime); uncached/writable handles are unaffected. ensureBridgeReady uses the cache. The in-process write->read reopen of the same bridge.lbug file remains a known LadybugDB Windows limitation, so the existing reopen tests stay win32-skipped. A new cache-aware itCacheReopen gate applies to the 3 new tests whose setup requires write-then-read in the same process (same class as itLbugReopen). The cache itself exercises read->read reuse and is unaffected. * fix(group): harden bridge RO-handle cache for concurrency, lifetime & Windows (#2313 review) Addresses the tri-review + Copilot findings on the read-only bridge-handle cache: - P1 (F2): serialize queryBridge per cached handle via a per-handle FIFO lock (the conn-lock.ts chain mechanic, keyed per cache entry, not the global lock). Two concurrent @group callers sharing one lbug.Connection can no longer dispatch two queries at once (the heap-corruption hazard). Uncached/writable handles skip the lock at zero cost. - P1 (F3): refcount lease — getCachedBridgeReadOnly acquires, closeBridgeDb releases (no caller change). The native close is deferred until in-flight readers drain (refs===0) and runs exactly once (closeStarted guard). invalidateBridgeCache and the mtime-evict path share one evict/close path. - Windows: bounded drain in evictBridgeEntry — a concurrent group_sync waits (<= WINDOWS_DRAIN_TIMEOUT_MS) for readers to release before the atomic rename on win32 so it stays clean; POSIX remains fully non-blocking; single-threaded sync still closes-before-rename on all platforms. - P0 (F1/F6): gate the mtime cache test with itCacheReopen (win32-skipped) and drop the manual invalidate so writeBridge self-invalidation is under test; add an external-writer (fsp.utimes) reopen case. - Windows coverage (F9): new cross-process integration test seeds bridge.lbug in a separate tsx process, so read->read handle reuse is proven on win32 CI (not skipped). Plus concurrent cold-open dedupe coverage. - P2/P3: scope the Windows NOTE to read->read (F4); JSDoc the closeBridgeDb release/close contract (F5); drop the if-branch in the B2 probe (F7); revert incidental Prettier churn in cross-impact.ts (F14); fix the stale describe header (F15); document the beforeExit/signal and ENOENT-mtime behavior (F11/F13). tsc clean; group unit + integration suites green. Co-Authored-By: Claude Opus 4.8 (1M context) * test(group): run the B2 rename-clash probe on win32 via cross-process seed (#2313 review) Moves the B2 "external rename while a cached RO handle is held" probe out of the unit suite (where it was win32-skipped, because its in-process writeBridge->RO-open is the unfixed Windows reopen) into the cross-process integration test, where a separate-process seed makes the RO open clean. The probe now RUNS ON WIN32 CI and empirically answers whether an open RO handle blocks an external atomic rename over bridge.lbug — the assumption under writeBridge's invalidate-before-rename and the win32 drain. Hardened (per adversarial review) so a win32 RED is the real steady-state share-mode signal, not an artifact: - use production retryRename (not bare fsp.rename) so transient EBUSY/EPERM from the Windows AV/indexer scanning the fresh temp file is absorbed; a RED then means the rename is blocked even after retries (FILE_SHARE_DELETE absent -> invalidate-before- rename is load-bearing). - stage the byte-identical replacement BEFORE opening the RO handle, so no second OS handle touches bridge.lbug while LadybugDB holds it (avoids a FILE_SHARE_READ red for the wrong question). - drop the post-rename query (handle survival is covered by the reuse test); the probe's sole verdict is whether the rename is blocked. Removes the old win32-skipped unit B2 (a strict subset of the new probe). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Gergő Magyar Co-authored-by: Claude Opus 4.8 (1M context) --- gitnexus/src/core/group/bridge-db.ts | 489 ++++++++++++++++-- gitnexus/src/core/group/cross-impact.ts | 12 +- .../group/bridge-cache-reopen.test.ts | 145 ++++++ .../integration/group/fixtures/seed-bridge.ts | 38 ++ gitnexus/test/unit/group/bridge-db.test.ts | 391 +++++++++++++- 5 files changed, 1004 insertions(+), 71 deletions(-) create mode 100644 gitnexus/test/integration/group/bridge-cache-reopen.test.ts create mode 100644 gitnexus/test/integration/group/fixtures/seed-bridge.ts diff --git a/gitnexus/src/core/group/bridge-db.ts b/gitnexus/src/core/group/bridge-db.ts index 8cb7782a8..0af5ca366 100644 --- a/gitnexus/src/core/group/bridge-db.ts +++ b/gitnexus/src/core/group/bridge-db.ts @@ -13,7 +13,9 @@ import { import { dedupeContracts, dedupeCrossLinks } from './normalization.js'; import { createLogger } from '../logger.js'; -const bridgeLogger = createLogger('bridge-db', { debugEnvVar: 'GITNEXUS_DEBUG_BRIDGE' }); +const bridgeLogger = createLogger('bridge-db', { + debugEnvVar: 'GITNEXUS_DEBUG_BRIDGE', +}); /** * Sidecar files that LadybugDB creates next to a `bridge.lbug` file. @@ -33,6 +35,358 @@ const bridgeLogger = createLogger('bridge-db', { debugEnvVar: 'GITNEXUS_DEBUG_BR */ const LBUG_SIDECAR_SUFFIXES = ['.wal', '.shadow'] as const; +/* ------------------------------------------------------------------ */ +/* Read-only bridge handle cache */ +/* ------------------------------------------------------------------ */ + +/** + * Cache of read-only bridge handles keyed by groupDir. Keeps one RO handle + * per groupDir alive across @group tool calls so a long-lived MCP server + * never reopens the same bridge.lbug in-process — reopening fails on Windows + * because the OS file handle isn't fully released before the next open races + * in (see PR #2269, #2274). + * + * deliberation: mtime-based invalidation was chosen over a simpler + * time-to-live or explicit-close model because: + * 1. TTL would force a reopen on a timer even when nothing changed. + * 2. Explicit-close requires every caller to know about the cache. + * 3. A cheap `fsp.stat` (uncached, but typically a single inode lookup on + * modern kernels) before each `ensureBridgeReady` call detects external + * writers (e.g. another process ran group sync) with zero false + * positives and no timer complication. + * 4. Same-process writes invalidate explicitly via `invalidateBridgeCache` + * before the atomic rename so the cached RO handle does not block it. + */ +interface CachedBridgeEntry { + handle: BridgeHandle; + mtime: number; + /** + * Active leases: callers between `getCachedBridgeReadOnly` (acquire, `refs++`) + * and `closeBridgeDb` (release, `refs--`). The native handle is never closed + * while `refs > 0` — a concurrent `@group` reader may still be querying it, + * and closing under a live query is a native use-after-free. + */ + refs: number; + /** Set once the entry leaves the cache; the native close is deferred to the last release. */ + evicted: boolean; + /** Guards `finalizeBridgeClose` so the native close runs exactly once. */ + closeStarted: boolean; + /** + * Per-handle FIFO serialization tail. The cached RO handle is shared across + * concurrent `@group` callers, but a LadybugDB `Connection` is NOT safe for + * concurrent query execution (see `lbug/conn-lock.ts` — two queries on one + * connection corrupt the native heap). `queryBridge` runs each op on this + * chain so no two ever overlap on one handle. Per-handle (not a single global + * lock) so different groups — separate connections — stay parallel. + */ + lockTail: Promise; + /** + * Resolves when the native handle has actually been closed. `writeBridge` on + * Windows awaits this (bounded — see `WINDOWS_DRAIN_TIMEOUT_MS`) before its + * atomic rename, because Windows cannot rename over an open handle. On POSIX + * the rename succeeds over an open RO handle (the old inode survives for the + * in-flight reader), so the close stays fully non-blocking there. + */ + drained: Promise; + /** Resolver for {@link CachedBridgeEntry.drained}; called once by `finalizeBridgeClose`. */ + resolveDrained: () => void; +} + +/** + * Windows-only bound on how long `invalidateBridgeCache` waits for in-flight + * readers to release before letting `writeBridge` rename. Past this, it falls + * through and `retryRename` (EBUSY ×3) copes — so a pathologically long reader + * can never wedge `group_sync`. ponytail: fixed 5s ceiling; make it + * configurable if a real workload shows reads routinely outlasting it. + */ +const WINDOWS_DRAIN_TIMEOUT_MS = 5000; + +const cachedBridgeHandles = new Map(); + +/** + * Reverse lookup: cache entry by its `BridgeHandle`. Lets `queryBridge` and + * `closeBridgeDb` find an entry from just the handle — including an *evicted* + * entry that is no longer in `cachedBridgeHandles` but whose native handle a + * lease still holds open. Uncached/writable handles (the `writeBridge` temp DB) + * are absent here, which is how those paths opt out of the lock and refcount. + */ +const bridgeEntryByHandle = new WeakMap(); + +/** + * In-flight opens keyed by groupDir. Prevents the TOCTOU race where two + * concurrent cache-miss calls both open a fresh handle and the second + * overwrites the first in `cachedBridgeHandles` — leaking the first + * handle. Mirrors the `local-backend.ts:1293` reinitPromises pattern. + */ +const inFlightOpens = new Map>(); + +function bridgeCacheKey(groupDir: string): string { + return path.resolve(groupDir); +} + +/** + * Serialize an operation on a cached handle's per-handle FIFO chain. Mirrors the + * promise-chain mechanic of `lbug/conn-lock.ts` (install a fresh unresolved + * tail, await the prior holder, release in `finally` so a throw never wedges the + * chain) — but keyed per handle, not a single global lock. No re-entry guard: + * `queryBridge` is a leaf (it never calls another locked bridge helper), and the + * native close runs outside the lock gated on `refs === 0`. + */ +export async function withHandleLock( + lock: { lockTail: Promise }, + fn: () => Promise, +): Promise { + const prior = lock.lockTail; + let release!: () => void; + lock.lockTail = new Promise((resolve) => { + release = resolve; + }); + await prior; + try { + return await fn(); + } finally { + release(); + } +} + +/** + * Close a cached entry's native handle exactly once. Guarded by `closeStarted` + * so the mtime-evict path, `invalidateBridgeCache`, the last lease release, and + * `closeAllCachedBridges` can all reach here and only one native close runs. + */ +async function finalizeBridgeClose(entry: CachedBridgeEntry): Promise { + if (entry.closeStarted) return; + entry.closeStarted = true; + bridgeEntryByHandle.delete(entry.handle); + try { + await closeBridgeHandle(entry.handle); + } finally { + entry.resolveDrained(); + } +} + +/** + * Remove an entry from the cache and release its native handle. The native + * close is DEFERRED until in-flight leases drain (`refs === 0`): closing a + * handle a concurrent `@group` reader is still querying is a native + * use-after-free (the `conn-lock.ts` hazard). When `refs === 0` (the common + * single-threaded case — e.g. `group_sync` with no concurrent read) the close + * runs now and the returned promise resolves when it completes, so + * `writeBridge`'s atomic rename never races a live RO handle on Windows. + * + * When `refs > 0` (a concurrent reader holds a lease), the native close is + * deferred to the last `closeBridgeDb` release — closing now would be a + * use-after-free. Platform split for the rename that follows: + * - POSIX: return immediately. The rename succeeds over the still-open RO + * handle (old inode survives for the reader); no wait, no starvation. + * - Windows: a rename over an open handle fails (EBUSY), so wait — bounded by + * `WINDOWS_DRAIN_TIMEOUT_MS` — for the reader to release and the deferred + * close to complete, then the rename is clean. On timeout, fall through and + * let `retryRename` cope, so a slow reader can never wedge `group_sync`. + * + * This is the single eviction path for BOTH the mtime-change branch and + * `invalidateBridgeCache`. + */ +async function evictBridgeEntry(key: string, entry: CachedBridgeEntry): Promise { + if (!entry.evicted) { + entry.evicted = true; + if (cachedBridgeHandles.get(key) === entry) cachedBridgeHandles.delete(key); + } + if (entry.refs <= 0) { + await finalizeBridgeClose(entry); + return; + } + // refs > 0: close deferred to the last closeBridgeDb release. + if (process.platform === 'win32') { + // Windows needs the handle closed before writeBridge renames. Wait (bounded) + // for readers to drain; on timeout, retryRename handles the residual EBUSY. + let timer: ReturnType; + const timeout = new Promise((resolve) => { + timer = setTimeout(resolve, WINDOWS_DRAIN_TIMEOUT_MS); + }); + await Promise.race([entry.drained, timeout]).finally(() => clearTimeout(timer)); + } +} + +/** + * Close a BridgeHandle's native resources without touching the cache. + * Shared by `closeBridgeDb` (uncached handles) and the cache invalidation + * / shutdown paths so neither duplicates the close logic. + */ +async function closeBridgeHandle(handle: BridgeHandle): Promise { + if (!handle._readOnly) { + try { + await (handle._conn as lbug.Connection).query('CHECKPOINT'); + } catch { + /* ignore — older LadybugDB or schemaless DB may not accept it */ + } + } + try { + await (handle._conn as lbug.Connection).close(); + } catch { + /* ignore */ + } + try { + await (handle._db as lbug.Database).close(); + } catch { + /* ignore */ + } +} + +/** + * Get or create a cached read-only bridge handle for `groupDir`. + * + * - First call: delegates to `openBridgeDbReadOnly`, records the file's + * `mtimeMs`, and caches the handle. + * - Subsequent calls (mtime unchanged): returns the cached handle — no + * reopen, no OS file-handle churn. + * - After the file's mtime changes (external writer, e.g. another process + * ran `gitnexus group sync`): closes the stale handle, opens a fresh + * one, and updates the cache. + * - After the file disappears (ENOENT): invalidates cache, returns null. + * + * Returns `null` when the bridge file is missing, has an incompatible + * schema version, or cannot be opened even after the retry loop in + * `openBridgeDbReadOnly`. + */ +export async function getCachedBridgeReadOnly(groupDir: string): Promise { + const key = bridgeCacheKey(groupDir); + const dbPath = path.join(groupDir, 'bridge.lbug'); + + // Fast path: cache hit, unchanged mtime → lease the cached handle. + const entry = cachedBridgeHandles.get(key); + if (entry) { + try { + const stat = await fsp.stat(dbPath); + // Re-check `evicted` AFTER the await: a concurrent writeBridge/invalidate + // may have evicted this entry while we awaited `stat`. Leasing an evicted + // (closing) handle would be a use-after-close. The `refs++` is the first + // synchronous statement after the check, so no evictor can slip between. + if (!entry.evicted && stat.mtimeMs === entry.mtime) { + entry.refs++; + return entry.handle; + } + } catch { + // File disappeared (ENOENT) — fall through to evict + reopen. + } + // mtime changed or file gone — evict (defers the native close if a + // concurrent reader still holds a lease; closes now otherwise). + if (!entry.evicted) await evictBridgeEntry(key, entry); + } + + // TOCTOU guard: if another caller is already opening for this key, await + // their in-flight promise and take a lease on the result instead of opening + // a second handle. + const inFlight = inFlightOpens.get(key); + if (inFlight) { + const handle = await inFlight; + if (!handle) return null; + // Same post-await guard as the fast path: the opener's entry may have been + // evicted between caching and this awaiter resuming. Only lease a live, + // identity-matched entry; otherwise retry from the top for a fresh handle. + const opened = cachedBridgeHandles.get(key); + if (opened && !opened.evicted && opened.handle === handle) { + opened.refs++; + return handle; + } + return getCachedBridgeReadOnly(groupDir); + } + + const openPromise: Promise = (async () => { + try { + const handle = await openBridgeDbReadOnly(groupDir); + if (!handle) return null; + + let mtime = 0; + try { + const stat = await fsp.stat(dbPath); + mtime = stat.mtimeMs; + } catch { + // bridge.lbug not stat-able right after open (rare race). Leaving + // mtime at 0 means the next call's fast-path comparison won't match + // (a real file's mtime is never 0), so it re-opens. Benign: the handle + // still works for this caller; we just don't cache-reuse it until a + // later open records a real mtime. + } + + let resolveDrained!: () => void; + const drained = new Promise((resolve) => { + resolveDrained = resolve; + }); + const newEntry: CachedBridgeEntry = { + handle, + mtime, + refs: 0, + evicted: false, + closeStarted: false, + lockTail: Promise.resolve(), + drained, + resolveDrained, + }; + cachedBridgeHandles.set(key, newEntry); + bridgeEntryByHandle.set(handle, newEntry); + return handle; + } finally { + inFlightOpens.delete(key); + } + })(); + inFlightOpens.set(key, openPromise); + + // Each caller (the opener and every awaiter) takes exactly one lease here, so + // refs counts callers correctly even under inFlightOpens coalescing. + const handle = await openPromise; + if (!handle) return null; + const opened = cachedBridgeHandles.get(key); + if (opened && !opened.evicted && opened.handle === handle) { + opened.refs++; + return handle; + } + return getCachedBridgeReadOnly(groupDir); +} + +/** + * Invalidate the cached read-only handle for `groupDir`. Drops it from the + * cache immediately; the native close is deferred until any in-flight reader + * leases drain (see {@link evictBridgeEntry}). With no concurrent reader this + * resolves only after the handle is actually closed — which is why + * `writeBridge` awaits it before its atomic rename (Windows: a still-open RO + * handle would block the rename with EBUSY). + */ +export async function invalidateBridgeCache(groupDir: string): Promise { + const key = bridgeCacheKey(groupDir); + const entry = cachedBridgeHandles.get(key); + if (entry) await evictBridgeEntry(key, entry); +} + +/** + * Close ALL cached bridge handles. Call on process shutdown only — it force- + * closes regardless of refs (safe at `beforeExit`, which fires only at + * event-loop quiescence, so no query is in flight). Do NOT wire this to a + * SIGTERM/SIGINT handler that can fire mid-request: that would close a handle + * under a live query. Routes through `finalizeBridgeClose` for the close-once + * guarantee. + */ +export async function closeAllCachedBridges(): Promise { + const entries = [...cachedBridgeHandles.values()]; + cachedBridgeHandles.clear(); + await Promise.all(entries.map((e) => finalizeBridgeClose(e))); +} + +// Best-effort process-exit cleanup. 'beforeExit' fires before 'exit' and +// lets async work drain (unlike 'exit' which is synchronous-only). It does +// NOT fire on process.exit()/SIGTERM/SIGINT — but that is fine here: the OS +// reclaims all handles on any exit path, and for read-only handles there is +// no WAL to flush, so the only thing lost on signal death is a tidy close +// (cosmetic). We deliberately do NOT register a SIGTERM/SIGINT handler: a +// signal can fire mid-request, and closeAllCachedBridges force-closes +// regardless of refs, which would close a handle under a live query. Shutdown +// sequencing is the MCP server's responsibility — it should call +// closeAllCachedBridges() at a quiescent point (also how tests get a +// deterministic teardown). +process.once('beforeExit', () => { + void closeAllCachedBridges(); +}); + async function removeLbugFile(basePath: string): Promise { const candidates = [basePath, ...LBUG_SIDECAR_SUFFIXES.map((s) => `${basePath}${s}`)]; for (const f of candidates) { @@ -195,20 +549,29 @@ export async function queryBridge( cypher: string, params?: Record, ): Promise { - const conn = handle._conn as lbug.Connection; - if (params && Object.keys(params).length > 0) { - const stmt = await conn.prepare(cypher); - if (!stmt.isSuccess()) { - const errMsg = await stmt.getErrorMessage(); - throw new Error(`Bridge query prepare failed: ${errMsg}`); + const run = async (): Promise => { + const conn = handle._conn as lbug.Connection; + if (params && Object.keys(params).length > 0) { + const stmt = await conn.prepare(cypher); + if (!stmt.isSuccess()) { + const errMsg = await stmt.getErrorMessage(); + throw new Error(`Bridge query prepare failed: ${errMsg}`); + } + const queryResult = await conn.execute(stmt, params); + const result = unwrapQueryResult(queryResult); + return (await result.getAll()) as T[]; } - const queryResult = await conn.execute(stmt, params); + const queryResult = await conn.query(cypher); const result = unwrapQueryResult(queryResult); return (await result.getAll()) as T[]; - } - const queryResult = await conn.query(cypher); - const result = unwrapQueryResult(queryResult); - return (await result.getAll()) as T[]; + }; + // Cached RO handles are shared across concurrent @group callers, so serialize + // conn ops per handle (a LadybugDB Connection is not safe for concurrent + // queries — conn-lock.ts). Uncached/writable handles (the writeBridge temp DB) + // are single-threaded — they're absent from bridgeEntryByHandle and skip the + // lock at zero cost. + const entry = bridgeEntryByHandle.get(handle); + return entry ? withHandleLock(entry, run) : run(); } /** @@ -230,48 +593,54 @@ function unwrapQueryResult(queryResult: lbug.QueryResult | lbug.QueryResult[]): return queryResult; } +/** + * Release a caller's reference to a bridge handle. + * + * - **Cache-owned handle** (returned by `getCachedBridgeReadOnly`): this is the + * matching *release* for that acquire — it decrements the lease refcount, it + * does NOT close the native handle. The cache owns the lifetime; the handle + * closes on explicit `invalidateBridgeCache`, mtime-eviction, or process + * shutdown. If the entry was already evicted and this is the last lease, the + * deferred native close fires here (exactly once). + * - **Uncached/writable handle** (e.g. the `writeBridge` temp DB): closes the + * native handle for real (CHECKPOINT-flush for writable handles). + * + * Contract: before renaming or deleting `bridge.lbug`, call + * `invalidateBridgeCache` (not this) — `closeBridgeDb` on a cache-owned handle + * is a lease release, so the file may stay open under other readers. + */ export async function closeBridgeDb(handle: BridgeHandle): Promise { - // CHECKPOINT before close so the WAL/.shadow contents are flushed into - // the main database file. Without this, LadybugDB 0.16.0's non-blocking - // checkpoint thread can outlive the close call and leave sidecar pages - // pending on disk, which makes a subsequent read-side open either race - // with the WAL replay or trip the database-id check on the sidecars. - // CHECKPOINT is a no-op when there's nothing pending, so it's cheap. - // - // ONLY on a writable handle. A read-only connection has nothing to flush, - // and issuing CHECKPOINT on it leaves a WAL/shadow lock artifact that makes - // the very next read-only open of the same path fail in-process — which broke - // repeated `@group` impact/trace calls in a long-lived MCP server (the read - // path opens read-only, queries, and closes per call). - if (!handle._readOnly) { - try { - await (handle._conn as lbug.Connection).query('CHECKPOINT'); - } catch { - /* ignore — older LadybugDB or schemaless DB may not accept it */ - } + const entry = bridgeEntryByHandle.get(handle); + if (!entry) { + // Uncached or writable handle — close for real. + await closeBridgeHandle(handle); + return; } - try { - await (handle._conn as lbug.Connection).close(); - } catch { - /* ignore */ - } - try { - await (handle._db as lbug.Database).close(); - } catch { - /* ignore */ - } - // NOTE: Windows in-process write→read reopen of the SAME bridge.lbug is still a - // known limitation (the writable close's OS file handle is not released before - // the read open races; the existing open-side LBUG_OPEN_RETRY only retries - // lock-pattern errors, not the post-rename sidecar database-id mismatch). The - // bridge's close-then-reopen tests stay Windows-skipped. A close-side - // waitForWindowsHandleRelease + finalizeLbugSidecarsAfterClose probe (mirroring - // safeClose) was tried and did NOT close that gap on Windows CI, so it was - // removed rather than carry latency/duplication for no Windows benefit. The - // read-only CHECKPOINT skip above is the load-bearing fix and works on - // Linux/macOS (the platforms where in-process reopen is supported). + // Cache-owned handle: release this lease. Close only the evicted handle whose + // last lease just dropped (deferred-close completion); the live cached handle + // stays open for reuse. + if (entry.refs > 0) entry.refs--; + if (entry.evicted && entry.refs <= 0) await finalizeBridgeClose(entry); } +// NOTE: Windows in-process write→read reopen of the SAME bridge.lbug is still a +// known limitation (the writable close's OS file handle is not released before +// the read open races; the existing open-side LBUG_OPEN_RETRY only retries +// lock-pattern errors, not the post-rename sidecar database-id mismatch). The +// bridge's close-then-reopen tests stay Windows-skipped. A close-side +// waitForWindowsHandleRelease + finalizeLbugSidecarsAfterClose probe (mirroring +// safeClose) was tried and did NOT close that gap on Windows CI, so it was +// removed rather than carry latency/duplication for no Windows benefit. +// +// Scope of the RO bridge-handle cache (getCachedBridgeReadOnly): it removes the +// PRODUCTION symptom — a long-lived MCP serve process reopening bridge.lbug on +// every @group call — by keeping one RO handle alive for read→READ reuse. +// It does NOT fix the write→READ reopen: the first @group read right after an +// in-process group_sync is a cache miss → openBridgeDbReadOnly, i.e. the same +// unfixed reopen, so on Windows that first post-sync read still returns null. +// The read-only CHECKPOINT skip above remains the load-bearing fix on +// Linux/macOS. + /* ------------------------------------------------------------------ */ /* retryRename — handles transient EBUSY/EPERM/EACCES on Windows */ /* ------------------------------------------------------------------ */ @@ -379,6 +748,13 @@ export async function writeBridge( input: WriteBridgeInput, ): Promise { await fsp.mkdir(groupDir, { recursive: true }); + + // Invalidate the RO cache before writing. On Windows the cached handle + // would block the atomic rename (tmp → bridge.lbug) because the OS keeps + // a shared-mode lock on the open file. Closing it first guarantees the + // rename succeeds without EBUSY. + await invalidateBridgeCache(groupDir); + const contracts = dedupeContracts(input.contracts); const crossLinks = dedupeCrossLinks(input.crossLinks); @@ -731,7 +1107,12 @@ export async function openBridgeDbReadOnly(groupDir: string): Promise { + let groupDir: string; + + beforeEach(async () => { + groupDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'bridge-xproc-')); + // Seed bridge.lbug in a SEPARATE process. Its writable handle is released by + // process death, so this process's first read-only open is NOT an in-process + // write→read reopen — the property that lets the reuse assertions run on win32. + const res = spawnSync(process.execPath, ['--import', tsxImportUrl, seedScript, groupDir], { + stdio: 'pipe', + timeout: 60_000, + }); + expect(res.status, `seed process failed: ${res.stderr?.toString() ?? ''}`).toBe(0); + }); + + afterEach(async () => { + await closeAllCachedBridges(); + await cleanupTempDir(groupDir); + }); + + it('reuses one cached handle across repeated calls without reopening', async () => { + // First open: a clean cross-process open (seed already exited) — succeeds on + // win32. This is the cold-cache open the cache does NOT need to fix. + const first = await getCachedBridgeReadOnly(groupDir); + expect(first).not.toBeNull(); + + // Repeated calls reuse the SAME handle — no reopen. THIS is the Windows fix: + // pre-cache, each of these reopened bridge.lbug and failed on Windows. + const second = await getCachedBridgeReadOnly(groupDir); + const third = await getCachedBridgeReadOnly(groupDir); + expect(second).toBe(first); + expect(third).toBe(first); + + // The reused handle still answers queries. + const rows = await queryBridge<{ repo: string }>( + first!, + 'MATCH (c:Contract) RETURN c.repo AS repo', + ); + expect(rows).toMatchObject([{ repo: 'backend' }]); + + await closeBridgeDb(first!); + await closeBridgeDb(second!); + await closeBridgeDb(third!); + }); + + it('concurrent cold-cache opens dedupe to one handle (no double-open) on the target platform', async () => { + // N concurrent first-callers must coalesce to a single open via inFlightOpens + // and all receive the same handle — verified here on the cross-process seed so + // it exercises a real win32 open, not the skipped in-process reopen. + const N = 6; + const handles = await Promise.all( + Array.from({ length: N }, () => getCachedBridgeReadOnly(groupDir)), + ); + expect(handles.every((h) => h !== null)).toBe(true); + const first = handles[0]!; + expect(handles).toMatchObject(Array.from({ length: N }, () => first)); + + for (const h of handles) await closeBridgeDb(h!); + }); + + // B2 probe — moved here from the unit suite so it RUNS ON WIN32. bridge.lbug is + // seeded cross-process (beforeEach), so opening RO is a clean cross-process + // open, not the in-process write→read reopen that forced the old probe to be + // win32-skipped. This settles, on Windows CI, the assumption under + // writeBridge's invalidate-before-rename and the win32 drain: does an open + // cached RO handle block an external atomic rename over bridge.lbug? + // LadybugDB opens RO with FILE_SHARE_DELETE, which should permit the rename; + // a failure on Windows CI means it does not, and invalidate-before-rename is + // load-bearing rather than belt-and-suspenders. + it('external rename over bridge.lbug succeeds while a cached RO handle is held', async () => { + const dbPath = path.join(groupDir, 'bridge.lbug'); + const tmpPath = path.join(groupDir, 'bridge.lbug.tmp'); + + // Stage the byte-identical replacement BEFORE opening the RO handle, so we + // never hold a SECOND OS handle on bridge.lbug while LadybugDB has it open + // (reading it concurrently would probe FILE_SHARE_READ — a different + // question — and could red for the wrong reason). + await fsp.copyFile(dbPath, tmpPath); + + // Hold the cached RO handle open (a long-lived MCP serve process's state). + const handle = await getCachedBridgeReadOnly(groupDir); + expect(handle).not.toBeNull(); + + // Rename the staged copy over bridge.lbug WHILE the RO handle is held — + // exactly what a concurrent `gitnexus group sync` does. Use production's + // retryRename policy (writeBridge uses it), so transient EBUSY/EPERM from + // the Windows AV/indexer scanning the fresh temp file is absorbed. A RED is + // then the real steady-state answer: an open RO handle blocks the atomic + // rename on Windows (FILE_SHARE_DELETE not set) → writeBridge's + // invalidate-before-rename and the win32 drain are load-bearing. (The + // handle-survives-rename property is covered by the reuse test above; this + // probe's sole verdict is whether the rename itself is blocked.) + let renameError: string | null = null; + try { + await retryRename(tmpPath, dbPath); + } catch (err: unknown) { + renameError = err instanceof Error ? err.message : String(err); + } + expect( + renameError, + `[B2] external rename blocked while cached RO handle held: ${renameError}`, + ).toBeNull(); + + await closeBridgeDb(handle!); + }); +}); diff --git a/gitnexus/test/integration/group/fixtures/seed-bridge.ts b/gitnexus/test/integration/group/fixtures/seed-bridge.ts new file mode 100644 index 000000000..fe50814dc --- /dev/null +++ b/gitnexus/test/integration/group/fixtures/seed-bridge.ts @@ -0,0 +1,38 @@ +/** + * Cross-process bridge seeder for `bridge-cache-reopen.test.ts`. + * + * Writes a valid `bridge.lbug` into `argv[2]` and exits. Running this as a + * SEPARATE process is the whole point: the writable handle is fully released by + * process death before the parent test opens read-only, so the test's first RO + * open is a clean cross-process open — NOT the in-process write→read reopen that + * still fails on Windows. That is what lets the cache's read→read REUSE + * assertion actually run on win32 instead of being skipped. + * + * Invoked as: node --import seed-bridge.ts + */ +import { writeBridge } from '../../../../src/core/group/bridge-db.js'; +import { makeContract } from '../../../unit/group/fixtures.js'; + +async function main(): Promise { + const groupDir = process.argv[2]; + if (!groupDir) { + process.stderr.write('usage: seed-bridge.ts \n'); + process.exit(2); + } + await writeBridge(groupDir, { + contracts: [makeContract()], + crossLinks: [], + repoSnapshots: {}, + missingRepos: [], + }); +} + +main().then( + () => process.exit(0), + (err: unknown) => { + process.stderr.write( + `seed-bridge failed: ${err instanceof Error ? (err.stack ?? err.message) : String(err)}\n`, + ); + process.exit(1); + }, +); diff --git a/gitnexus/test/unit/group/bridge-db.test.ts b/gitnexus/test/unit/group/bridge-db.test.ts index ac3fdc3be..d2a4c9a8a 100644 --- a/gitnexus/test/unit/group/bridge-db.test.ts +++ b/gitnexus/test/unit/group/bridge-db.test.ts @@ -18,7 +18,7 @@ import { indexContract, findContractNode, } from '../../../src/core/group/bridge-db.js'; -import type { CrossLink } from '../../../src/core/group/types.js'; +import type { BridgeHandle, CrossLink } from '../../../src/core/group/types.js'; import { makeContract } from './fixtures.js'; /** @@ -30,13 +30,12 @@ import { makeContract } from './fixtures.js'; * `closeBridgeDb` fix that skips CHECKPOINT on read-only handles (a CHECKPOINT * on a read-only connection left a lock artifact that failed the next open). * - * On WINDOWS the writable-close → read-open handoff still does not release the - * OS file handle before the read open races (the existing open-side - * `LBUG_OPEN_RETRY_*` only retries lock-pattern errors, not the post-rename - * sidecar database-id mismatch), so these tests stay Windows-skipped — the - * pre-existing limitation. A close-side `waitForWindowsHandleRelease` + - * `finalizeLbugSidecarsAfterClose` probe was tried and did not close the gap on - * Windows CI, so it was not kept. + * On WINDOWS the direct openBridgeDbReadOnly reopen still fails (see NOTE in + * closeBridgeDb). The read-only bridge-handle cache (getCachedBridgeReadOnly) + * solves this for production by keeping one handle alive across calls instead + * of reopening — see the `bridge handle cache` describe block. These tests + * exercise the DIRECT reopen path (bypassed by the cache) and stay skipped on + * Windows. */ const itLbugReopen = process.platform === 'win32' ? it.skip : it; @@ -145,7 +144,9 @@ describe('writeBridge + read', () => { await writeBridge(tmpDir, { contracts: [makeContract()], crossLinks: [], - repoSnapshots: { backend: { indexedAt: '2026-01-01', lastCommit: 'abc' } }, + repoSnapshots: { + backend: { indexedAt: '2026-01-01', lastCommit: 'abc' }, + }, missingRepos: ['missing-repo'], }); const exists = await bridgeExists(tmpDir); @@ -156,7 +157,9 @@ describe('writeBridge + read', () => { const report = await writeBridge(tmpDir, { contracts: [makeContract(), makeContract({ repo: 'frontend', role: 'consumer' })], crossLinks: [], - repoSnapshots: { backend: { indexedAt: '2026-01-01', lastCommit: 'abc' } }, + repoSnapshots: { + backend: { indexedAt: '2026-01-01', lastCommit: 'abc' }, + }, missingRepos: [], }); expect(report.contractsInserted).toBe(2); @@ -268,7 +271,9 @@ describe('writeBridge + read', () => { await writeBridge(tmpDir, { contracts: [], crossLinks: [], - repoSnapshots: { 'hr/backend': { indexedAt: '2026-01-01', lastCommit: 'abc' } }, + repoSnapshots: { + 'hr/backend': { indexedAt: '2026-01-01', lastCommit: 'abc' }, + }, missingRepos: [], }); const handle = await openBridgeDbReadOnly(tmpDir); @@ -313,7 +318,11 @@ describe('writeBridge + read', () => { missingRepos: [], }); const handle = await openBridgeDbReadOnly(tmpDir); - const rows = await queryBridge<{ fromRepo: string; toRepo: string; matchType: string }>( + const rows = await queryBridge<{ + fromRepo: string; + toRepo: string; + matchType: string; + }>( handle!, 'MATCH (a:Contract)-[l:ContractLink]->(b:Contract) RETURN l.fromRepo AS fromRepo, l.toRepo AS toRepo, l.matchType AS matchType', ); @@ -377,7 +386,11 @@ describe('writeBridge + read', () => { }); const handle = await openBridgeDbReadOnly(tmpDir); - const contracts = await queryBridge<{ repo: string; symbolUid: string; symbolName: string }>( + const contracts = await queryBridge<{ + repo: string; + symbolUid: string; + symbolName: string; + }>( handle!, 'MATCH (c:Contract) RETURN c.repo AS repo, c.symbolUid AS symbolUid, c.symbolName AS symbolName ORDER BY c.repo', ); @@ -436,6 +449,332 @@ describe('writeBridge + read', () => { }); }); +/* ------------------------------------------------------------------ */ +/* getCachedBridgeReadOnly cache tests */ +/* ------------------------------------------------------------------ */ + +/** + * The RO bridge-handle cache avoids reopening bridge.lbug per @group + * tool call, which fails on Windows (the OS handle isn't fully released + * before the next open races in). These tests verify read→read reuse and + * mtime/size-based invalidation on macOS/Linux. Each begins with the + * beforeEach `writeBridge` (writable) followed by a read-only open, i.e. the + * in-process write→read reopen that is the unfixed LadybugDB Windows + * limitation — so every test here is `itCacheReopen` (win32-skipped). The + * cache layer (one handle held alive) is what these exercise, not the native + * close-then-reopen the itLbugReopen tests cover. + */ +describe('bridge handle cache', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'bridge-cache-test-')); + // Create a valid bridge.lbug to open + await writeBridge(tmpDir, { + contracts: [makeContract()], + crossLinks: [], + repoSnapshots: {}, + missingRepos: [], + }); + }); + + afterEach(async () => { + // Close any cached handles so cleanupTempDir doesn't hit EBUSY + const { closeAllCachedBridges } = await import('../../../src/core/group/bridge-db.js'); + await closeAllCachedBridges(); + await cleanupTempDir(tmpDir); + }); + + // The beforeEach calls writeBridge (writable) then the test body opens + // read-only via getCachedBridgeReadOnly. On Windows this in-process + // write→read reopen is the known LadybugDB limitation (same class as + // itLbugReopen) — the OS handle isn't fully released after the writer + // closes. The cache exercises read→read reuse, not write→read, so the + // skip only affects the test setup, not the cache logic. + const itCacheReopen = process.platform === 'win32' ? it.skip : it; + + itCacheReopen('same groupDir returns the same handle instance', async () => { + const { getCachedBridgeReadOnly } = await import('../../../src/core/group/bridge-db.js'); + const first = await getCachedBridgeReadOnly(tmpDir); + expect(first).not.toBeNull(); + + const second = await getCachedBridgeReadOnly(tmpDir); + expect(second).not.toBeNull(); + // Must be the SAME object — not a new open + expect(second).toBe(first); + }); + + itCacheReopen('writeBridge self-invalidates the cache (no manual invalidate)', async () => { + const { getCachedBridgeReadOnly, queryBridge, closeBridgeDb } = + await import('../../../src/core/group/bridge-db.js'); + const first = await getCachedBridgeReadOnly(tmpDir); + expect(first).not.toBeNull(); + + // Rewrite the bridge WITHOUT calling invalidateBridgeCache here — writeBridge + // must self-invalidate (impl: invalidateBridgeCache before its atomic rename). + // A manual invalidate would mask that, leaving the load-bearing invariant + // untested. + await writeBridge(tmpDir, { + contracts: [makeContract({ repo: 'updated-repo' })], + crossLinks: [], + repoSnapshots: {}, + missingRepos: [], + }); + + const second = await getCachedBridgeReadOnly(tmpDir); + expect(second).not.toBeNull(); + // Different handle — writeBridge's own invalidate dropped the old entry. + expect(second).not.toBe(first); + + // New handle sees the updated data. + const rows = await queryBridge<{ repo: string }>( + second!, + 'MATCH (c:Contract) RETURN c.repo AS repo', + ); + expect(rows).toMatchObject([{ repo: 'updated-repo' }]); + + // Release leases as real consumers do (finally{closeBridgeDb}). `first` was + // evicted by writeBridge (close deferred behind this lease) — releasing it + // fires the deferred native close. + await closeBridgeDb(first!); + await closeBridgeDb(second!); + }); + + itCacheReopen('cache reopens when an external writer bumps mtime', async () => { + // Exercises the stat-based mtime/size invalidation branch directly, WITHOUT + // going through writeBridge's own invalidate. bridge.lbug is a native binary, + // so we bump mtime with fsp.utimes (rewriting bytes would corrupt it and the + // reopen would return null). Simulates another process having written the + // bridge out-of-band. + const { getCachedBridgeReadOnly, queryBridge, closeBridgeDb } = + await import('../../../src/core/group/bridge-db.js'); + const dbPath = path.join(tmpDir, 'bridge.lbug'); + const first = await getCachedBridgeReadOnly(tmpDir); + expect(first).not.toBeNull(); + + const future = new Date(Date.now() + 5000); + await fsp.utimes(dbPath, future, future); + + const second = await getCachedBridgeReadOnly(tmpDir); + expect(second).not.toBeNull(); + // mtime moved → the fast path missed → a fresh handle was opened. + expect(second).not.toBe(first); + + const rows = await queryBridge<{ repo: string }>( + second!, + 'MATCH (c:Contract) RETURN c.repo AS repo', + ); + expect(rows).toMatchObject([{ repo: 'backend' }]); + + await closeBridgeDb(first!); + await closeBridgeDb(second!); + }); + + itCacheReopen('concurrent calls return the same handle instance', async () => { + const { getCachedBridgeReadOnly } = await import('../../../src/core/group/bridge-db.js'); + + // Fire N concurrent cache-miss calls — the TOCTOU guard should make + // only ONE actual openBridgeDbReadOnly call; all the rest await it. + const N = 10; + const results = await Promise.all( + Array.from({ length: N }, () => getCachedBridgeReadOnly(tmpDir)), + ); + + // All returned the same handle instance (proves no double-open) + const first = results[0]!; + for (const h of results) { + expect(h).toBe(first); + } + + // Verify the handle works — query returns expected data + const { queryBridge } = await import('../../../src/core/group/bridge-db.js'); + const rows = await queryBridge<{ repo: string }>( + first, + 'MATCH (c:Contract) RETURN c.repo AS repo', + ); + expect(rows).toHaveLength(1); + expect(rows[0].repo).toBe('backend'); + }); +}); + +/* ------------------------------------------------------------------ */ +/* Concurrency: per-handle query serialization + refcount lease */ +/* ------------------------------------------------------------------ */ + +/** + * The cached RO handle is shared across concurrent @group callers in a + * long-lived MCP serve process. Two correctness properties must hold: + * 1. No two queries run on one connection at once (LadybugDB Connection is + * not concurrency-safe — conn-lock.ts). queryBridge serializes per handle. + * 2. The native handle is never closed while a reader holds a lease, and is + * closed exactly once on the last release (refcount). + */ +describe('bridge handle lock (withHandleLock)', () => { + it('serializes — never two operations overlap on one lock', async () => { + const { withHandleLock } = await import('../../../src/core/group/bridge-db.js'); + const lock = { lockTail: Promise.resolve() }; + let active = 0; + const observedMax: number[] = []; + const section = async () => { + active++; + observedMax.push(active); + await new Promise((r) => setTimeout(r, 1)); + active--; + }; + await Promise.all(Array.from({ length: 8 }, () => withHandleLock(lock, section))); + // If two sections ever overlapped, active would reach 2. + expect(Math.max(...observedMax)).toBe(1); + }); + + it('releases the lock when an operation throws (chain not wedged)', async () => { + const { withHandleLock } = await import('../../../src/core/group/bridge-db.js'); + const lock = { lockTail: Promise.resolve() }; + await expect( + withHandleLock(lock, async () => { + throw new Error('boom'); + }), + ).rejects.toThrow('boom'); + // A subsequent op still runs — the failed op released its tail. + const result = await withHandleLock(lock, async () => 'ok'); + expect(result).toBe('ok'); + }); +}); + +describe('bridge handle cache — refcount lease', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'bridge-refcount-test-')); + await writeBridge(tmpDir, { + contracts: [makeContract()], + crossLinks: [], + repoSnapshots: {}, + missingRepos: [], + }); + }); + + afterEach(async () => { + const { closeAllCachedBridges } = await import('../../../src/core/group/bridge-db.js'); + await closeAllCachedBridges(); + await cleanupTempDir(tmpDir); + }); + + // Each test below opens RO right after the beforeEach writeBridge (write→read + // reopen) — the unfixed Windows limitation — so all are win32-skipped. + const itCacheReopen = process.platform === 'win32' ? it.skip : it; + + // Spy on the native close of a handle without `any` (strict-typing rule): + // _conn is typed `unknown`, so cast to the minimal structural shape we use. + const spyConnClose = (handle: BridgeHandle) => + vi.spyOn(handle._conn as { close: () => Promise }, 'close'); + + itCacheReopen('invalidate defers the native close until the last lease releases', async () => { + const { getCachedBridgeReadOnly, invalidateBridgeCache, closeBridgeDb } = + await import('../../../src/core/group/bridge-db.js'); + // Two leases on the same cached handle (refs === 2). + const a = await getCachedBridgeReadOnly(tmpDir); + const b = await getCachedBridgeReadOnly(tmpDir); + expect(a).not.toBeNull(); + expect(b).toBe(a); + + const closeSpy = spyConnClose(a!); + + // group_sync-style invalidate while readers hold leases → close deferred. + await invalidateBridgeCache(tmpDir); + expect(closeSpy).not.toHaveBeenCalled(); + + // First release: refs 2 → 1, still not closed. + await closeBridgeDb(a!); + expect(closeSpy).not.toHaveBeenCalled(); + + // Last release: refs 1 → 0, native close fires exactly once. + await closeBridgeDb(b!); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + itCacheReopen('refs count every awaiter under inFlightOpens (not just one)', async () => { + const { getCachedBridgeReadOnly, invalidateBridgeCache, closeBridgeDb } = + await import('../../../src/core/group/bridge-db.js'); + // N concurrent cache-miss calls coalesce to one open but each takes a lease. + const N = 5; + const handles = await Promise.all( + Array.from({ length: N }, () => getCachedBridgeReadOnly(tmpDir)), + ); + const first = handles[0]!; + expect(handles).toMatchObject(Array.from({ length: N }, () => first)); + + const closeSpy = spyConnClose(first); + await invalidateBridgeCache(tmpDir); + + // Release N-1 leases — if refs had been miscounted as 1, the close would + // have fired on the first release. It must not. + for (let i = 0; i < N - 1; i++) await closeBridgeDb(first); + expect(closeSpy).not.toHaveBeenCalled(); + + // The Nth release drops refs to 0 → close once. + await closeBridgeDb(first); + expect(closeSpy).toHaveBeenCalledTimes(1); + }); + + itCacheReopen('mtime-evict also defers close while a lease is held', async () => { + const { getCachedBridgeReadOnly, closeBridgeDb } = + await import('../../../src/core/group/bridge-db.js'); + const dbPath = path.join(tmpDir, 'bridge.lbug'); + const stale = await getCachedBridgeReadOnly(tmpDir); + expect(stale).not.toBeNull(); + const closeSpy = spyConnClose(stale!); + + // External writer bumps mtime; the next get evicts the stale entry. The + // lease on `stale` is still held, so its close must defer (this is the + // OTHER live close-under-lease path, alongside invalidate). + const future = new Date(Date.now() + 5000); + await fsp.utimes(dbPath, future, future); + const fresh = await getCachedBridgeReadOnly(tmpDir); + expect(fresh).not.toBe(stale); + expect(closeSpy).not.toHaveBeenCalled(); + + // Releasing the stale lease fires its deferred close exactly once. + await closeBridgeDb(stale!); + expect(closeSpy).toHaveBeenCalledTimes(1); + + await closeBridgeDb(fresh!); + }); + + // Exercises the win32-only bounded-drain branch by mocking process.platform on + // a non-Windows runner (the real win32 path is proven by the cross-process + // integration test; this proves the branch LOGIC — that invalidate blocks + // until the reader releases on Windows rather than racing the rename). + // Skipped on real win32 (its in-process setup is the unsupported reopen). + itCacheReopen('on win32, invalidate waits for the in-flight reader to drain', async () => { + const { getCachedBridgeReadOnly, invalidateBridgeCache, closeBridgeDb } = + await import('../../../src/core/group/bridge-db.js'); + const handle = await getCachedBridgeReadOnly(tmpDir); + expect(handle).not.toBeNull(); + + const realPlatform = process.platform; + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + try { + let invalidateResolved = false; + const invalidate = invalidateBridgeCache(tmpDir).then(() => { + invalidateResolved = true; + }); + + // With a lease held, the win32 drain must NOT resolve yet (POSIX would + // return immediately here — that's the platform difference under test). + await new Promise((r) => setTimeout(r, 20)); + expect(invalidateResolved).toBe(false); + + // Releasing the lease drains refs→0, closes the handle, and unblocks the + // waiting invalidate well within the bounded timeout. + await closeBridgeDb(handle!); + await invalidate; + expect(invalidateResolved).toBe(true); + } finally { + Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }); + } + }); +}); + describe('retryRename', () => { afterEach(() => { vi.restoreAllMocks(); @@ -476,7 +815,9 @@ describe('retryRename', () => { throw err; }); - await expect(retryRename('/src/a', '/dst/b', 5)).rejects.toMatchObject({ code: 'ENOENT' }); + await expect(retryRename('/src/a', '/dst/b', 5)).rejects.toMatchObject({ + code: 'ENOENT', + }); expect(calls).toBe(1); }); @@ -489,7 +830,9 @@ describe('retryRename', () => { throw err; }); - await expect(retryRename('/src/a', '/dst/b', 3)).rejects.toMatchObject({ code: 'EPERM' }); + await expect(retryRename('/src/a', '/dst/b', 3)).rejects.toMatchObject({ + code: 'EPERM', + }); expect(calls).toBe(3); }); @@ -523,7 +866,11 @@ describe('findContractNode', () => { it('tier 1: returns contract matched by symbolUid', () => { const index = createContractLookupIndex(); - const c = makeContract({ symbolUid: 'uid-42', repo: 'backend', role: 'provider' }); + const c = makeContract({ + symbolUid: 'uid-42', + repo: 'backend', + role: 'provider', + }); indexContract(index, c, 'node-A'); expect(findContractNode(index, 'backend', 'provider', 'uid-42', 'anywhere.ts', 'anyName')).toBe( 'node-A', @@ -541,7 +888,11 @@ describe('findContractNode', () => { it('tier 1 is role-scoped: provider uid match does not resolve consumer query', () => { const index = createContractLookupIndex(); - const c = makeContract({ symbolUid: 'uid-42', role: 'provider', repo: 'backend' }); + const c = makeContract({ + symbolUid: 'uid-42', + role: 'provider', + repo: 'backend', + }); indexContract(index, c, 'node-A'); expect( findContractNode(index, 'backend', 'consumer', 'uid-42', 'src/routes.ts', 'getUsers'), @@ -625,3 +976,9 @@ describe('findContractNode', () => { ); }); }); + +// The B2 cross-process rename-clash probe moved to +// test/integration/group/bridge-cache-reopen.test.ts, where a cross-process +// seed lets it run on win32 (the in-process write→read reopen no longer gates +// it). It empirically answers whether an open cached RO handle blocks an +// external atomic rename of bridge.lbug on Windows. From 15583fc9e9f45c9c85d841060c8530e7196cb829 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 30 Jun 2026 08:13:12 +0100 Subject: [PATCH 3/5] chore(deps)(deps): bump onnxruntime-node in /gitnexus (#2321) Bumps [onnxruntime-node](https://github.com/Microsoft/onnxruntime) from 1.26.0 to 1.27.0. - [Release notes](https://github.com/Microsoft/onnxruntime/releases) - [Changelog](https://github.com/microsoft/onnxruntime/blob/main/docs/ReleaseManagement.md) - [Commits](https://github.com/Microsoft/onnxruntime/compare/v1.26.0...v1.27.0) --- updated-dependencies: - dependency-name: onnxruntime-node dependency-version: 1.27.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abhigyan Patwari <126312502+abhigyanpatwari@users.noreply.github.com> --- gitnexus/package-lock.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index 9dbd48476..a3c8e3335 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -4206,9 +4206,9 @@ "license": "MIT" }, "node_modules/onnxruntime-node": { - "version": "1.26.0", - "resolved": "https://registry.npmjs.org/onnxruntime-node/-/onnxruntime-node-1.26.0.tgz", - "integrity": "sha512-OHl6PiOEOqxaLHL0N9eFrbzS7IGmu3BtJNH3RTEnRAheCIkfc3gjcjl4sGcjp9C22ZC9YTquDOxSdT/stBQ6BQ==", + "version": "1.27.0", + "resolved": "https://registry.npmjs.org/onnxruntime-node/-/onnxruntime-node-1.27.0.tgz", + "integrity": "sha512-QEzGwrvNBgv4uPVdnbHsOGG4G6T96mdlcFI8aAKPjMU8wOPpVocPXb6k3QGkaZagVTv2G9Bnnbo6Z3JdXr1fQw==", "hasInstallScript": true, "license": "MIT", "os": [ @@ -4219,7 +4219,7 @@ "dependencies": { "adm-zip": "^0.5.16", "global-agent": "^4.1.3", - "onnxruntime-common": "1.26.0" + "onnxruntime-common": "1.27.0" } }, "node_modules/onnxruntime-node/node_modules/onnxruntime-common": { From 9c5a1743037420a33801f9c7c048ba6898f9a850 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 30 Jun 2026 08:26:48 +0100 Subject: [PATCH 4/5] chore(deps)(deps): bump commander from 14.0.3 to 15.0.0 in /gitnexus (#2322) Bumps [commander](https://github.com/tj/commander.js) from 14.0.3 to 15.0.0. - [Release notes](https://github.com/tj/commander.js/releases) - [Changelog](https://github.com/tj/commander.js/blob/master/CHANGELOG.md) - [Commits](https://github.com/tj/commander.js/compare/v14.0.3...v15.0.0) --- updated-dependencies: - dependency-name: commander dependency-version: 15.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abhigyan Patwari <126312502+abhigyanpatwari@users.noreply.github.com> --- gitnexus/package-lock.json | 10 +++++----- gitnexus/package.json | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index a3c8e3335..f7511fb5e 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -16,7 +16,7 @@ "@scarf/scarf": "^1.4.0", "busboy": "^1.6.0", "cli-progress": "^3.12.0", - "commander": "^14.0.3", + "commander": "^15.0.0", "cors": "^2.8.5", "express": "^5.2.1", "express-rate-limit": "^8.4.1", @@ -2527,12 +2527,12 @@ } }, "node_modules/commander": { - "version": "14.0.3", - "resolved": "https://registry.npmjs.org/commander/-/commander-14.0.3.tgz", - "integrity": "sha512-H+y0Jo/T1RZ9qPP4Eh1pkcQcLRglraJaSLoyOtHxu6AapkjWVCy2Sit1QQ4x3Dng8qDlSsZEet7g5Pq06MvTgw==", + "version": "15.0.0", + "resolved": "https://registry.npmjs.org/commander/-/commander-15.0.0.tgz", + "integrity": "sha512-z67u4ZhzCL/Tydu1lJARtEZYWbWaN7oYLHbsuzocr6y4N6WZAagG3RQ4FW61V1/0+jImpj293XfrcYnd1qxtPg==", "license": "MIT", "engines": { - "node": ">=20" + "node": ">=22.12.0" } }, "node_modules/content-disposition": { diff --git a/gitnexus/package.json b/gitnexus/package.json index f4adcb9da..11b5fcb5e 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -61,7 +61,7 @@ "@scarf/scarf": "^1.4.0", "busboy": "^1.6.0", "cli-progress": "^3.12.0", - "commander": "^14.0.3", + "commander": "^15.0.0", "cors": "^2.8.5", "express": "^5.2.1", "express-rate-limit": "^8.4.1", From e148bc089a9323aead9d9029b7dadbbc9b0a0e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Tue, 30 Jun 2026 15:08:43 +0100 Subject: [PATCH 5/5] fix(group): replace LadybugDB-incompatible multi-label Cypher (#2325) (#2327) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(group): use labels(n) IN allowlist instead of LadybugDB-incompatible multi-label Cypher (#2325) manifest-extractor and http-route-extractor built Cypher with the openCypher label disjunction `MATCH (n:A|B|C)`, which LadybugDB's parser rejects. The error was swallowed by try/catch, so manifest contracts silently fell back to synthetic UIDs with empty filePath and http-route cross-file handler resolution silently returned null. Replace all 7 queries with `MATCH (n) WHERE labels(n) IN [...]`. LadybugDB returns labels(n) as a single string, so this is an exact allowlist — a 1:1 behavior-preserving syntax translation (validated against LadybugDB 0.17.1). Export the two http-route query constants so integration tests can run the exact production strings against a real DB, and add per-branch real-DB regression coverage (the bug shipped because no test exercised these queries). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(group): import CypherExecutor from contract-extractor in #2325 test The new manifest regression test imported `CypherExecutor` from `group/types.js`, which does not export it — the type is defined only in `group/contract-extractor.js` (as all production extractors import it). This was a real TS2305 under `tsc -p tsconfig.test.json`, masked from CI because the default tsconfig excludes `test/` and `import type` is erased at runtime. Split the import so the type resolves from its real module. Co-Authored-By: Claude Opus 4.8 (1M context) * test(group): run #2325 native-LadybugDB tests in the lbug-db project Per TESTING.md, every test that opens a real `@ladybugdb/core` handle must be registered in the sequential `lbug-db` Vitest project (and excluded from `default`) to avoid native-mmap file-lock conflicts across parallel forks on Windows. The two new group integration tests use `withTestLbugDB`/pool-adapter but were in neither list, so they ran under the parallel `default` project. Add both to `lbug-db.include` and `default.exclude`, matching every sibling. Co-Authored-By: Claude Opus 4.8 (1M context) * refactor(group): export custom-contract resolve query for #2325 test The #2325 integration test hand-copied the 21-label `custom`-branch resolve query into a local `LABELS_CUSTOM_QUERY` constant, so editing the production allowlist would silently desync the canary. Promote the query to an exported `CUSTOM_CONTRACT_RESOLVE_QUERY` (mirroring http-route-extractor's exported query strings) and import it in the test, so the canary always runs the exact production query. Behavior unchanged — same query string. Co-Authored-By: Claude Opus 4.8 (1M context) * test(group): de-brittle the #2325 custom-query label assertion The unit test asserted a fixed 7-label ordered substring of the 21-label custom-branch allowlist, coupling it to label order and no-space formatting — a harmless reorder would have broken it. Replace with order/spacing-tolerant membership checks for a spread of individual labels, keeping the unconditional `not.toContain('Function|Method')` guard as the real regression check. Co-Authored-By: Claude Opus 4.8 (1M context) * test(group): correct #2325 http-route docstring + add real-trigger canary The http-route test claimed `MATCH (n:Function|Method|CodeElement)` "which LadybugDB rejects" — but that 3-label disjunction actually PARSES. Verified against the real parser, the genuine #2325 trigger is a *reserved-keyword* label in the disjunction: `Macro` and `Union` both are, and only the manifest custom branch (21-label list) and the lib branch (missing `Package` table) actually threw. The http-route conversion to `labels(n) IN [...]` was a consistency change, not a parser fix. Correct the misleading docstring and add a rejection canary pinned to the real cause (`MATCH (n:Function|Macro|Union)` rejects), so a future query that reintroduces a reserved-keyword disjunction is caught. Co-Authored-By: Claude Opus 4.8 (1M context) * test(group): cover the thrift package-strip path against a real LadybugDB The thrift-only branch of resolveSymbol strips a `package.` prefix from the service name (`com.example.AuthService` -> `AuthService`) before the Class/Interface lookup — previously exercised only with a mocked executor. Add a service-contract integration case (no method, so it takes the package-strip path, not the grpc-identical method path) that resolves the real `cls:AuthService`. Without the strip the lookup matches nothing and falls back to a synthetic uid, so this is a non-vacuous guard for the strip. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(group): drop vestigial 'Package' label from lib contract lookup The `lib` branch allowlisted `labels(n) IN ['Package','Module']`, but there is no `Package` node table (see NODE_TABLES) — the entry only ever matched nothing. Restrict to `['Module']`, the label libraries actually resolve to. Behavior-neutral: the lib integration case still resolves its Module symbol. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(group): update PIPELINE label-scoped queries to labels(n) IN form The resolveSymbol label-scoping bullets still showed the banned `MATCH (n:A|B)` disjunction; a contributor copying them would reintroduce #2325. Rewrite them in the actual `labels(n) IN [...]` form, note the real trigger (LadybugDB rejects a disjunction naming a reserved keyword such as `Macro`/`Union`), and reflect the lib allowlist as `['Module']` after dropping the vestigial `Package` label. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(group): correct #2325 root-cause comments in the extractors The production comments claimed LadybugDB rejects the `MATCH (n:A|B)` disjunction "outright". Verified against the real parser, it rejects only when a label is a reserved keyword (`Macro`, `Union`) or names a missing node table. So only the manifest `custom` branch (reserved keywords in its 21-label list) and the `lib` branch (missing `Package` table) actually threw; the http-route/grpc/thrift/topic disjunctions parse fine and were converted to `labels(n) IN [...]` for consistency and future-proofing, not because they were broken. Rewrite the comments to say so accurately. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) * test(group): make #2325 test prose name the real reserved-keyword trigger The manifest test docstring/title and the unit-test comment said LadybugDB rejects the `MATCH (n:A|B)` disjunction generally. It rejects only when a label is a reserved keyword (`Macro`/`Union`) or a missing table. Reword the docstring (custom + lib branches threw; others parsed), retitle the rejection canary to "its list names reserved keywords Macro/Union", and correct the unit-test comment. The rejection canary still passes — the custom 21-label list does contain Macro/Union. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- gitnexus/src/core/group/PIPELINE.md | 13 +- .../group/extractors/http-route-extractor.ts | 31 +++- .../group/extractors/manifest-extractor.ts | 51 ++++-- .../group/http-route-resolve-symbol.test.ts | 111 +++++++++++ .../manifest-resolve-symbol-2325.test.ts | 173 ++++++++++++++++++ .../unit/group/manifest-extractor.test.ts | 12 +- gitnexus/vitest.config.ts | 4 + 7 files changed, 364 insertions(+), 31 deletions(-) create mode 100644 gitnexus/test/integration/group/http-route-resolve-symbol.test.ts create mode 100644 gitnexus/test/integration/group/manifest-resolve-symbol-2325.test.ts diff --git a/gitnexus/src/core/group/PIPELINE.md b/gitnexus/src/core/group/PIPELINE.md index 8bdf86c48..9a8c4b972 100644 --- a/gitnexus/src/core/group/PIPELINE.md +++ b/gitnexus/src/core/group/PIPELINE.md @@ -112,11 +112,14 @@ flowchart TD EMIT --> BRIDGE[(bridge.lbug
#795)] ``` -Label-scoped queries in `resolveSymbol` keep accidental cross-matches -out: -- `topic` → `(n:Function|Method|Class|Interface)` -- `grpc` method → `(n:Function|Method)`, service → `(n:Class|Interface)` -- `lib` → `(n:Package|Module)` +Label-scoped queries in `resolveSymbol` keep accidental cross-matches out. +They use the `MATCH (n) WHERE labels(n) IN [...]` allowlist form, NOT the +`MATCH (n:A|B)` disjunction — LadybugDB's parser rejects a disjunction that +names a reserved keyword (e.g. `Macro`, `Union`), which is what broke the +`custom` branch in #2325: +- `topic` → `labels(n) IN ['Function','Method','Class','Interface']` +- `grpc`/`thrift` method → `labels(n) IN ['Function','Method']`, service → `labels(n) IN ['Class','Interface']` +- `lib` → `labels(n) IN ['Module']` ## Cross-impact query (PR #606) diff --git a/gitnexus/src/core/group/extractors/http-route-extractor.ts b/gitnexus/src/core/group/extractors/http-route-extractor.ts index 79c005e92..f28f3fac4 100644 --- a/gitnexus/src/core/group/extractors/http-route-extractor.ts +++ b/gitnexus/src/core/group/extractors/http-route-extractor.ts @@ -80,10 +80,21 @@ WHERE sym.filePath = $filePath AND sym.startLine IS NOT NULL AND sym.endLine IS RETURN sym.id AS uid, sym.name AS name, sym.filePath AS filePath, sym.startLine AS startLine, sym.endLine AS endLine, labels(sym) AS labels`; -// Repo-wide lookup of a symbol by exact name (label-union, as in -// manifest-extractor.ts). Used to resolve a provider's named handler when it is -// defined in a file OTHER than its route registration — and only honored when -// the result is unique (see resolveSymbolByNameUnique). +// Repo-wide lookup of a symbol by exact name. Used to resolve a provider's +// named handler when it is defined in a file OTHER than its route registration — +// and only honored when the result is unique (see resolveSymbolByNameUnique). +// +// Label filtering uses `labels(n) IN [...]` rather than the openCypher +// disjunction `MATCH (n:A|B|C)`. NOTE: this 3-label set (Function/Method/ +// CodeElement) actually PARSES — LadybugDB only rejects a disjunction that +// names a reserved keyword (e.g. `Macro`, `Union`) or a missing node table, +// neither of which applies here. So this query was NOT broken by #2325; it +// uses the `labels(n) IN` form for consistency with the manifest custom-branch +// fix (which WAS broken) and to stay immune if a reserved-keyword label is +// added later. `labels(n)` returns the node's single label as a string here, so +// `IN [...]` is an exact allowlist. (Exported so integration tests can run the +// exact production query against a real LadybugDB — the bug shipped because no +// test ran these strings against the real parser.) // // `n.filePath <> ''` excludes synthetic non-source `CodeElement` nodes that // carry no real file — ORM model/table nodes (orm.ts emits `filePath: ''`) and @@ -91,9 +102,9 @@ RETURN sym.id AS uid, sym.name AS name, sym.filePath AS filePath, // degenerate edge-less node NOR inflates the uniqueness count and masks the real // handler. `LIMIT 2` bounds materialization: distinguishing unique (1) from // ambiguous (>=2) never needs more than two rows (the count guard stays exact). -const RESOLVE_BY_NAME_QUERY = ` -MATCH (n:Function|Method|CodeElement) -WHERE n.name = $name AND n.filePath <> '' +export const RESOLVE_BY_NAME_QUERY = ` +MATCH (n) WHERE labels(n) IN ['Function','Method','CodeElement'] + AND n.name = $name AND n.filePath <> '' RETURN n.id AS uid, n.name AS name, n.filePath AS filePath LIMIT 2`; @@ -103,9 +114,9 @@ LIMIT 2`; // the precise rung — it survives aliases and local same-name collisions that a // repo-wide name lookup cannot, and only resolves on a unique match within that // module. `LIMIT 2` keeps the uniqueness count exact (see RESOLVE_BY_NAME_QUERY). -const RESOLVE_IN_MODULE_QUERY = ` -MATCH (n:Function|Method|CodeElement) -WHERE n.name = $name AND (n.filePath STARTS WITH $fileDot OR n.filePath STARTS WITH $fileSlash) +export const RESOLVE_IN_MODULE_QUERY = ` +MATCH (n) WHERE labels(n) IN ['Function','Method','CodeElement'] + AND n.name = $name AND (n.filePath STARTS WITH $fileDot OR n.filePath STARTS WITH $fileSlash) RETURN n.id AS uid, n.name AS name, n.filePath AS filePath LIMIT 2`; diff --git a/gitnexus/src/core/group/extractors/manifest-extractor.ts b/gitnexus/src/core/group/extractors/manifest-extractor.ts index 8b2a80f65..53e65c20f 100644 --- a/gitnexus/src/core/group/extractors/manifest-extractor.ts +++ b/gitnexus/src/core/group/extractors/manifest-extractor.ts @@ -7,6 +7,21 @@ export interface ManifestExtractResult { crossLinks: CrossLink[]; } +// Repo-wide symbol lookup for `custom` workspace contracts. Exported so the +// #2325 integration test can run the EXACT production query against a real +// LadybugDB — a hand-copied query string in the test would silently drift +// from this allowlist. Uses the `labels(n) IN [...]` allowlist form rather +// than a `MATCH (n:A|B)` disjunction: this 21-label list contains the +// reserved-keyword labels `Macro` and `Union`, and LadybugDB's parser rejects +// a disjunction that names a reserved keyword (#2325) — which the resolver's +// try/catch then swallowed. `labels(n) IN` has no such collision. +export const CUSTOM_CONTRACT_RESOLVE_QUERY = `MATCH (n) + WHERE labels(n) IN ['Function','Method','Class','Interface','Struct','Enum','Trait','Constructor','TypeAlias','Impl','Macro','Union','Typedef','Property','Record','Delegate','Annotation','Template','Const','Static','CodeElement'] + AND n.name = $symbolName + RETURN n.id AS uid, n.name AS name, n.filePath AS filePath + ORDER BY n.filePath ASC + LIMIT 1`; + /** * Canonicalize an HTTP path for matching against Route.name in the graph. * Mirrors core/ingestion/pipeline.ts ensureSlash semantics: @@ -189,6 +204,18 @@ export class ManifestExtractor { // Cross-impact still works: the bridge query joins on the synthetic // uid, and the local impact engine derives the same uid for the // unresolved symbol — name-based hints are the additional safety net. + // + // Label filtering uses `MATCH (n) WHERE labels(n) IN [...]`, NOT the + // openCypher disjunction `MATCH (n:A|B|C)`. LadybugDB's parser rejects a + // disjunction that names a reserved keyword (`Macro` and `Union` both are) + // OR a label with no node table (e.g. the old `lib` branch's `Package`). + // The `custom` branch (reserved keywords in its list) and `lib` branch + // (missing `Package` table) genuinely threw (#2325) and the whole try/catch + // below swallowed it; the other branches parsed but use the same form for + // consistency and future-proofing. `labels(n)` returns the node's single + // label as a string here, so `IN [...]` is an exact allowlist that includes + // listed labels and excludes everything else — and is immune to both + // failure modes (no keyword collision; an unknown label is just a non-match). try { let rows: Record[]; if (link.type === 'http') { @@ -222,7 +249,7 @@ export class ManifestExtractor { // avoid cross-matching Files/Variables/Imports that happen to // share the topic name. rows = await executor( - `MATCH (n:Function|Method|Class|Interface) WHERE n.name = $contract + `MATCH (n) WHERE labels(n) IN ['Function','Method','Class','Interface'] AND n.name = $contract RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -246,7 +273,7 @@ export class ManifestExtractor { const methodName = parts[1]?.trim() ?? ''; if (methodName) { rows = await executor( - `MATCH (n:Function|Method) WHERE n.name = $methodName + `MATCH (n) WHERE labels(n) IN ['Function','Method'] AND n.name = $methodName RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -254,7 +281,7 @@ export class ManifestExtractor { ); } else if (serviceName) { rows = await executor( - `MATCH (n:Class|Interface) WHERE n.name = $serviceName + `MATCH (n) WHERE labels(n) IN ['Class','Interface'] AND n.name = $serviceName RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -266,11 +293,12 @@ export class ManifestExtractor { } else if (link.type === 'lib') { // Only exact match on the symbol's name. Previous fallback to // CONTAINS on n.filePath would promote "react" to "react-native" - // or "@types/react" — silent wrong attribution. Restrict to - // package-level labels so we don't return arbitrary symbols - // named after a library. + // or "@types/react" — silent wrong attribution. Restrict to the + // package-level `Module` label so we don't return arbitrary symbols + // named after a library. (There is no `Package` node table — see + // NODE_TABLES — so a `Package` entry only ever matched nothing.) rows = await executor( - `MATCH (n:Package|Module) WHERE n.name = $contract + `MATCH (n) WHERE labels(n) IN ['Module'] AND n.name = $contract RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -291,14 +319,7 @@ export class ManifestExtractor { const symbolName = link.contract.includes('::') ? link.contract.split('::').pop()! : link.contract; - rows = await executor( - `MATCH (n:Function|Method|Class|Interface|Struct|Enum|Trait|Constructor|TypeAlias|Impl|Macro|Union|Typedef|Property|Record|Delegate|Annotation|Template|Const|Static|CodeElement) - WHERE n.name = $symbolName - RETURN n.id AS uid, n.name AS name, n.filePath AS filePath - ORDER BY n.filePath ASC - LIMIT 1`, - { symbolName }, - ); + rows = await executor(CUSTOM_CONTRACT_RESOLVE_QUERY, { symbolName }); } else { return null; } diff --git a/gitnexus/test/integration/group/http-route-resolve-symbol.test.ts b/gitnexus/test/integration/group/http-route-resolve-symbol.test.ts new file mode 100644 index 000000000..efc5fb99d --- /dev/null +++ b/gitnexus/test/integration/group/http-route-resolve-symbol.test.ts @@ -0,0 +1,111 @@ +/** + * Issue #2325 (http-route extractor half): `RESOLVE_BY_NAME_QUERY` and + * `RESOLVE_IN_MODULE_QUERY` were converted from the `MATCH (n:A|B)` multi-label + * disjunction to the `labels(n) IN [...]` allowlist form, for consistency with + * the manifest custom-branch fix. + * + * IMPORTANT nuance (verified against the real parser): the actual #2325 failure + * was triggered by *reserved-keyword* labels. LadybugDB's parser rejects a + * disjunction that names a reserved keyword — `Macro` and `Union` both are — so + * the manifest custom branch (whose 21-label list contains `Macro`/`Union`) + * genuinely threw, and the resolver's try/catch swallowed it. The http-route + * 3-label disjunction `(n:Function|Method|CodeElement)` contains no reserved + * keyword and actually PARSES — so this half was a consistency conversion, not a + * parser fix. The value of these cases is verifying the EXPORTED production + * queries resolve and filter correctly against a real LadybugDB (the unit tests + * cover the resolution *logic* with a fake executor; these cover *parsing + + * filtering*). The last case pins the real reserved-keyword trigger so a future + * query that reintroduces a `MATCH (n:…|Macro|…)` disjunction is caught. + */ +import { it, expect, afterEach } from 'vitest'; +import { + RESOLVE_BY_NAME_QUERY, + RESOLVE_IN_MODULE_QUERY, +} from '../../../src/core/group/extractors/http-route-extractor.js'; +import { initLbug, executeParameterized, closeLbug } from '../../../src/core/lbug/pool-adapter.js'; +import { withTestLbugDB } from '../../helpers/test-indexed-db.js'; + +const SEED = [ + // BY_NAME: one real-file Function + a same-named File (label excluded) + a + // same-named CodeElement with empty filePath (excluded by `n.filePath <> ''`). + `CREATE (:Function {id:'fn:getOrders', name:'getOrders', filePath:'src/handlers/orders.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:File {id:'file:getOrders', name:'getOrders', filePath:'src/getOrders.ts'})`, + `CREATE (:CodeElement {id:'ce:getOrders', name:'getOrders', filePath:''})`, + // IN_MODULE: same name in two modules — only the prefixed one resolves. + `CREATE (:Function {id:'fn:listUsers:handlers', name:'listUsers', filePath:'src/handlers/users.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:Function {id:'fn:listUsers:admin', name:'listUsers', filePath:'src/admin/users.ts', startLine:1, endLine:9, content:'', description:''})`, + // IN_MODULE label-allowlist decoy: same name, SAME module prefix, wrong label. + // The STARTS-WITH prefix would match it, so only the `labels(n) IN [...]` filter + // excludes it — drop the filter and this surfaces, flipping the row count. + `CREATE (:File {id:'file:listUsers:handlers', name:'listUsers', filePath:'src/handlers/users.ts'})`, + // LIMIT 2 cap: three same-named Functions — the uniqueness count must stay exact. + `CREATE (:Function {id:'fn:dup:1', name:'dup', filePath:'src/a.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:Function {id:'fn:dup:2', name:'dup', filePath:'src/b.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:Function {id:'fn:dup:3', name:'dup', filePath:'src/c.ts', startLine:1, endLine:9, content:'', description:''})`, +]; + +withTestLbugDB( + 'issue-2325-http-route-resolveSymbol', + (handle) => { + afterEach(async () => { + try { + await closeLbug(handle.repoId); + } catch { + /* best-effort */ + } + }); + + it('RESOLVE_BY_NAME_QUERY resolves a handler by name, excluding a File and an empty-filePath node', async () => { + await initLbug(handle.repoId, handle.dbPath); + const rows = await executeParameterized(handle.repoId, RESOLVE_BY_NAME_QUERY, { + name: 'getOrders', + }); + // File (wrong label) and the empty-filePath CodeElement are both excluded. + expect(rows).toHaveLength(1); + expect(rows[0].uid).toBe('fn:getOrders'); + expect(rows[0].filePath).toBe('src/handlers/orders.ts'); + }); + + it('RESOLVE_IN_MODULE_QUERY resolves only the handler in the target module prefix', async () => { + await initLbug(handle.repoId, handle.dbPath); + const rows = await executeParameterized(handle.repoId, RESOLVE_IN_MODULE_QUERY, { + name: 'listUsers', + fileDot: 'src/handlers/users.', + fileSlash: 'src/handlers/users/', + }); + expect(rows).toHaveLength(1); + expect(rows[0].uid).toBe('fn:listUsers:handlers'); + expect(rows[0].filePath).toBe('src/handlers/users.ts'); + }); + + it('RESOLVE_BY_NAME_QUERY caps materialization at 2 rows (uniqueness count stays exact)', async () => { + await initLbug(handle.repoId, handle.dbPath); + const rows = await executeParameterized(handle.repoId, RESOLVE_BY_NAME_QUERY, { + name: 'dup', + }); + // Three matches exist; LIMIT 2 returns exactly two so the caller treats it + // as ambiguous (>=2) without over-materializing. + expect(rows).toHaveLength(2); + }); + + it('LadybugDB rejects a disjunction naming a reserved-keyword label (the real #2325 trigger)', async () => { + await initLbug(handle.repoId, handle.dbPath); + // The genuine #2325 failure: a `MATCH (n:A|B)` disjunction whose label set + // includes a reserved keyword (`Macro`/`Union`) is a parser error the + // resolver's try/catch silently swallowed. The exported queries avoid this + // by using the `labels(n) IN [...]` allowlist form. (The http-route + // `Function|Method|CodeElement` form parses — this guards the real cause.) + await expect( + executeParameterized( + handle.repoId, + `MATCH (n:Function|Macro|Union) WHERE n.name = $name RETURN n.id AS uid LIMIT 2`, + { name: 'getOrders' }, + ), + ).rejects.toThrow(/Parser exception|Invalid input|Prepare failed/i); + }); + }, + { + seed: SEED, + poolAdapter: true, + }, +); diff --git a/gitnexus/test/integration/group/manifest-resolve-symbol-2325.test.ts b/gitnexus/test/integration/group/manifest-resolve-symbol-2325.test.ts new file mode 100644 index 000000000..98ef9fc01 --- /dev/null +++ b/gitnexus/test/integration/group/manifest-resolve-symbol-2325.test.ts @@ -0,0 +1,173 @@ +/** + * Regression test for issue #2325: + * manifest-extractor `resolveSymbol` used a multi-label Cypher disjunction + * (`MATCH (n:Function|Method|Class|...)`). LadybugDB's parser rejects such a + * disjunction only when a label is a reserved keyword (`Macro` and `Union` both + * are) or names a missing node table — so the `custom` branch (reserved + * keywords in its 21-label list) and the `lib` branch (no `Package` table) + * genuinely threw, silently falling back to a synthetic UID with empty + * `filePath`; the other branches happened to parse. The fix uses + * `MATCH (n) WHERE labels(n) IN [...]` uniformly — immune to both failure modes + * (LadybugDB returns a node's single label as a string, so `IN [...]` is an + * exact allowlist). + * + * These cases run the REAL production query (via `extractFromManifest`) against a + * real LadybugDB — the only layer that can catch a parser rejection, since the + * unit tests mock the executor. Each per-branch case seeds a wrong-label decoy + * whose `filePath` sorts BEFORE the target so a widened/broken allowlist would + * surface the decoy under `ORDER BY n.filePath ASC LIMIT 1` and flip the + * assertion — making the exclusion check non-vacuous. + */ +import { it, expect, afterEach } from 'vitest'; +import { + ManifestExtractor, + CUSTOM_CONTRACT_RESOLVE_QUERY, +} from '../../../src/core/group/extractors/manifest-extractor.js'; +import type { GroupManifestLink } from '../../../src/core/group/types.js'; +import type { CypherExecutor } from '../../../src/core/group/contract-extractor.js'; +import { initLbug, executeParameterized, closeLbug } from '../../../src/core/lbug/pool-adapter.js'; +import { withTestLbugDB } from '../../helpers/test-indexed-db.js'; + +// Targets + sort-first wrong-label decoys for each link-type branch. +const SEED = [ + // custom → Class; decoy File (not in allowlist) sorts first. + `CREATE (:Class {id:'cls:MyServiceFacade', name:'MyServiceFacade', filePath:'src/main/java/com/example/MyServiceFacade.java', startLine:1, endLine:42, content:'', description:''})`, + `CREATE (:File {id:'file:MyServiceFacade', name:'MyServiceFacade', filePath:'aaa/MyServiceFacade.java'})`, + // grpc/thrift method → Method; decoy Class (not in [Function,Method]) sorts first. + `CREATE (:Method {id:'mth:Login', name:'Login', filePath:'src/auth_grpc.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:Class {id:'cls:Login', name:'Login', filePath:'aaa/Login.java', startLine:1, endLine:9, content:'', description:''})`, + // grpc/thrift service → Class; decoy Function (not in [Class,Interface]) sorts first. + `CREATE (:Class {id:'cls:AuthService', name:'AuthService', filePath:'src/auth_service.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:Function {id:'fn:AuthService', name:'AuthService', filePath:'aaa/AuthService.go', startLine:1, endLine:9, content:'', description:''})`, + // lib → Module (there is NO Package node table); decoy Function sorts first. + `CREATE (:Module {id:'mod:mylib', name:'mylib', filePath:'src/index.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:Function {id:'fn:mylib', name:'mylib', filePath:'aaa/mylib.ts', startLine:1, endLine:9, content:'', description:''})`, + // topic → Function; decoy File sorts first. + `CREATE (:Function {id:'fn:orders.created', name:'orders.created', filePath:'src/consumer.ts', startLine:1, endLine:9, content:'', description:''})`, + `CREATE (:File {id:'file:orders.created', name:'orders.created', filePath:'aaa/orders.created.ts'})`, +]; + +/** Custom-branch `custom` query as emitted BEFORE the fix — kept to document + * the real failure: its label list contains the reserved keywords `Macro` and + * `Union`, which make LadybugDB's parser reject the whole disjunction. */ +const MULTI_LABEL_CUSTOM_QUERY = `MATCH (n:Function|Method|Class|Interface|Struct|Enum|Trait|Constructor|TypeAlias|Impl|Macro|Union|Typedef|Property|Record|Delegate|Annotation|Template|Const|Static|CodeElement) + WHERE n.name = $symbolName + RETURN n.id AS uid, n.name AS name, n.filePath AS filePath + ORDER BY n.filePath ASC + LIMIT 1`; + +// Direct-query canary for the `labels(n)`-is-a-string assumption the whole fix +// relies on. Uses the EXACT production query (imported, not hand-copied) so the +// canary can never silently drift from the real allowlist. + +withTestLbugDB( + 'issue-2325-manifest-resolveSymbol', + (handle) => { + afterEach(async () => { + try { + await closeLbug(handle.repoId); + } catch { + /* best-effort */ + } + }); + + // role:'consumer' → providerRepo = link.to = 'repo-b' (the seeded DB); the + // resolved symbol lands in crossLinks[0].to. consumerRepo ('repo-a') has no + // executor, so to.symbolRef carries the provider-side resolution. + const resolveVia = async ( + type: GroupManifestLink['type'], + contract: string, + ): Promise<{ symbolUid: string; filePath: string; name: string }> => { + await initLbug(handle.repoId, handle.dbPath); + const executor: CypherExecutor = (query, params) => + executeParameterized(handle.repoId, query, params ?? {}); + const extractor = new ManifestExtractor(); + const result = await extractor.extractFromManifest( + [{ from: 'repo-a', to: 'repo-b', type, contract, role: 'consumer' }], + new Map([['repo-b', executor]]), + ); + expect(result.crossLinks).toHaveLength(1); + const to = result.crossLinks[0].to; + return { + symbolUid: to.symbolUid, + filePath: to.symbolRef.filePath, + name: to.symbolRef.name, + }; + }; + + it('LadybugDB rejects the custom-branch disjunction (its list names reserved keywords Macro/Union)', async () => { + await initLbug(handle.repoId, handle.dbPath); + // The genuine #2325 trigger for the custom branch: `Macro` and `Union` are + // reserved keywords, so the parser rejects the whole `(n:…|Macro|…|Union|…)` + // disjunction — which the resolver's try/catch then swallowed. + await expect( + executeParameterized(handle.repoId, MULTI_LABEL_CUSTOM_QUERY, { + symbolName: 'MyServiceFacade', + }), + ).rejects.toThrow(/Parser exception|Invalid input|Prepare failed/i); + }); + + it('direct labels(n) IN query resolves the symbol (canary for labels()-is-a-string)', async () => { + await initLbug(handle.repoId, handle.dbPath); + const rows = await executeParameterized(handle.repoId, CUSTOM_CONTRACT_RESOLVE_QUERY, { + symbolName: 'MyServiceFacade', + }); + expect(rows).toHaveLength(1); + expect(rows[0].uid).toBe('cls:MyServiceFacade'); + expect(rows[0].filePath).toBe('src/main/java/com/example/MyServiceFacade.java'); + }); + + it('custom contract resolves the real Class symbol, excluding a same-named File (#2325)', async () => { + const r = await resolveVia('custom', 'custom::MyServiceFacade'); + expect(r.symbolUid).toBe('cls:MyServiceFacade'); + expect(r.filePath).toBe('src/main/java/com/example/MyServiceFacade.java'); + expect(r.name).toBe('MyServiceFacade'); + // real resolution, not the synthetic fallback the bug produced + expect(r.symbolUid.startsWith('manifest::')).toBe(false); + }); + + it('grpc method contract resolves a Method, excluding a same-named Class', async () => { + const r = await resolveVia('grpc', 'AuthService/Login'); + expect(r.symbolUid).toBe('mth:Login'); + expect(r.filePath).toBe('src/auth_grpc.ts'); + expect(r.symbolUid.startsWith('manifest::')).toBe(false); + }); + + it('grpc service contract resolves a Class, excluding a same-named Function', async () => { + const r = await resolveVia('grpc', 'AuthService'); + expect(r.symbolUid).toBe('cls:AuthService'); + expect(r.filePath).toBe('src/auth_service.ts'); + expect(r.symbolUid.startsWith('manifest::')).toBe(false); + }); + + it('thrift service contract strips the package prefix before resolving the Class', async () => { + // The thrift-only branch strips `package.` from the service name + // (`com.example.AuthService` -> `AuthService`). Without the strip the + // lookup would query for `com.example.AuthService`, match nothing, and + // fall back to a synthetic `manifest::` uid — so this resolving to the + // real Class is the load-bearing assertion for the package-strip path. + const r = await resolveVia('thrift', 'com.example.AuthService'); + expect(r.symbolUid).toBe('cls:AuthService'); + expect(r.filePath).toBe('src/auth_service.ts'); + expect(r.symbolUid.startsWith('manifest::')).toBe(false); + }); + + it('lib contract resolves a Module, excluding a same-named Function', async () => { + const r = await resolveVia('lib', 'mylib'); + expect(r.symbolUid).toBe('mod:mylib'); + expect(r.filePath).toBe('src/index.ts'); + expect(r.symbolUid.startsWith('manifest::')).toBe(false); + }); + + it('topic contract resolves a Function, excluding a same-named File', async () => { + const r = await resolveVia('topic', 'orders.created'); + expect(r.symbolUid).toBe('fn:orders.created'); + expect(r.filePath).toBe('src/consumer.ts'); + expect(r.symbolUid.startsWith('manifest::')).toBe(false); + }); + }, + { + seed: SEED, + poolAdapter: true, + }, +); diff --git a/gitnexus/test/unit/group/manifest-extractor.test.ts b/gitnexus/test/unit/group/manifest-extractor.test.ts index c732a16a3..5e15460a3 100644 --- a/gitnexus/test/unit/group/manifest-extractor.test.ts +++ b/gitnexus/test/unit/group/manifest-extractor.test.ts @@ -839,7 +839,17 @@ describe('ManifestExtractor', () => { await extractor.extractFromManifest(links, dbExecutors); - expect(capturedCypher).toContain('Function|Method|Class|Interface|Struct|Enum|Trait'); + // This `custom`-branch list contains the reserved keywords `Macro`/`Union`, + // which make LadybugDB's parser reject the `MATCH (n:A|B|C)` disjunction + // (#2325), so the allowlist is carried as `labels(n) IN [...]`, not `n:A|B`. + expect(capturedCypher).toContain('labels(n) IN ['); + // Membership checks that tolerate label order/spacing changes in the + // production allowlist (the negative guards below are the real regression + // check — a re-introduced `:A|B` disjunction has no quoted labels at all). + expect(capturedCypher).toContain("'Function'"); + expect(capturedCypher).toContain("'Method'"); + expect(capturedCypher).toContain("'CodeElement'"); + expect(capturedCypher).not.toContain('Function|Method'); expect(capturedCypher).not.toContain('NOT n:File'); }); diff --git a/gitnexus/vitest.config.ts b/gitnexus/vitest.config.ts index 741ecc6a5..4b7355b27 100644 --- a/gitnexus/vitest.config.ts +++ b/gitnexus/vitest.config.ts @@ -72,6 +72,8 @@ export default defineConfig({ 'test/integration/analyze-wal-checkpoint-failure.test.ts', 'test/integration/lbug-non-ascii-path.test.ts', 'test/integration/lbug-conn-serialization.test.ts', + 'test/integration/group/manifest-resolve-symbol-2325.test.ts', + 'test/integration/group/http-route-resolve-symbol.test.ts', ], fileParallelism: false, sequence: { groupOrder: 1 }, @@ -107,6 +109,8 @@ export default defineConfig({ 'test/integration/analyze-wal-checkpoint-failure.test.ts', 'test/integration/lbug-non-ascii-path.test.ts', 'test/integration/lbug-conn-serialization.test.ts', + 'test/integration/group/manifest-resolve-symbol-2325.test.ts', + 'test/integration/group/http-route-resolve-symbol.test.ts', 'test/integration/skills-e2e.test.ts', ], },