mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-05 02:43:32 +00:00
7 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
pcaceres-hazloagil
|
a8736a07d0
|
fix(lbug): checkpoint race in pool-adapter.ts + pin @ladybugdb/core to 0.18.3 (#3189)
* fix(lbug): await evict-then-reopen so it can't race the checkpoint
closeOne() closed the evicted repo's shared Database with a
fire-and-forget `db.close().catch(() => {})` (no await). Both call
sites that evict-then-reopen — evictLRU() right before doInitLbug
opens the new connection, and the "idle & changed" path in initLbug —
proceeded to open the next repo's connection immediately after,
without waiting for the evicted repo's close (and the checkpoint it
triggers) to finish. On the real engine the new open can then collide
with that still-in-flight checkpoint, surfacing on any read as:
Runtime exception: Cannot open database in read-only mode while
checkpoint is in progress. Please retry later.
This reproduces reliably once more than MAX_POOL_SIZE (5) distinct
repos are queried within a short window (self-hosted deployments with
more than a handful of active repos hit it routinely), and gets worse
under genuinely concurrent requests for different repos, since nothing
serialized pool mutations across callers either.
Fix:
- closeOne / evictLRU are now async and await their internal work
(closeOne's own close() call; evictLRU's call to closeOne), closing
the race within a single initLbug call.
- The exported initLbug is wrapped in a small async mutex
(initLbugInner does the real work) so concurrent initLbug calls for
different repos serialize instead of each racing their own
evict-then-reopen against the others.
- closeLbug's two closeOne() calls are now awaited too — closeOne
becoming async meant closeLbug could resolve before pool.delete()
had actually run, which a repo-pinning test caught (isLbugReady()
briefly still true right after a resolved closeLbug()).
- closeOne now deletes the pool entry (and clears its pin, and
notifies pool-close listeners) BEFORE the awaited db.close(), not
after. Review caught that the previous order left a "zombie" entry
reachable via pool.get(repoId) — closed=true, available emptied, but
still present — for the duration of that await; a same-repo
query/init landing in that window would see isLbugReady() as true
and hit a "Connection pool integrity error" in checkout() instead of
just reopening. Deleting first removes the entry entirely, so a
concurrent caller takes the normal fresh-open path instead.
Verified two ways:
- Against the compiled bundle (`ghcr.io/abhigyanpatwari/gitnexus`,
1.6.10/1.6.11 — pool-adapter.js is byte-identical between them): an
A/B docker build with 7 tiny local repos and genuinely concurrent
(parallel, not sequential) /api/graph requests goes from 7/7 failing
to 7/7 succeeding on a freshly-analyzed pool.
- Unit tests here (mocks @ladybugdb/core the same way as
lbug-pool-pinning.test.ts): one asserts the evicted repo's close()
completes before the initLbug call that triggered the eviction
settles; another asserts closeLbug's own promise doesn't resolve
before the underlying close() does. Both gate their mock's close()
on a real short delay and were confirmed to fail against code that
drops the corresponding await.
Note: a second, deeper issue was also observed in the docker A/B
setup — repeated rounds of concurrent access show a repo that has
gone through one evict+reopen cycle can become permanently unable to
reopen for reads, identically with and without this fix. That did not
reproduce with mocks and isn't understood yet; filed separately as
#3186, which stays open and untouched by this PR — this fix closes a
real, root-caused bug on its own but does not resolve #3186 by itself.
Second review round caught a follow-up: the idle-timeout sweep calls
closeOne(repoId) directly, outside of initLbug's poolLock. Now that
closeOne deletes the pool entry before its awaited close(), an
unsynchronized idle close racing a same-repo initLbug could let that
initLbug treat the repo as absent while the idle close (and its
checkpoint) is still in flight — reopening the same class of race this
PR exists to close, just via the idle path instead of LRU eviction.
Routed the idle sweep's closeOne call through withPoolLock too, so it
serializes against initLbug the same way evictLRU already does.
(Tried to add a mocked regression test for this specific interleaving;
dropped it — the mock's dbCache-reuse path masks the difference
regardless of the fix, so it could not be made to discriminate
reliably. Fixed by direct code review instead, same as the note below
already does for the native-engine-specific checkpoint collision.)
Also removed the initLbugInner per-repoId initPromises dedup map: with
every initLbug call now serialized through poolLock, a second call for
a repoId already being initialized cannot observe a pending promise in
initPromises (the first call always fully completes, including its
finally-block cleanup, before the lock releases) — the branch was dead
code the bot correctly flagged twice.
Third review round caught two more follow-ups on the same theme (both
introduced by making the idle sweep route through poolLock):
- closeLbug()'s no-arg ("close everything") branch still calls closeOne
directly in a loop over a snapshotted pool.keys(), without the lock —
an initLbug racing that loop could register a fresh entry the
snapshot never saw, leaving it resident after a call meant to empty
the pool. Wrapped the snapshot+loop in withPoolLock.
- The idle timer callback can now sit queued behind an in-progress
initLbug before its turn arrives, and that init (or a concurrent
touchRepo()) can refresh lastUsed in the meantime — so the pre-lock
idleness check taken when the timer fired can be stale by the time
it actually runs. Re-check lastUsed/checkedOut again inside the lock,
right before closing, instead of trusting the outer snapshot.
* fix(deps): pin @ladybugdb/core back to 0.18.3
Bisected the "checkpoint is in progress" symptom (root cause #2, not
touched by the pool-adapter.ts fix in the previous commit) down to a
single dependency-version-bump commit with zero application code
changes:
|
||
|
Gergő Magyar
|
9538be957d
|
fix(lbug): scale the buffer-pool budget by the OS page-size granule ratio (#2631) (#2636)
* fix(lbug): scale the buffer-pool budget by the OS-page discard-granule ratio (#2631) LadybugDB bills buffer-pool budget per discard granule, not per 4 KiB frame: the engine's vm_region.cpp sets discardGranuleSize = max(frameSize, osPageSize), claimFrame charges the whole granule when its first frame becomes resident, and releaseFrame refunds only when the granule's last frame leaves — while BufferManager::reserve measures eviction progress in refunded bytes and throws 'The buffer pool is full and no memory could be freed!' after three zero-refund passes. On a 64 KiB-page kernel (Ascend/aarch64 openEuler — the #2631 reporter's host) that is 16 frames per granule: the same COPY bills up to 16× the budget it needs on x86, and whole eviction passes can evict frames yet refund nothing. Apple Silicon macOS (16 KiB pages) is the same mechanism at 4×. Measured with the reporter's exact command and version: vllm-ascend needs a (128, 256] MiB pool on 4 KiB pages — 64/128 MiB reproduce the reporter's byte-identical error, 256 MiB and the 576 MiB adaptive pool succeed — so their 64 KiB host cannot survive on a page-size-blind budget. Scale every derived pool size by granuleRatio = max(1, osPageSize/4096): the per-element estimate, the COPY-safety floor, and the default cap (still bounded by 80% of RAM). 4 KiB hosts are byte-identical to before — proven by pinning the existing sizing tests to an explicit 4096 page size, which also stops them drifting on 16 KiB Apple Silicon runners. GITNEXUS_LBUG_BUFFER_POOL_SIZE keeps absolute precedence and 0 still restores the native default. Also: bufferPoolExhaustionRemedy() gives the exhaustion error an actionable cause→consequence→remedy message; the isLbugPageSizeFrameError comment that called pool exhaustion 'a sizing problem, not a page-size one' is corrected — that framing inverted when #2582 made pool size a function of a page-size-blind estimate. Cannot execute on a 64 KiB kernel here: the scaled path is proven by unit stubs plus the engine-source math above; the env override remains the field escape hatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(cli): actionable pool-exhaustion remedies at the COPY sites and a doctor pool line (#2631) The node-COPY throw and the relationship-COPY warning now append bufferPoolExhaustionRemedy() when the failure is the engine's pool-exhaustion class: the raw binder text gave the operator nothing to act on, and on non-4K-page hosts the pool bills up to pageSize/4KiB × faster than the sizing was calibrated for. The relationship path appends the remedy once per bulk load, not once per failed pair. doctor prints the effective pool size next to the page-size line ('pool size 2048 MiB', with an '(×N page-size scaling)' suffix on non-4K hosts) so support triage sees the sizing inputs at a glance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(lbug): re-anchor getEffectiveBufferPoolSize's placement and reuse granuleRatio in doctor Self-review fixes: the getter's insertion had orphaned resolveBufferManagerSize's doc comment (it read as documenting the wrong function), and doctor's scale note duplicated the granule math with a hardcoded 4096. granuleRatio is now exported (it already carried the test-seam default param) and doctor consumes it. No behavioral change — the sizing suite pins byte-identical outputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lbug): keep the hintless pool default unscaled and make both remedies visible (#2631) Review fixes: - Scale only the analyze-path cap (scaledAnalyzePoolCap), not defaultBufferPoolSize: the pool is an eager native allocation at DB open (measured, see POOL_BYTES_PER_ELEMENT), so a page-size-scaled hintless default would hand a long-lived MCP process up to 80% of RAM — the #2557 OOM exposure the 2 GiB cap removed. Fix the MAP_NORESERVE claim that contradicted that measurement. - Log the rel-pair pool remedy (loadGraphToLbug returns warnings that no call site reads) and dedup it with a local boolean instead of matching the remedy's own wording. - Label the GITNEXUS_LBUG_BUFFER_POOL_SIZE=0 sentinel as the native 80%-of-RAM default in both the remedy and doctor instead of '0 MiB'. - Extract poolSizeDoctorLine (pageSizeDoctorLines convention): mark env overrides, drop the scaling suffix that misdescribed absolute values. - Fold _resetOsPageSizeCacheForTest into _setOsPageSizeForTests(undefined). - Document the analyze-path scaling in both README env tables. --------- Co-authored-by: Gergo Magyar <abhigyan1.patwari@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Claude
|
05dadb7950 |
fix(lbug): raise the adaptive pool floor to a COPY-safe 256 MiB
The 64 MiB floor was too small: LadybugDB's bulk COPY needs working buffer-pool memory that scales with the repo, so a 64 MiB pool fails with "buffer pool is full and no memory could be freed" on any non-trivial repo (empirically: the 6-file skills-e2e idempotency fixture needs >=128 MiB; the ~1800-file GitNexus checkout needs >=256 MiB). Introduce a distinct ADAPTIVE_POOL_FLOOR (256 MiB) for the hint clamp, kept separate from BUFFER_POOL_FLOOR (64 MiB), which still guards defaultBufferPoolSize on tiny-RAM machines; the hint is still clamped up to the machine default so it can never over-commit. This keeps the change as what it actually is — a large-repo optimization: GitNexus full analyze is 51s (2 GiB) -> 35s (adaptive ~414 MiB). Small repos now open COPY-safely at 256 MiB instead of the 2 GiB default (same wall time on Linux, where commit is lazy; the eager commit is far cheaper than 2 GiB on Windows). The reframed comments drop the earlier unrepresentative "3-file repo / 64 MiB fast" claim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Claude
|
ad5ff42804 |
feat(lbug): add adaptive buffer-pool size hint
Adds the sizing lever without changing behavior yet: a module-scoped buffer-pool size hint plus estimateBufferPool(graphElementCount), read by resolveBufferManagerSize with precedence env-override > clamp(hint, 64MiB, default) > default. The hint can only shrink the pool from the default (clamped to [floor, default]), so the 2GiB/80%-RAM cap and the GITNEXUS_LBUG_BUFFER_POOL_SIZE escape hatch (incl. 0) are preserved. With no hint set, resolveBufferManagerSize returns exactly what it did before. Motivation: LadybugDB eagerly commits the buffer pool at DB open, so the fixed min(2GiB,80%RAM) pool adds a measured ~2.8s to every analyze even on a 3-file repo (dominant on Windows). Sizing the pool to the graph lets small repos use the fast 64MiB floor while large repos keep the cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
249f5c7aab
|
fix(lbug): bound the LadybugDB buffer pool instead of the native 80%-of-RAM default (#2560)
* fix(lbug): bound the LadybugDB buffer pool instead of the native 80%-of-RAM default (#2557) createLbugDatabase passed bufferManagerSize=0, which the native runtime sizes at 80% of physical RAM. A long-lived gitnexus mcp process (or a large incremental analyze) could balloon to that ceiling — 19.5 GiB observed against a 105 MiB on-disk index — and OOM-kill the host session. Resolve the pool at call time: default min(2 GiB, max(64 MiB, 80% of totalmem)), overridable via GITNEXUS_LBUG_BUFFER_POOL_SIZE (bytes); 0 deliberately restores the native unbounded default; invalid values warn and fall back, mirroring GITNEXUS_WAL_CHECKPOINT_THRESHOLD. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(readme): document GITNEXUS_LBUG_BUFFER_POOL_SIZE and GITNEXUS_LBUG_MAX_DB_SIZE (#2557) Both env tables gain the new buffer-pool ceiling variable and the previously code-comment-only GITNEXUS_LBUG_MAX_DB_SIZE, with the mmap-vs-memory distinction the issue had to discover from source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(lbug): keep parseBufferPoolSize module-private Review finding: the export had zero importers — parseWalCheckpointThreshold earns its export via the CLI flag validation, but the buffer-pool CLI flag was deliberately deferred. Re-export when a consumer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(autofix): apply prettier + eslint fixes via /autofix command --------- Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Copilot
|
87b91c821e
|
fix(lbug): add WAL checkpoint-threshold control (#1772)
* Initial plan * fix(analyze): add WAL auto-checkpoint CLI control and default-off behavior * test(analyze): share lbug auto-checkpoint parsing and align validation * fix(analyze): always enable lbug auto-checkpoint and expose threshold control * refactor(lbug): inline always-on auto-checkpoint constructor arg * fix(analyze): guide checkpoint-threshold on Ladybug WAL checkpoint IO failures * test(analyze): cover checkpoint IO guidance and add integration guard * fix(analyze): tighten checkpoint IO detection and remove test hook * fix(analyze): remove checkpoint test hook and tighten error matching * fix(analyze): rename to wal-checkpoint-threshold, raise default, add manual checkpoint driver with retry Address review feedback on PR #1772: - Rename CLI flag, env var, AnalyzeOptions field, recovery-hint tag, and parser/constants from lbug-* to engine-neutral wal-* (matches the existing WAL_RECOVERY_SUGGESTION / isWalCorruptionError convention). - Raise default threshold from -1 (Ladybug stock ~16 MiB) to 64 MiB so users on the default config no longer hit the original rename/remove race. - Align both READMEs to publish 67108864 (64 MiB) instead of 65536 (which would have made the crash more frequent). - Add wal-checkpoint-driver.ts: a periodic manual CHECKPOINT driver wrapped in a 3-attempt jittered retry (50/200/500 ms), driven from runFullAnalysis. Opt-out via GITNEXUS_WAL_MANUAL_CHECKPOINT=0. Moves the race window into a JS-controllable retry surface while keeping native auto-checkpoint on. - Move LBUG_CHECKPOINT_RENAME_RE / REMOVE_RE plus the predicate (renamed to isLbugCheckpointIoError) into lbug-config.ts alongside isWalCorruptionError. Predicate is now exported. Add a permissive fallback matcher and pin the matched Ladybug version in comments. - Warn instead of silently defaulting when GITNEXUS_WAL_CHECKPOINT_THRESHOLD is set to a non-empty unparseable value (closes the CLI-vs-env asymmetry). - Add a typed RecoveryHint string-literal union in cli-message.ts so future hint tags can't drift. - Add a real integration test under test/integration/ that triggers a Ladybug checkpoint IO failure via a pre-existing directory at the rename target (portable across platforms; no test-only injection hook). - Add small-disk / CI caveat (32 MiB secondary suggestion) to the recovery hint and README env-var rows. - Document CLI/env precedence in the analyze --help block. - Help placeholder: <value> -> <bytes>. - Rename analyze-lbug-auto-checkpoint.test.ts to use the new wal-* token. * chore(lbug): remove dead jitteredDelay helper and apply prettier - Drop unused `jitteredDelay` function flagged by CodeQL in PR #1772; the retry loop already inlines the same calculation with the injectable `randomImpl` so the helper was dead. Move the non-cryptographic-by-design comment next to the actual jitter site. - Apply `prettier --write` to wal-checkpoint-driver.ts and the new integration test to absorb the PR autofix bot's formatting findings. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: Test <test@example.com> |
||
|
evolution
|
8ca9cb1a4d
|
fix(lbug): recover from WAL corruption by quarantining .wal file (#1402) (#1417)
* fix(lbug): recover from WAL corruption by quarantining .wal file (#1402) LadybugDB crashes when the WAL file is corrupted — the open fails with an unrecoverable native error. This makes the pool adapter detect WAL corruption errors, quarantine the offending .wal file, and retry the open. MCP tool responses (cypher, context, impact) now include a recoverySuggestion field when WAL corruption is detected. Changes: - Add isWalCorruptionError() regex-based detector in lbug-config.ts - Add throwOnWalReplayFailure and enableChecksums to createLbugDatabase() - Extract openReadOnlyDatabase() with stdout silencing + db.init() - Add tryQuarantineAndReopen() for .wal quarantine + retry in doInitLbug - Wrap cypher/context/impact with WAL recoverySuggestion in MCP responses - Share WAL_RECOVERY_SUGGESTION constant across all MCP error paths - Fix restoreStdout() placement (before db.init() → finally block) - Add unit tests for detection, pool recovery, and MCP feedback * fix(test): remove superfluous argument from LocalBackend constructor (#1402) LocalBackend has no constructor — the { registryPath } argument was ignored. * fix(lbug): address WAL recovery review feedback --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |