refactor(lbug): extract safeClose helper to consolidate WAL flush

[azizur100389]

Closes #1376

Summary

• The CHECKPOINT try/catch pattern was duplicated in four places: closeLbug(), withLbugDb's retry cleanup, doInitLbug's reconnect path, and the /api/embed handler in api.ts.
• Extract a single safeClose() helper in lbug-adapter.ts and wire all four call sites through it. Naming follows @magyargergo's suggestion in GitNexus 1.6.3: --embeddings fails with VECTOR extension WAL corruption on macOS #1301.

Changes

File	What
gitnexus/src/core/lbug/lbug-adapter.ts	Add safeClose(); replace inline CHECKPOINT in closeLbug, withLbugDb retry, doInitLbug reconnect
gitnexus/src/server/api.ts	Import safeClose; replace inline executeQuery('CHECKPOINT') in embed route
gitnexus/test/unit/lbug-checkpoint.test.ts	New — structural guards for the consolidation (export, delegation, single source)
gitnexus/test/unit/rate-limit.test.ts	Source-grep: embed route uses safeClose, no inline executeQuery('CHECKPOINT')

Test plan

• npx tsc --noEmit — clean
• npx vitest run test/unit/lbug-checkpoint.test.ts test/unit/rate-limit.test.ts test/unit/lbug — 44/44 pass
• npx prettier --check on changed files — clean
• Pre-commit hook (eslint + prettier + typecheck) — clean

[vercel]

@azizur100389 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 4m 47s —— View job

---

PR #1377 Review: refactor(lbug): extract safeClose helper to consolidate WAL flush

Tasks

• Read repo context files (AGENTS.md, CONTRIBUTING.md, project structure)
• Read changed files (lbug-adapter.ts, api.ts, lbug-checkpoint.test.ts, rate-limit.test.ts)
• Run hidden Unicode / bidi checks
• Run git diff to verify full change set vs. parent
• Search for remaining CHECKPOINT patterns (lbug-adapter.ts, api.ts, bridge-db.ts, pool-adapter.ts)
• Assess safeClose implementation
• Assess all four call sites
• Assess failure paths
• Assess tests
• Assess API/embed regression
• Post final review

---

Review bar for this PR

• safeClose() attempts a WAL flush (CHECKPOINT) and swallows CHECKPOINT failure without preventing close.
• conn.close() / db.close() still happen in every full-close path after safeClose().
• closeLbug() delegates to safeClose().
• withLbugDb retry cleanup and doInitLbug reconnect both delegate to safeClose().
• /api/embed uses safeClose() for CHECKPOINT-only flush (connection kept open intentionally).
• No inline duplicated executeQuery('CHECKPOINT') or conn.query('CHECKPOINT') remains in the four targeted production files.
• Tests cover delegation and single-source-of-truth; at least one behavioural test verifies runtime behaviour.
• No bidi/invisible Unicode controls in changed files.

---

Current PR state

Item	Status
Head SHA	79994db7d2d49aa231e1bc61f983edc314dea9d6
Status	OPEN, not draft
Commits	1
Changed files	4 (lbug-adapter.ts +26/-23, api.ts +3/-6, lbug-checkpoint.test.ts +40/-0, rate-limit.test.ts +7/-0)
PR body claims: npx tsc --noEmit clean	Stated clean; no CI artifact available to confirm independently
PR body claims: 44/44 focused LBUG tests	Stated; could not rerun without npm install
Issue #1376 linkage	Present in PR body ("Closes #1376")
Hidden bidi/Unicode controls	None — bidi grep clean, non-ASCII is only em-dashes in comments (consistent with codebase style)
Vercel authorization comment	Irrelevant to this backend refactor
Code matches PR body	Yes — four call sites wired through safeClose(); description accurate

---

Understanding the change

LadybugDB 0.16.0+ uses a non-blocking WAL checkpoint thread. If the connection is closed before that thread drains, sidecar pages remain on disk and a subsequent open can race with WAL replay or fail the database-id check. The fix (#1359) added try { await conn.query('CHECKPOINT') } catch {} before each close. That pattern was then copy-pasted to three more close paths and the embed handler — four copies of identical semantics with no single owner.

This PR extracts a 5-line helper safeClose() that is the single source of truth for that pattern and wires all four call sites through it. The /api/embed handler uses safeClose() for a CHECKPOINT-only flush because the server keeps the connection alive for subsequent queries; the other three paths call safeClose() and then explicitly close conn and db themselves.

---

Findings

[Minor] safeClose name does not match its behaviour

• Category: Naming / Maintainability
• Files: gitnexus/src/core/lbug/lbug-adapter.ts:1066
• Issue: safeClose() does not close anything. It performs a CHECKPOINT/WAL flush and returns. The actual conn.close() and db.close() calls happen in each caller separately. The JSDoc correctly describes it as "Flush the WAL" — but the function name implies a connection is being closed.
• Why it matters: A future developer adding a new close path may see await safeClose() in closeLbug() and assume it handles the close, omitting the explicit conn.close(). Conversely, a developer seeing await safeClose() in /api/embed might wonder why the connection isn't closed afterwards. flushWAL, safeCheckpoint, or persistWAL would be unambiguous.
• Note: The PR body attributes the name to a reviewer's suggestion in GitNexus 1.6.3: --embeddings fails with VECTOR extension WAL corruption on macOS #1301, so this is the reviewer's own choice. Flagging for awareness regardless.
• Recommended fix: Rename to flushWAL or safeCheckpoint and update the three callers in lbug-adapter.ts and one in api.ts. Fix this →
• Blocks merge: No

[Minor] Tests are structural only — no behavioural unit test for safeClose

• Category: Test Quality
• Files: gitnexus/test/unit/lbug-checkpoint.test.ts
• Issue: All four new assertions are source-grep tests that read the TypeScript source as a string and match patterns. They confirm the consolidation contract but cannot catch runtime regressions. There is no test that:
  1. Calls safeClose() with a mock connection and asserts CHECKPOINT was called.
  2. Calls safeClose() after CHECKPOINT throws and asserts no exception propagates.
  3. Calls safeClose() with conn = null and asserts early return without error.
• Why it matters: Source-grep tests would still pass if safeClose() were changed to async () => { throw new Error() }, as long as the export and delegation string patterns match. The function is 5 lines and unlikely to regress, but there is zero runtime proof.
• Recommended fix: Add one vitest unit test in lbug-checkpoint.test.ts:

  it('safeClose swallows CHECKPOINT failure and never throws', async () => {
    const mockConn = { query: vi.fn().mockRejectedValue(new Error('unsupported')) };
    // Inject conn via module mock or setter
    await expect(safeClose()).resolves.toBeUndefined();
    expect(mockConn.query).toHaveBeenCalledWith('CHECKPOINT');
  });

  Because conn is a module-level variable, the test may need vi.spyOn or a test export hook.
• Blocks merge: No (code is trivial and correct; this is a follow-up gap)

[Minor] Residual inline CHECKPOINT pattern in bridge-db.ts (pre-existing, out of scope)

• Category: Duplication
• Files: gitnexus/src/core/group/bridge-db.ts:237-241
• Issue: closeBridgeDb() still has the identical inline CHECKPOINT-before-close pattern:

  try {
    await (handle._conn as lbug.Connection).query('CHECKPOINT');
  } catch { /* ignore */ }

  This is the same semantics that safeClose() was extracted to consolidate.
• Why it matters: bridge-db.ts manages its own connection handle (a BridgeHandle, not the module-level conn singleton in lbug-adapter.ts), so safeClose() cannot be reused directly without refactoring. The duplication is understandable but undocumented. A future WAL semantic change needs to be applied to both.
• Recommended fix: Either extract a parallel safeBridgeClose(handle) helper in bridge-db.ts, or add a comment noting the intentional duplication and its relationship to safeClose() in lbug-adapter.ts. This is out of scope for this PR but worth a follow-up issue.
• Blocks merge: No (pre-existing; out of stated scope)

[Minor] Source-grep in rate-limit.test.ts is narrower than intended

• Category: Test Quality
• Files: gitnexus/test/unit/rate-limit.test.ts
• Issue: The new test guards against executeQuery('CHECKPOINT') in api.ts source (single-quoted). It would miss a direct conn.query('CHECKPOINT') added to api.ts, a double-quoted variant, or a template literal. The test correctly checks for the former pattern (which was the old code), but a broader guard would be more future-proof.
• Recommended fix: Additionally assert not.toMatch(/conn\.query\(['"]CHECKPOINT/)` to catch any direct-query duplication in api.ts.
• Blocks merge: No

---

safeClose implementation assessment

export const safeClose = async (): Promise<void> => {
  if (!conn) return;                         // ✓ null guard — no throw on uninitialized
  try {
    await conn.query('CHECKPOINT');          // ✓ awaited
  } catch {
    /* ignore */                             // ✓ CHECKPOINT failure swallowed
  }
};

• CHECKPOINT attempted and awaited: ✓
• CHECKPOINT failure swallowed: ✓ — correct, CHECKPOINT not supported on older LadybugDB
• Close performed by safeClose: ✗ — intentional design; close is left to each caller. This is the naming concern above.
• Exported: ✓ — api.ts imports it; export is necessary
• Null-guard semantics: The old code used if (conn) { try … } inline. safeClose() uses if (!conn) return. Functionally identical.
• Module-level conn: safeClose() references the module-level singleton conn. This works correctly because all callers are in the same module or are calling through the exported helper post-initialization.
• Circular imports: None introduced. api.ts → lbug-adapter.ts pre-existed.
• Logging: No noisy logs on normal path. ✓
• Readability: Clear and minimal. ✓

The change from executeQuery('CHECKPOINT') (old /api/embed) to conn.query('CHECKPOINT') (new safeClose) skips the result.getAll() call that executeQuery makes after every query. For CHECKPOINT this is harmless — no rows are returned — but it is a behavioral difference worth noting.

---

Call-site assessment

closeLbug() (lbug-adapter.ts:1075)

• Calls await safeClose() then closes conn and db in separate try/catch {} blocks. ✓
• Close errors are still silently swallowed (as before). ✓
• Pattern before/after: semantically identical. ✓

withLbugDb retry cleanup (lbug-adapter.ts:259-278)

• Old: if (conn) { try { await conn.query('CHECKPOINT'); } catch {} }
• New: await safeClose(); — safeClose()'s internal if (!conn) return preserves the null guard. ✓
• conn.close() and db.close() still happen after, with separate try/catch. ✓
• Retry behaviour unchanged. ✓

doInitLbug reconnect path (lbug-adapter.ts:296-313)

• Same pattern as retry cleanup. Old inline guard if (conn) replaced by safeClose()'s internal guard. ✓
• Stale connection still closed. ✓
• Reconnect semantics unchanged. ✓

/api/embed handler (api.ts:1711)

• Old: try { await executeQuery('CHECKPOINT'); } catch {}
• New: await safeClose()
• safeClose() is a more correct replacement: it guards !conn explicitly rather than relying on executeQuery to throw "not initialized" and catching that.
• Connection intentionally stays open for other routes. ✓
• Rate-limiter middleware (createRouteLimiter) still wired. ✓
• Import path: '../core/lbug/lbug-adapter.js' — correct, no new dependency. ✓

---

Failure-path assessment

Scenario	Behaviour
CHECKPOINT succeeds, close succeeds	✓ Normal path — all callers work correctly
CHECKPOINT fails, close succeeds	✓ safeClose() swallows CHECKPOINT error; callers proceed to close
CHECKPOINT succeeds, close fails	✓ Close errors caught by each caller's try {} catch {} (unchanged semantics)
CHECKPOINT fails, close fails	✓ CHECKPOINT error swallowed; close error caught by caller
conn is null when safeClose() called	✓ if (!conn) return — early return, no error
/api/embed throws before cleanup	✓ safeClose() is inside the success block of the embed pipeline; the outer catch at line 1722-1729 handles job failure separately. Cleanup semantics unchanged.
Retry cleanup during failed initialization	✓ safeClose() called inside runWithSessionLock, then conn/db closed. Same as before.

One gap: there is no test for the CHECKPOINT-fails-close-still-runs path. Code inspection confirms correctness, but runtime proof is absent (see Findings §2).

---

Duplication assessment

After this PR, in the four targeted files:

• lbug-adapter.ts: exactly one conn.query('CHECKPOINT') call (inside safeClose). ✓
• api.ts: zero executeQuery('CHECKPOINT') or conn.query('CHECKPOINT') calls. ✓

Remaining duplication:

• bridge-db.ts:238: one inline conn.query('CHECKPOINT') in closeBridgeDb(). Pre-existing, separate connection type, out of PR scope. (See Findings §3.)
• pool-adapter.ts:582: closeLbug in the pool adapter — checked, no inline CHECKPOINT found there.

Source-grep tests guard the lbug-adapter.ts and api.ts files adequately. The bridge-db.ts gap is noted but pre-existing.

---

Test assessment

lbug-checkpoint.test.ts — 3 structural assertions:

1. exports safeClose — regex matches export const safeClose. ✓ Would fail pre-PR.
2. closeLbug delegates to safeClose — slices from export const closeLbug and checks for await safeClose(). ✓ Would fail pre-PR (old code had inline conn.query).
3. safeClose is the only place that issues conn.query(CHECKPOINT) — counts matches with single-quote pattern. ✓ Correctly asserts count === 1. Would fail pre-PR (count was 3).

rate-limit.test.ts — 1 structural assertion added:
4. embed route flushes WAL via safeClose, not inline executeQuery — checks await safeClose() present and executeQuery('CHECKPOINT') absent. ✓ Would fail pre-PR.

Gaps:

• No test exercises safeClose() at runtime. All guards are text-match.
• Tests clean up no spies/mocks (none created — pure source reads). ✓
• No hidden Unicode in test source or string literals. ✓
• Existing lbug-embedding-hashes.test.ts, lbug-extension-loader.test.ts, lbug-readonly-error.test.ts are unaffected — correct.

---

API/embed regression assessment

• The embed route (api.ts:1627–1741) structure is unchanged. The only change is replacing 5 lines of inline CHECKPOINT try/catch with await safeClose().
• Response status codes, job management, timeout, SSE progress, and cancel routes are untouched.
• Cleanup runs in the same position (inside the success callback of the async embed pipeline, line 1711).
• Error paths at lines 1722-1729 and 1739-1740 are unchanged.
• The shared-repo lock at line 1635 is unchanged.
• New import (safeClose) added to the existing import block from lbug-adapter.js. No new module boundary. ✓
• Server build: safeClose is exported from lbug-adapter.ts as a named export. TypeScript would catch any mismatch. PR body states npx tsc --noEmit is clean. ✓

---

Hidden Unicode / hygiene assessment

• Bidi controls (U+202A–U+202E, U+2066–U+2069): None found in any of the four changed files. ✓
• Non-ASCII characters: Em-dashes (—, U+2014) found in comments in lbug-adapter.ts. These are consistent with existing codebase style (same character appears in pre-existing comments throughout the file). Not a concern.
• No invisible whitespace, null bytes, or control characters in executable code or string literals. ✓
• GitHub's "hidden Unicode" warnings are likely triggered by the em-dashes in the new docstring. These are benign.

---

Elegance / maintainability assessment

• The helper is 5 lines, has no parameters, and does exactly one thing. ✓
• The docstring is accurate ("Flush the WAL") and references the PR number. ✓
• The function name safeClose is the only friction point — it describes "the safe part of closing" rather than the whole operation, and does not close. For a reader looking at /api/embed, where the connection stays open, safeClose is confusing without reading the implementation.
• No unrelated refactoring was introduced. No API behaviour was changed. ✓
• Future close paths will naturally reach for safeClose() — discovery is straightforward since it lives next to closeLbug. ✓
• The four source-grep tests are proportionate — they guard consolidation without over-constraining formatting.

---

Final verdict

production-ready with minor follow-ups

All four intended call sites correctly delegate to safeClose(). CHECKPOINT failure is swallowed in all paths, close still occurs in every full-close path, and the /api/embed handler correctly does CHECKPOINT-only. Close errors were silently swallowed before this PR and remain so — no behavioral regression. The change from executeQuery('CHECKPOINT') to conn.query('CHECKPOINT') in the embed path is semantically equivalent for CHECKPOINT.

The two follow-ups worth tracking: (1) rename safeClose to something that doesn't imply closing a connection (e.g. flushWAL) — the naming creates a correctness illusion for future callers; (2) add one behavioral vitest unit test that proves CHECKPOINT failure is swallowed at runtime rather than only in source text. Neither blocks merge.

---

[magyargergo]

@azizur100389 Could you please look into why is that test failed only on windows? 🙏

 FAIL   lbug-db  test/integration/local-backend-calltool.test.ts > withTestLbugDB(local-backend-calltool) > callTool dispatch with real DB > query tool returns results for keyword search
AssertionError: expected [] to include 'proc:login-flow'
 ❯ test/integration/local-backend-calltool.test.ts:101:56
     99|         expect(result).toHaveProperty('processes');
    100|         expect(result).toHaveProperty('definitions');
    101|         expect(result.processes.map((p: any) => p.id)).toContain('proc…
       |                                                        ^
    102|         expect(result.process_symbols.map((s: any) => s.id)).toContain…
    103|

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯


 Test Files  1 failed | 271 passed (272)
      Tests  1 failed | 7850 passed | 11 skipped (7862)

[magyargergo]

Apperently cannot leave a comment in the code. 🤷

[azizur100389]

Good question — they serve different purposes here:

• Line 122 (safeClose): mid-setup flush — we need the WAL checkpointed so the pool adapter sees the FTS indexes, but the connection must stay open for subsequent queries.
• Cleanup (closeLbug): end-of-test teardown — we need the full close (CHECKPOINT + conn.close + db.close + state reset) to release native resources. closeLbug already delegates to safeClose internally for the CHECKPOINT step, so the flush is covered.

Replacing closeLbug with safeClose in cleanup would leave the connection and DB handle open, leaking native resources between test files.

[magyargergo]

Good question — they serve different purposes here:

• Line 122 (safeClose): mid-setup flush — we need the WAL checkpointed so the pool adapter sees the FTS indexes, but the connection must stay open for subsequent queries.
• Cleanup (closeLbug): end-of-test teardown — we need the full close (CHECKPOINT + conn.close + db.close + state reset) to release native resources. closeLbug already delegates to safeClose internally for the CHECKPOINT step, so the flush is covered.

Replacing closeLbug with safeClose in cleanup would leave the connection and DB handle open, leaking native resources between test files.

Sorry I thought we moved the conn.close as well. That's why I suggested the consolidation.

---

@azizur100389 could you please write a close function that includes checkpoint query and the connection close? Also write a custom lint rule to prevent calling the lbug close directly. 🙏

[github-actions]

Claude finished @magyargergo's task in 4m 0s —— View job

---

PR #1377 Review: refactor(lbug): extract safeClose helper to consolidate WAL flush

Tasks

• Read repo context files (AGENTS.md, CONTRIBUTING.md, project structure)
• Read changed files (lbug-adapter.ts, api.ts, lbug-checkpoint.test.ts, rate-limit.test.ts, test-indexed-db.ts, eslint.config.mjs)
• Check CI status and latest commits
• Run bidi/Unicode checks on changed files
• Search for remaining CHECKPOINT patterns
• Assess safeClose / flushWAL implementations
• Assess all call sites
• Assess ESLint rule for preventing direct lbug close
• Assess failure paths
• Assess tests (structural + behavioural)
• Assess API/embed regression
• Post final review

---

Review bar for this PR

• flushWAL() performs CHECKPOINT only — connection stays open — CHECKPOINT failure swallowed.
• safeClose() calls flushWAL() then conn.close() + db.close(), each in their own try/catch; nulls handles after each.
• closeLbug() delegates to safeClose() and resets module-level state; no inline close logic.
• withLbugDb retry cleanup and doInitLbug reconnect both delegate to safeClose().
• /api/embed uses flushWAL() (connection stays open); no inline executeQuery('CHECKPOINT') remains.
• ESLint no-restricted-syntax rule prevents new direct conn.close() / db.close() calls in lbug-adapter.ts.
• At least one behavioural test exercises the WAL helper at runtime (not only source-grep).
• No bidi/invisible Unicode controls in changed files; CI green across all 5 workflows.

---

Current PR state

Item	Status
Head SHA	839cce8
Status	OPEN, not draft
Commits on branch	4 (79994db → d42d97a → 2bb8090 → 839cce8)
Changed files	6 (lbug-adapter.ts, api.ts, lbug-checkpoint.test.ts, rate-limit.test.ts, test-indexed-db.ts, eslint.config.mjs)
CI	✅ 5/5 green (CI, Docker Build, Dependency Review, Gitleaks, CodeQL)
TypeCheck	Stated clean in PR body; not independently re-runnable without install
Focused LBUG tests	Stated 44/44; CI passes
Issue #1376 linkage	Present ("Closes #1376")
Hidden bidi/Unicode	None — clean; em-dashes in comments are pre-existing codebase style
Vercel authorization	Irrelevant to this backend refactor
Code vs PR body	PR body describes 79994db (original commit); code has evolved 3 more commits per reviewer feedback. The latest state is correct and supersedes the PR body

Note on PR evolution: This branch has grown significantly since the first Claude review at 79994db. Three follow-up commits addressed reviewer feedback: added runtime behavioural tests, fixed a Windows CI WAL flush race in the test helper, and — most importantly — expanded safeClose to actually perform the connection close, resolving the naming concern from the first review. The PR body should be updated to reflect the current state, but this is cosmetic.

---

Understanding of the change

LadybugDB 0.16.0+ uses a non-blocking WAL checkpoint thread. If the connection is closed before that thread drains, sidecar pages remain on disk; a subsequent open can race with WAL replay or fail the database-id check. The fix added try { await conn.query('CHECKPOINT') } catch {} before close — then that pattern was copy-pasted to four locations with no single owner.

This PR extracts two helpers:

• flushWAL() — CHECKPOINT only, for paths where the connection must stay open (e.g. /api/embed which serves subsequent queries on the same connection).
• safeClose() — flushWAL + conn.close + db.close, for paths that fully release the connection.
• closeLbug() — delegates to safeClose() + resets module-level state (full teardown).

An ESLint no-restricted-syntax rule in eslint.config.mjs enforces that direct conn.close() / db.close() calls cannot be added to lbug-adapter.ts outside the sole authorised site in safeClose.

---

Findings

[Minor] Behavioural tests only exercise the null-conn fast path

• Category: Test Quality
• Files: gitnexus/test/unit/lbug-checkpoint.test.ts
• Issue: The two runtime tests call flushWAL() at module load when conn is null. They confirm the null-guard returns without throwing, which is valuable. However, no test mocks an active connection to verify: (a) CHECKPOINT is actually called, (b) a CHECKPOINT failure does not propagate, or (c) safeClose closes conn and db after a failed CHECKPOINT.
• Why it matters: Code inspection confirms correctness, but a mock-based unit test would catch a future refactor that accidentally re-raises CHECKPOINT errors or omits the close step.
• Recommended fix (non-blocking): Add one vi.spyOn test in a follow-up:

  it('flushWAL swallows CHECKPOINT failure', async () => {
    const mockConn = { query: vi.fn().mockRejectedValue(new Error('unsupported')) };
    // inject via module-level setter or vi.spyOn module
    await expect(flushWAL()).resolves.toBeUndefined();
  });

• Blocks merge: No — code is trivially correct and the structural guards plus CI provide adequate confidence.

[Minor] bridge-db.ts retains inline CHECKPOINT pattern (pre-existing, out of scope)

• Category: Duplication
• Files: gitnexus/src/core/group/bridge-db.ts:237-241
• Issue: closeBridgeDb() still has the identical inline pattern:

  try {
    await (handle._conn as lbug.Connection).query('CHECKPOINT');
  } catch { /* ignore */ }

  This is the same semantics that flushWAL was extracted to consolidate.
• Why it matters: bridge-db.ts manages a BridgeHandle (not the module-level singleton conn), so flushWAL cannot be reused without additional plumbing. The ESLint rule deliberately covers only lbug-adapter.ts. A future WAL semantic change needs to be applied to both.
• Recommended fix: Follow-up issue. A comment noting the intentional duplication would help.
• Blocks merge: No (pre-existing; explicitly out of PR scope).

[Minor] rate-limit.test.ts source-grep is narrower than the risk it guards

• Category: Test Quality
• Files: gitnexus/test/unit/rate-limit.test.ts
• Issue: The test asserts not.toMatch(/executeQuery\('CHECKPOINT'\)/) in api.ts source. It would miss a direct conn.query('CHECKPOINT') call in api.ts, a double-quoted variant, or a template literal. Manual inspection confirms no such pattern exists, but the guard is not future-proof.
• Recommended fix: Add expect(apiSource).not.toMatch(/conn\.query\(['"\]CHECKPOINT/)`.
• Blocks merge: No.

---

flushWAL / safeClose implementation assessment

export const flushWAL = async (): Promise<void> => {
  if (!conn) return;                         // ✓ null guard
  try {
    await conn.query('CHECKPOINT');          // ✓ awaited; single CHECKPOINT call in adapter
  } catch {
    /* ignore */                             // ✓ failure swallowed correctly
  }
};

export const safeClose = async (): Promise<void> => {
  await flushWAL();                          // ✓ delegates; CHECKPOINT failure already swallowed
  if (conn) {
    try {
      // eslint-disable-next-line no-restricted-syntax -- sole authorised close site
      await conn.close();                    // ✓ awaited
    } catch { /* best-effort */ }           // ✓ close errors were silently swallowed before this PR too
    conn = null;                             // ✓ handle nulled regardless of close success
  }
  if (db) {
    try {
      // eslint-disable-next-line no-restricted-syntax -- sole authorised close site
      await db.close();                      // ✓ awaited
    } catch { /* best-effort */ }
    db = null;                               // ✓ handle nulled
  }
};

• CHECKPOINT attempted and awaited: ✓ (single source; structural test guards count === 1)
• CHECKPOINT failure swallowed: ✓ correct
• close attempted after CHECKPOINT failure: ✓ — flushWAL swallows before returning; if (conn) proceeds
• close awaited: ✓
• close errors swallowed: ✓ — same semantics as pre-PR code (verified by reading closeLbug history)
• handles nulled after close attempt: ✓ — happens inside the if (conn) / if (db) block regardless of close success
• Double-close safety: ✓ — if (conn) / if (db) guards prevent double-close; null after first call
• Exported: ✓ — flushWAL imported by api.ts; safeClose used internally and by callers
• Circular imports: None introduced
• ESLint override: eslint-disable-next-line no-restricted-syntax at the two call sites inside safeClose — correctly scoped to the sole authorised site

The naming is now semantically accurate: flushWAL does a flush, safeClose does close. This resolves the primary naming concern from the first review.

---

Call-site assessment

closeLbug() (lbug-adapter.ts:1086)

• await safeClose() — CHECKPOINT + conn.close + db.close. ✓
• Module state (currentDbPath, ftsLoaded, vectorExtensionLoaded, ensuredFTSIndexes) reset after safeClose. ✓
• No inline CHECKPOINT. ✓
• No inline conn.close() / db.close(). ✓

withLbugDb retry cleanup (lbug-adapter.ts:259-264)

• await safeClose() inside runWithSessionLock. ✓
• Module state reset after. ✓
• CHECKPOINT failure does not block cleanup. ✓

doInitLbug reconnect path (lbug-adapter.ts:285-291)

• if (conn || db) { await safeClose(); ... } — guard preserved. ✓
• Stale connection closed before new init. ✓
• Module state reset after. ✓

/api/embed handler (api.ts:1711)

• await flushWAL() — CHECKPOINT only, connection stays open for subsequent routes. ✓
• Import: flushWAL from '../core/lbug/lbug-adapter.js' — correct, pre-existing import path. ✓
• No inline executeQuery('CHECKPOINT'). ✓
• safeClose correctly not used here (would close the connection the server needs alive). ✓

---

Failure-path assessment

Scenario	Behaviour
CHECKPOINT succeeds, close succeeds	✓ Normal path
CHECKPOINT fails, close succeeds	✓ flushWAL swallows; safeClose proceeds to close
CHECKPOINT succeeds, close fails	✓ Close error swallowed (best-effort, consistent with pre-PR semantics); handle nulled
CHECKPOINT fails, close fails	✓ Both errors swallowed; handles nulled
conn null at safeClose call	✓ flushWAL returns early; if (conn) false; no-op
/api/embed throws before cleanup	✓ flushWAL inside the success callback of the pipeline; outer catch at lines 1721-1730 handles job failure separately — unchanged
Retry cleanup during failed init	✓ safeClose() inside runWithSessionLock; conn/db nulled; state reset; correct

One gap: no test exercises the "CHECKPOINT fails but close still runs" path at runtime (see Findings §1).

---

Duplication assessment

After this PR in targeted production files:

• lbug-adapter.ts: exactly one conn.query('CHECKPOINT') (inside flushWAL). ✓
• lbug-adapter.ts: exactly one await conn.close() and one await db.close() (both inside safeClose). ✓ (Structural tests guard these counts.)
• api.ts: zero executeQuery('CHECKPOINT') or conn.query('CHECKPOINT'). ✓

Remaining duplication:

• bridge-db.ts:238: one inline conn.query('CHECKPOINT') — pre-existing, separate BridgeHandle type, intentionally out of scope.
• pool-adapter.ts, lbug-config.ts: conn.close() calls for pool/temp handles — different connection lifecycle, not the singleton; ESLint rule correctly scoped to lbug-adapter.ts only.

The source-grep tests (matches.length === 1) cleanly guard the single-source invariant in lbug-adapter.ts.

---

Test assessment

Structural tests (lbug-checkpoint.test.ts) — 6 assertions:

1. exports flushWAL — ✓ regex against source
2. exports safeClose — ✓
3. safeClose delegates to flushWAL — ✓ slices safeClose body, checks await flushWAL()
4. closeLbug delegates to safeClose; no inline conn.close/db.close — ✓ double-checks absence
5. flushWAL is the only conn.query(CHECKPOINT) site — ✓ count === 1
6. conn.close() / db.close() only inside safeClose — ✓ count === 1 each

All would fail pre-PR. ✓

Behavioural tests (lbug-checkpoint.test.ts) — 2 assertions:

• flushWAL() resolves without error when conn is null (module load state). ✓
• flushWAL() is idempotent (can be called twice). ✓

These are runtime tests with a real import — they catch regressions that source-grep misses. The gap is that they only cover the null-conn fast path (see Findings §1).

rate-limit.test.ts — 1 new assertion:

• await flushWAL() present in api.ts source; executeQuery('CHECKPOINT') absent. ✓ Would fail pre-PR.

test-indexed-db.ts — not a test file itself; the flushWAL() call added at line 122 fixes the Windows CI race where FTS index pages weren't checkpointed before the pool adapter started reading. This directly addresses the Windows test failure reported by @magyargergo.

---

API/embed regression assessment

• Embed route cleanup: await flushWAL() replaces the previous try { await executeQuery('CHECKPOINT') } catch {} block.
• flushWAL internally uses conn.query('CHECKPOINT') directly instead of executeQuery. For CHECKPOINT, executeQuery would call result.getAll() after the query (zero rows returned) — conn.query does not. This is semantically equivalent for CHECKPOINT but slightly more efficient. ✓
• Response status codes, job management, SSE progress, cancel routes, and lock release are untouched. ✓
• Error paths at lines 1721-1730 and 1739-1741 are unchanged. ✓
• flushWAL import added to the existing lbug-adapter.js import block — no new module boundary. ✓
• Connection stays open after flushWAL() — correct; safeClose is deliberately not used here. ✓

---

Hidden Unicode / hygiene assessment

• Bidi controls (U+202A–U+202E, U+2066–U+2069): None in any changed file. ✓
• Non-ASCII characters: Em-dashes (—, U+2014) in comments in lbug-adapter.ts — pre-existing codebase style, present throughout the file before this PR. Not a concern.
• No invisible whitespace, null bytes, or control characters in executable code, string literals, or tests. ✓

---

Elegance / maintainability assessment

The final shape is clean and well-layered:

flushWAL()    — CHECKPOINT only (5 lines; use when connection must stay open)
safeClose()   — flushWAL + conn.close + db.close (18 lines; use for full close)
closeLbug()   — safeClose + module state reset (6 lines; use for full teardown)

• Each function does exactly one thing with no overlap. ✓
• JSDoc @see cross-references guide future developers to the right level. ✓
• The ESLint rule makes the invariant machine-enforced, not just convention. ✓
• The eslint-disable-next-line comments at the sole authorised conn.close()/db.close() sites are clearly annotated with "sole authorised close site". ✓
• No unrelated refactoring; API behaviour unchanged. ✓
• The naming concern from the first review ("safeClose does not close anything") is fully resolved. ✓

---

Final verdict

production-ready

All four call sites correctly route through the appropriate helper (flushWAL for CHECKPOINT-only, safeClose for full close). CHECKPOINT failure is swallowed in every path, and close errors preserve pre-PR best-effort semantics. The naming concern from the first review has been resolved — safeClose now actually closes the connection, while flushWAL handles the keep-alive case in /api/embed. The ESLint rule adds enforcement that prevents future regressions, and the Windows FTS race is fixed by the WAL flush in the test helper. The only follow-ups are a mock-based behavioural test for the CHECKPOINT-fails path and a note about bridge-db.ts's pre-existing duplicate — neither blocks shipping.

---