fix(embeddings): add CHECKPOINT before closing database to prevent WA…

[LINSUISHENG034]

…L corruption

Summary

Motivation / context

Areas touched

• gitnexus/ (CLI / core / MCP server)
• gitnexus-web/ (Vite / React UI)
• .github/ (workflows, actions)
• eval/ or other tooling
• Docs / agent config only (AGENTS.md, CLAUDE.md, .cursor/, llms.txt, etc.)

Scope & constraints

In scope

Explicitly out of scope / not done here

Implementation notes

Testing & verification

• cd gitnexus && npm test
• cd gitnexus && npm run test:integration (if core/indexing/MCP paths changed)
• cd gitnexus && npx tsc --noEmit
• cd gitnexus-web && npm test (if web changed)
• cd gitnexus-web && npx tsc -b --noEmit (if web changed)
• Manual / Playwright E2E (note environment — see gitnexus-web/e2e/)

Risk & rollout

Checklist

• PR body meets repo minimum length (workflow may label short descriptions)
• If AGENTS.md / overlays changed: headers, scope block, and changelog updated per project conventions
• No secrets, tokens, or machine-specific paths committed

[vercel]

@LINSUISHENG034 is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

Claude finished @magyargergo's task in 5m 24s —— View job

---

PR #1314 Review: fix(embeddings): add CHECKPOINT before closing database to prevent WAL corruption

Tasks:

• Post initial comment
• Read repo docs (AGENTS.md, GUARDRAILS.md, DoD.md, CONTRIBUTING.md)
• Read lbug-adapter.ts (full file, all close paths)
• Inspect hidden Unicode / bidi characters
• Read embeddings files and test files
• Search for callers of withLbugDb, closeLbug, initLbug
• Check existing tests for lbug / embeddings / CHECKPOINT
• Check CI status
• Check package.json for LadybugDB version
• Analyze concurrency / session lock semantics
• Write full review

---

Review bar for this PR

• Every LadybugDB close path that can follow embedding writes performs a checkpoint before close
• CHECKPOINT happens under the correct session/connection lock (no concurrent write on same conn)
• CHECKPOINT failure handling is explicit and justified — no silent diagnostic loss
• closeLbug() preserves global state cleanup semantics
• At least one focused regression test or integration test covers checkpoint-before-close behavior
• No hidden Unicode bidi/invisible controls in executable code
• CI, typecheck, lint/format, focused lbug/embedding tests green

---

Current PR state

• Head SHA: f77904a
• Commits: 1
• Changed files: 1 (gitnexus/src/core/lbug/lbug-adapter.ts, +31/-0)
• CI status: CI workflow in progress; PR Conventional Labeler, Gitleaks, PR Description Check, Dependency Review, CodeQL all pass; Docker Build in progress
• Typecheck/lint/format: Not yet confirmed (CI in progress)
• Focused lbug/embedding test status: No dedicated CHECKPOINT tests exist in any test file; existing integration tests (lbug-core-adapter, lbug-lock-retry, lbug-pool, lbug-embedding-hashes) pass on CI based on prior runs but none test checkpoint-before-close ordering
• Regression test presence: None added by this PR
• Manual reproduction/verification: Not documented in PR body (template sections left empty)
• Hidden Unicode: Non-ASCII characters found at lines 28, 31, 139, 171, 207, 285, 289, 303, 310, 333, 367, 406, 434, 439, 695, 1030, 1038, 1048, 1055, 1079 — all are em dashes (—) in comments only, no bidi controls; no grep -P "[\x{202A}-\x{202E}\x{2066}-\x{2069}]" hits
• Vercel: Irrelevant to this data-integrity fix
• PR body vs code: PR body is entirely the default template with all sections empty — the code is implemented but the PR provides zero reproduction, motivation, test, or risk evidence

---

Understanding of the change

LadybugDB (@ladybugdb/core ^0.16.0, a KuzuDB-derived embedded graph DB) uses a WAL/shadow-sidecar architecture. Its internal checkpoint thread is non-blocking and can outlive a close() call — meaning if GitNexus closes the DB connection immediately after embedding writes, the WAL pages may not have been flushed into the main DB file. A subsequent read-side open can then race with WAL replay or trip the database-id check on stale sidecar files, manifesting as corruption or "zombie" embeddings.

This PR adds an explicit conn.query('CHECKPOINT') before every conn.close() call across three close paths in lbug-adapter.ts:

1. withLbugDb() stale/busy retry cleanup (lines 260–267) — inside runWithSessionLock
2. doInitLbug() DB-path switch cleanup (lines 305–311) — called only from within a runWithSessionLock context
3. closeLbug() (lines 1075–1081) — NOT inside runWithSessionLock

Global state in the adapter: conn, db, currentDbPath, ftsLoaded, vectorExtensionLoaded, ensuredFTSIndexes, and the module-level sessionLock promise chain.

---

Findings

[HIGH] closeLbug() executes CHECKPOINT outside the session lock

• Category: Concurrency / session lock
• Files: gitnexus/src/core/lbug/lbug-adapter.ts:1066–1098, gitnexus/src/server/api.ts:771,1693, gitnexus/src/core/run-analyze.ts:231,244,262,486,499, gitnexus/src/mcp/local/local-backend.ts:256,3843
• Issue: closeLbug() is a public export called directly from api.ts, run-analyze.ts, analyze-worker.ts, and local-backend.ts. None of these call sites wrap closeLbug() in runWithSessionLock. The function runs conn.query('CHECKPOINT') followed by conn.close() without any session lock, while an in-flight withLbugDb() operation may be actively using the same conn. The pre-existing race between closeLbug() and concurrent operations was there before this PR (the conn.close() was already racy), but this PR adds a new async conn.query() step before the close, widening the window and adding a new failure surface — a concurrent query on an active connection can corrupt the in-flight operation's state in ways that close() alone would not.
• Why it matters here: The shutdown handler in api.ts disposes job managers before calling closeLbug(), providing some practical protection. However, the delete-repo path at api.ts:771 holds only a repo-level lock (not the session lock) and calls closeLbug() while in-flight withLbugDb() calls from other request handlers can still be active. This makes the CHECKPOINT addition to closeLbug() potentially racing with active embedding reads or writes.
• Recommended fix: Wrap closeLbug()'s body in runWithSessionLock, or document with evidence that all callers of closeLbug() guarantee no concurrent operations are in flight. Add a concurrency-focused test if practical. Fix this →
• Blocks merge: yes if active races remain unremediated or unproven safe

[HIGH] No regression test for checkpoint-before-close behavior

• Category: Test coverage
• Files: gitnexus/test/integration/lbug-core-adapter.test.ts, gitnexus/test/integration/lbug-lock-retry.test.ts, gitnexus/test/unit/lbug-embedding-hashes.test.ts (none modified)
• Issue: The PR adds production code in three locations that change the close lifecycle of LadybugDB — a data-integrity-sensitive path. No test was added. No existing test asserts: (a) that conn.query('CHECKPOINT') is called before conn.close(), (b) that a failed CHECKPOINT still allows close to proceed, (c) that the close path correctly resets globals after a checkpoint failure, or (d) that write→close→reopen recovers all written data. A future refactor can silently remove or reorder CHECKPOINT without breaking any test.
• Why it matters here: DoD.md §2.7 requires: "Tests cover the real changed path — they would fail if behavior, wiring, or contracts were broken." DoD.md §6 Not-Done Signals: "The runtime path is not actually exercised by the tests." This is a data-integrity fix for which CI green proves nothing about the correctness of the WAL flush behavior.
• Recommended fix: Add at minimum: a unit test mocking conn to assert query('CHECKPOINT') is called before close() in closeLbug(), doInitLbug(), and the withLbugDb() stale path; a failure-branch test asserting close still runs when CHECKPOINT throws; an integration test that writes embeddings, calls closeLbug(), reopens the DB, and reads back all hashes. Fix this →
• Blocks merge: yes per DoD.md decision rule and this PR's data-integrity classification

[MEDIUM] CHECKPOINT errors are swallowed silently — no diagnostic logging

• Category: Error handling / observability
• Files: gitnexus/src/core/lbug/lbug-adapter.ts:264–266, 309–311, 1078–1080
• Issue: All three CHECKPOINT catch blocks swallow errors with /* best-effort */ or /* ignore — older LadybugDB... */ and no logging. The closeLbug() comment says "ignore — older LadybugDB or schemaless DB may not accept it," but if CHECKPOINT fails for a real durability reason (disk full, WAL already corrupt, internal DB error), there is no diagnostic trace anywhere. This is the exact class of failure the PR is meant to prevent — swallowing its error signal makes future corruption silently undiagnosable.
• Why it matters here: DoD.md §2.8 explicitly: "no silent catches that swallow diagnostics." GUARDRAILS.md's "Embeddings vanished after analyze" sign already notes that a Warning: could not load cached embeddings line is intentionally emitted for cache restore failures. The same visibility principle applies to CHECKPOINT failures during close. The three catch blocks have inconsistent comments (/* best-effort */ vs /* ignore — older LadybugDB... */), making intent unclear.
• Recommended fix: Log checkpoint failures at debug or warn level (e.g., console.warn('[lbug] CHECKPOINT failed on close:', err.message)) so operators have diagnostic evidence. Keep close in a finally block to remain robust. Distinguish known unsupported errors from unexpected failures if feasible. Extract a small safeCheckpoint(conn, label) helper to unify the comment and logging in one place. Fix this →
• Blocks merge: yes if silent failure hides realistic corruption signals; softens to no if caller logging or observability elsewhere compensates

[MEDIUM] Duplicated checkpoint logic — three identical try/catch blocks

• Category: Code design / maintainability
• Files: gitnexus/src/core/lbug/lbug-adapter.ts:260–266, 305–311, 1075–1081
• Issue: The same best-effort CHECKPOINT try/catch block is written three times with slightly inconsistent comments (/* best-effort */ vs /* ignore — older LadybugDB or schemaless DB may not accept it */). When the lifecycle is next modified (retry count, error handling strategy, LadybugDB version checks), one copy is likely to be updated while another is missed.
• Why it matters here: DoD.md §2.3: "No copy-paste duplication created for convenience." This is exactly copy-paste lifecycle logic in a data-integrity-sensitive area.
• Recommended fix: Extract async function safeCheckpoint(conn: lbug.Connection | null, reason: string): Promise<void> that encapsulates the null check, try/catch, and logging. Call it from all three close paths. Fix this →
• Blocks merge: no, but recommended as part of any changes addressing the above findings

[LOW] Em dashes in comments — explains GitHub Unicode warning

• Category: File hygiene
• Files: gitnexus/src/core/lbug/lbug-adapter.ts (multiple lines)
• Issue: Non-ASCII em dash characters (—, U+2014) are present throughout the file in comments. GitHub surfaces a hidden Unicode warning for these. No bidi control characters (U+202A–U+202E, U+2066–U+2069) are present. The em dashes are entirely in comments and do not affect executable code or SQL strings.
• Why it matters here: Low risk. The GitHub warning is cosmetic for this file. The em dash style pre-exists this PR throughout the codebase and appears in existing lines, not introduced by the PR's added lines.
• Recommended fix: No action required if repo style allows em dashes in comments. The added lines (260–283, 305–324, 1067–1098) use only ASCII comment punctuation.
• Blocks merge: no

[LOW] PR body contains no reproduction, motivation, or test evidence

• Category: Documentation / process
• Files: PR description
• Issue: The PR body is the default template with all section content blank. It does not document: the failure mode triggering this fix, the LadybugDB version or sidecar behavior, commands run to validate the fix, manual stress test or write-close-reopen result, platform tested on, or limitations of best-effort checkpoint.
• Why it matters here: DoD.md §2.9: "Residual risks, compatibility impacts, and operational concerns are either resolved or clearly stated in the PR description." Without reproduction evidence, reviewers cannot assess whether the fix addresses the actual failure mode.
• Recommended fix: Update PR body with failure scenario, why CHECKPOINT is the right fix, commands run, and what was validated.
• Blocks merge: no if code and tests are otherwise complete

---

Definition of Done check

DoD item	Status
Every LadybugDB close path performs checkpoint before close	✅ satisfied — all three close paths modified
CHECKPOINT happens under correct session/connection lock	⚠️ partially satisfied — withLbugDb stale path and doInitLbug are inside session lock; closeLbug is not
CHECKPOINT does not run concurrently with active writes	⚠️ partially satisfied — session-locked paths are safe; closeLbug is not guaranteed safe
closeLbug() preserves global state cleanup semantics	✅ satisfied — null assignments and flag resets are preserved after both checkpoint and close
CHECKPOINT failure handling is explicit and justified	❌ not satisfied — failures are silently swallowed; DoD.md §2.8 violated
Best-effort swallowing does not hide actionable signals	❌ not satisfied — no logging on any failure path
At least one focused regression test covers checkpoint-before-close	❌ not satisfied — zero tests added
Manual reproduction or stress test documented	❌ not satisfied — PR body is empty
CI, typecheck, lint/format, focused lbug tests green	⚠️ partially satisfied — CI in progress; no new focused tests to be green
No hidden Unicode bidi controls	✅ satisfied — em dashes in comments only, no executable impact
Diff is focused and contains only the intended change	✅ satisfied — no unrelated churn

---

LadybugDB lifecycle assessment

• withLbugDb(): CHECKPOINT is inside an explicit runWithSessionLock — correct. The stale cleanup in the busy retry path also acquires the session lock before checkpointing and closing. State resets (conn = null, db = null, etc.) are correctly inside the same lock.
• ensureLbugInitialized(): Not directly modified; delegates to doInitLbug() which now includes CHECKPOINT on DB switch.
• doInitLbug(): Called only from ensureLbugInitialized(), which is itself called only from within runWithSessionLock contexts. CHECKPOINT here is effectively session-locked. State cleanup (null assignments, flag resets) is correct.
• closeLbug(): Not session-locked. The checkpoint and close run on the global conn without acquiring the session lock. If concurrent withLbugDb operations are active, this can interfere. The double if (conn) check (one for CHECKPOINT, one for close) is safe within a single-call scope but not safe against concurrent callers.
• Stale connection cleanup: Handled in withLbugDb stale retry path — correctly session-locked.
• Database path switch: Handled in doInitLbug() — effectively session-locked via call chain.
• Global conn / db state: Reset correctly in all paths — conn = null; db = null; currentDbPath = null; ftsLoaded = false; vectorExtensionLoaded = false; ensuredFTSIndexes.clear() — even after checkpoint/close failures.
• Close ordering: CHECKPOINT → conn.close() → db.close() — correct in all three paths.
• Missed close paths: closeLbugConnection(tempHandle) at lines 709, 787, 1171 uses the pool adapter's closeLbugConnection (from lbug-config.ts), not the module-level close. These do not use the new CHECKPOINT logic. Whether these paths also need checkpointing depends on whether they follow embedding writes, which cannot be determined from the diff alone.

---

WAL / CHECKPOINT assessment

• Why checkpoint is needed: LadybugDB 0.16.0 has a non-blocking internal checkpoint thread. If the process closes the DB before the thread completes, sidecar pages remain pending, causing WAL replay races or database-id mismatches on next open. This is especially likely after embedding writes which generate significant WAL data.
• Whether CHECKPOINT is supported: @ladybugdb/core ^0.16.0 — no explicit documentation of CHECKPOINT SQL command in the repo. The catch block comment claims "older LadybugDB or schemaless DB may not accept it," suggesting it's expected to work on 0.16.0 but is treated as optional. No empirical test proves this assumption.
• Checkpoint-before-close ordering: Correct in all three paths — CHECKPOINT is awaited before conn.close() and db.close().
• Unsupported checkpoint behavior: Caught and silently ignored. Safe for close semantics (close still runs in all paths) but provides no visibility.
• Checkpoint failure handling: Silent swallowing — see Finding 3 above.
• WAL/shadow sidecar flushing: The intent is correct — CHECKPOINT should force WAL contents into the main DB file. Effectiveness depends on LadybugDB's implementation of CHECKPOINT SQL.
• Write-close-reopen behavior: No integration test validates this. The key scenario (write many embeddings → closeLbug() → reopen → fetchExistingEmbeddingHashes → assert all rows present) is unverified.
• Cross-platform: The test file lbug-core-adapter.test.ts notes a known Windows regression (file lock not released until process exit on Windows with LadybugDB 0.16.0). CHECKPOINT behavior on Windows under this constraint is untested.

---

Concurrency / locking assessment

• Session lock coverage: runWithSessionLock serializes withLbugDb and initLbug calls correctly. The busy retry path is session-locked. doInitLbug is always called inside a lock via ensureLbugInitialized.
• Active writes: Protected by session lock for all withLbugDb-driven writes.
• Active reads: Protected by session lock for all withLbugDb-driven reads.
• Close during operation: closeLbug() can still run concurrently with a withLbugDb operation — this is a pre-existing issue that the PR does not fix and slightly extends (the added CHECKPOINT query is a new async step in the racy path).
• DB switch during operation: Protected — doInitLbug (which closes old DB) is inside session lock.
• Teardown/shutdown: Shutdown handlers call closeLbug() directly. In the API server, jobManager.dispose() and embedJobManager.dispose() are called first; assuming they drain in-flight operations, the shutdown path is safe in practice.
• Deadlock risk: No deadlock risk introduced — runWithSessionLock is a promise chain without mutex-style recursion issues.
• Race risk after this PR: closeLbug() race risk unchanged in category (pre-existing), slightly increased in surface area (additional async step). The session lock approach for withLbugDb and doInitLbug is correct and safe.

---

Error handling assessment

• Checkpoint failure: Caught and silently swallowed in all three paths.
• Close failure: Also caught and swallowed in all three paths (pre-existing behavior preserved).
• Both failure: State reset (conn = null, etc.) always executes because it's outside the try/catch blocks — correct behavior.
• Logging: None. Silent everywhere.
• User-visible behavior: Close proceeds regardless — robust. But any WAL corruption signal is lost.
• Diagnostic value: Effectively zero. If the CHECKPOINT fails for a real durability reason, operators get no evidence. GUARDRAILS.md already shows the pattern of emitting observable warnings for embedding cache failures.
• Whether swallowing is acceptable: Not acceptable without at minimum a debug/warn log — DoD.md §2.8 is explicit on this.

---

Embeddings durability assessment

• Embedding write path: Embedding writes go through conn.query() calls on the module-level connection inside withLbugDb. These are correctly session-locked.
• Hash/vector persistence: fetchExistingEmbeddingHashes (in lbug-adapter.ts) reads from the EMBEDDING_TABLE_NAME table. If CHECKPOINT flushes WAL correctly before close, these should survive reopen. Not tested.
• fetchExistingEmbeddingHashes: Not changed by this PR. Semantics preserved.
• Local backend open/close: mcp/local/local-backend.ts calls closeLbug() and initLbug(). These now have CHECKPOINT in the close path. The local backend calls closeLbug() outside the session lock (same issue as above).
• Large write batches: CHECKPOINT after large embedding writes can take significant wall time. No benchmark exists. The comment "CHECKPOINT is a no-op when there's nothing pending" in closeLbug is accurate for the happy path but understates the cost after large write batches.
• Close/reopen regression: No integration test covers this scenario.
• Corruption reproduction evidence: Not provided. The PR description has no failure scenario, no steps to reproduce, and no validation commands.

---

Performance assessment

• Checkpoint frequency: Correct — CHECKPOINT runs only on close/switch/stale cleanup, never inside hot embedding batch loops.
• Close latency: May increase significantly after large embedding writes when the WAL contains substantial pending data. This is unavoidable for correctness but is not measured or bounded.
• Large WAL flush cost: Claimed to be "cheap on the happy path" (no pending data). True when WAL is empty; potentially not cheap after a large ingestion run.
• Hot-path impact: No impact on embedding write throughput — CHECKPOINT is not in the write path.
• Server shutdown impact: Shutdown will now CHECKPOINT before close — acceptable for correctness.
• Whether "cheap on happy path" is justified: Partially. The comment is accurate for the common case (queries with no pending WAL), but could be misleading after embedding-heavy analyze runs.

---

Hidden Unicode / file hygiene assessment

• Changed file inspected: gitnexus/src/core/lbug/lbug-adapter.ts — fully inspected
• Bidi/invisible controls: grep -P "[\x{202A}-\x{202E}\x{2066}-\x{2069}]" — no hits
• Non-ASCII punctuation: Em dashes (—, U+2014) present throughout existing lines in comments. Present in pre-existing code, not introduced by this PR's added lines.
• Executable code impact: None — all non-ASCII characters are in comments or comment-adjacent positions
• Whether cleanup is needed: No action required from this PR. Em dashes are a pre-existing style choice.

---

Test assessment

• Checkpoint-before-close tests: None
• Call-order tests: None (no mock asserting CHECKPOINT before close)
• Failure-branch tests: None (no test that CHECKPOINT throw still allows close)
• Integration write-close-reopen test: None
• Concurrency test: None
• Platform coverage: Windows behavior under LadybugDB 0.16.0 file-lock constraints is untested for CHECKPOINT
• Whether existing tests would fail before this PR: No — existing tests don't exercise CHECKPOINT at all. Adding CHECKPOINT cannot cause existing tests to fail, but removing it cannot either. This means the change has no test coverage and no regression protection.

---

Validation assessment

• Latest CI: In progress (CI workflow, Docker Build); earlier checks pass
• Typecheck: Not yet confirmed (CI in progress)
• Lint/format: Not yet confirmed
• Focused lbug tests: Existing tests don't cover CHECKPOINT behavior
• Focused embedding tests: Not covering close lifecycle
• Integration tests: Not added
• Full test suite: CI in progress
• Manual stress/reproduction test: Not documented anywhere
• Not verified: CHECKPOINT SQL support on LadybugDB 0.16.0, write-close-reopen data recovery, closeLbugConnection paths (pool adapter, temp handles) don't get CHECKPOINT

---

Final verdict

Not production-ready

Three blockers prevent merge under the repo's own Definition of Done:

1. Zero test coverage for the changed behavior. DoD.md §2.7 and §6 are explicit: a change whose runtime path is not exercised by tests is Not Done, regardless of CI status. A future refactor can silently break checkpoint ordering without any test failure.

2. closeLbug() runs CHECKPOINT outside the session lock, which adds a new async query step on the global conn in a path that can already race with concurrent withLbugDb() operations. While the underlying race is pre-existing, this PR widens the window and adds a new failure mode. The fix requires either session-locking closeLbug() or proving all callers guarantee quiescence.

3. Silent error swallowing violates DoD.md §2.8 (Observability). CHECKPOINT failures — which represent exactly the class of failure this PR aims to prevent — produce no log output anywhere. If CHECKPOINT fails for a real durability reason, operators lose all diagnostic evidence. The repo already has precedent for emitting observable warnings on embedding cache failures (GUARDRAILS.md "Embeddings vanished after analyze").

The fix direction is correct — CHECKPOINT before close is the right approach for WAL durability — but the implementation is incomplete: it lacks tests, has a session-lock gap in closeLbug(), swallows the exact error signal it's meant to prevent, and leaves three copies of the same logic that will drift.

[github-actions]

CI Report

✅ All checks passed

Pipeline Status

Stage	Status	Details
✅ Typecheck	success	tsc --noEmit
✅ Tests	success	unit tests, 3 platforms
✅ E2E	success	gitnexus-web changes only

Test Results

Tests	Passed	Failed	Skipped	Duration
7892	7891	0	1	373s

✅ All 7891 tests passed

1 test(s) skipped — expand for details

• buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric	Coverage	Covered	Base	Delta	Status
Statements	77.38%	23830/30795	77.02%	📈 +0.4	🟢 ███████████████░░░░░
Branches	66.17%	15291/23108	65.97%	📈 +0.2	🟢 █████████████░░░░░░░
Functions	82.51%	2355/2854	81.86%	📈 +0.7	🟢 ████████████████░░░░
Lines	80.34%	21495/26755	79.89%	📈 +0.5	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[magyargergo]

Thank you for this PR!