diff --git a/GUARDRAILS.md b/GUARDRAILS.md index 6b2f58cb2..aa3f70cf7 100644 --- a/GUARDRAILS.md +++ b/GUARDRAILS.md @@ -61,7 +61,7 @@ Format: **Trigger → Instruction → Reason**. Append new Signs when the same m - **Trigger:** Errors opening `.gitnexus/lbug` while MCP and analyze both run. - **Do:** Stop overlapping processes (one writer at a time). Retry analyze or restart MCP. -- **Why:** Embedded DB expects single-process ownership. Known gap: as of `@ladybugdb/core` 0.18.0, one contention error — `"Only one write transaction at a time is allowed in the system."` — isn't recognized by our busy/lock retry matcher (`isDbBusyError` in `src/core/lbug/lbug-config.ts`), so it surfaces as a raw failure instead of a retried one. If you see that exact message, it's the same "one writer at a time" issue above, not a new failure mode. +- **Why:** Embedded DB expects single-process ownership. `@ladybugdb/core` 0.18.0 also reports this contention as `"Only one write transaction at a time is allowed in the system."` — our busy/lock retry matcher (`isDbBusyError` in `src/core/lbug/lbug-config.ts`) recognizes this exact string too, so it's auto-retried the same as any other lock error. If you see that exact message, it's the same "one writer at a time" issue above, not a new failure mode. --- diff --git a/RUNBOOK.md b/RUNBOOK.md index 53d4ccd21..c1a1b3b8d 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -154,7 +154,7 @@ Analyze re-execs Node with a **large old-space heap** when needed (`analyze.ts`) Only one process should open a repo's `.gitnexus/lbug` store at a time. If MCP and a second `analyze` run conflict, stop one process, then retry `analyze` or restart MCP. -If the error text is `"Only one write transaction at a time is allowed in the system."` instead of a lock/busy message, it's the same underlying conflict — our retry matcher doesn't currently recognize that exact string (see `isDbBusyError` in `src/core/lbug/lbug-config.ts`), so it isn't auto-retried. The fix is the same: stop the overlapping process. +If the error text is `"Only one write transaction at a time is allowed in the system."` instead of a lock/busy message, it's the same underlying conflict — our retry matcher (`isDbBusyError` in `src/core/lbug/lbug-config.ts`) recognizes this exact string and auto-retries it. The fix if it still surfaces after retries is the same: stop the overlapping process. --- diff --git a/gitnexus/src/core/lbug/lbug-config.ts b/gitnexus/src/core/lbug/lbug-config.ts index 263f6981d..02c2ba66d 100644 --- a/gitnexus/src/core/lbug/lbug-config.ts +++ b/gitnexus/src/core/lbug/lbug-config.ts @@ -368,10 +368,13 @@ export interface LbugConnectionHandle { } /** - * Return true when the error message indicates that a LadybugDB file lock - * could not be acquired — either at construction time - * (`new lbug.Database(...)` raises from `local_file_system.cpp`) or during - * a query (another writer holds the exclusive lock). + * Return true when the error message indicates that a LadybugDB write + * transaction could not proceed due to lock contention — either a file + * lock that could not be acquired (either at construction time, + * `new lbug.Database(...)` raising from `local_file_system.cpp`, or during + * a query, another writer holds the exclusive lock), or a same-process + * write transaction rejected because another write transaction is already + * active on the connection. * * Lives here (not in `lbug-adapter.ts`) so both the construction-time * retry (`openWithLockRetry` in this file) and the query-time retry @@ -383,10 +386,21 @@ export const isDbBusyError = (err: unknown): boolean => { // `lock` already subsumes `could not set lock`; the broader term is kept // because graph-DB transient errors include "deadlock", "lock contention", // and the LadybugDB native module's "could not set lock on file" — all of - // which deserve a retry. If a non-transient lock-shaped error ever - // surfaces (e.g., "lock file missing" during recovery), tighten this - // matcher rather than raising the retry budget. - return msg.includes('busy') || msg.includes('lock') || msg.includes('already in use'); + // which deserve a retry. LadybugDB also reports same-process writer + // contention without the words "busy" or "lock". + // + // "only one write transaction at a time" was observed against LadybugDB + // 0.18.0 (see gitnexus/package.json @ladybugdb/core). + // + // If a non-transient lock-shaped error ever surfaces (e.g., "lock file + // missing" during recovery), tighten this matcher rather than raising the + // retry budget. + return ( + msg.includes('busy') || + msg.includes('lock') || + msg.includes('already in use') || + msg.includes('only one write transaction at a time') + ); }; export function createLbugDatabase( diff --git a/gitnexus/test/integration/lbug-lock-retry.test.ts b/gitnexus/test/integration/lbug-lock-retry.test.ts index b95092e2d..298a4c2a1 100644 --- a/gitnexus/test/integration/lbug-lock-retry.test.ts +++ b/gitnexus/test/integration/lbug-lock-retry.test.ts @@ -14,7 +14,7 @@ import { withTestLbugDB } from '../helpers/test-indexed-db.js'; // Pure-function tests — no DB needed, but grouped here for cohesion // with the retry logic they guard. -import { isDbBusyError } from '../../src/core/lbug/lbug-config.js'; +import { isDbBusyError, openLbugConnection } from '../../src/core/lbug/lbug-config.js'; describe('isDbBusyError', () => { it('returns true for "busy" errors (case-insensitive)', () => { @@ -34,6 +34,15 @@ describe('isDbBusyError', () => { expect(isDbBusyError('already in use')).toBe(true); }); + it('returns true for "only one write transaction at a time" errors', () => { + expect( + isDbBusyError(new Error('Only one write transaction at a time is allowed in the system.')), + ).toBe(true); + expect(isDbBusyError('only one write transaction at a time is allowed in the system.')).toBe( + true, + ); + }); + it('returns true for "could not set lock" errors', () => { expect(isDbBusyError(new Error('Could not set lock on the database file'))).toBe(true); }); @@ -65,6 +74,48 @@ describe('isDbBusyError', () => { }); }); +// ─── openLbugConnection construction-time retry ──────────────────────────── + +// Minimal stub of the `lbug` module surface used by openLbugConnection. +// Duplicated locally (see lbug-open-retry.test.ts's makeStubLbug) rather +// than shared, matching this codebase's existing per-test-file convention. +interface StubModuleControl { + databaseThrows: Array; + databaseCallCount: number; +} + +const makeStubLbug = (control: StubModuleControl) => { + class FakeDatabase { + constructor(_path: string, ..._rest: unknown[]) { + control.databaseCallCount++; + const next = control.databaseThrows.shift(); + if (next instanceof Error) throw next; + } + async close(): Promise {} + } + class FakeConnection { + constructor(_db: FakeDatabase) {} + async close(): Promise {} + } + return { Database: FakeDatabase, Connection: FakeConnection } as any; +}; + +describe('openLbugConnection — write-transaction contention retry', () => { + it('retries on write-transaction contention and succeeds on a later attempt', async () => { + const control: StubModuleControl = { + databaseThrows: [ + new Error('Only one write transaction at a time is allowed in the system.'), + null, + ], + databaseCallCount: 0, + }; + const stub = makeStubLbug(control); + const handle = await openLbugConnection(stub, '/some/path/lbug'); + expect(handle.db).toBeDefined(); + expect(control.databaseCallCount).toBe(2); + }); +}); + // ─── withLbugDb retry integration tests ─────────────────────────────────── withTestLbugDB('lock-retry', (handle) => { @@ -88,6 +139,21 @@ withTestLbugDB('lock-retry', (handle) => { expect(callCount).toBe(2); }); + it('retries on LadybugDB single-writer transaction contention', async () => { + const { withLbugDb } = await import('../../src/core/lbug/lbug-adapter.js'); + let callCount = 0; + const result = await withLbugDb(handle.dbPath, async () => { + callCount++; + if (callCount === 1) { + throw new Error('Only one write transaction at a time is allowed in the system.'); + } + return 'recovered'; + }); + + expect(result).toBe('recovered'); + expect(callCount).toBe(2); + }); + it('propagates non-BUSY errors immediately without retrying', async () => { const { withLbugDb } = await import('../../src/core/lbug/lbug-adapter.js'); let callCount = 0; @@ -111,7 +177,23 @@ withTestLbugDB('lock-retry', (handle) => { }), ).rejects.toThrow('Could not set lock'); - // DB_LOCK_RETRY_ATTEMPTS = 3 (default in the implementation) + // Matches DB_LOCK_RETRY_ATTEMPTS in lbug-adapter.ts. If that budget + // changes, this assertion — not this comment — is the source of truth. + expect(callCount).toBe(3); + }); + + it('throws after max retry attempts on write-transaction contention', async () => { + const { withLbugDb } = await import('../../src/core/lbug/lbug-adapter.js'); + let callCount = 0; + await expect( + withLbugDb(handle.dbPath, async () => { + callCount++; + throw new Error('Only one write transaction at a time is allowed in the system.'); + }), + ).rejects.toThrow('Only one write transaction at a time is allowed in the system.'); + + // Matches DB_LOCK_RETRY_ATTEMPTS in lbug-adapter.ts. If that budget + // changes, this assertion — not this comment — is the source of truth. expect(callCount).toBe(3); }); });