mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-14 00:48:18 +00:00
fix(reindex-frontmatter): reuse the connected engine instead of self-deadlocking on the PGLite lock (#1963) (#3558)
Adversarial review: survived two independent refuters — the only PR of 32 reviewed this way to do so. The bug: `gbrain reindex-frontmatter` and `gbrain backfill <kind>` were 100% dead on PGLite. cli.ts takes the data-dir lock, the command modules built a second engine on the same dir, and acquireLock never reaps a live PID — 30s timeout, exit 1, with the error naming the waiting process itself as the holder. Reproduced on the parent commit at 33.2s; passes in 5.0s with the fix. Root-cause fix at the dispatch layer, not a softening of the lock, and the sibling census confirmed these were the only two affected callers. Postgres path verified before merge (it was the review's one open gap, since the bug is PGLite-only and all verification had gone there while the change itself is connection-teardown ownership). Against real Postgres 16 + pgvector: reindex-frontmatter and all three registered backfills exit 0 with zero residual connections, zero advisory locks, and zero cycle-lock rows — byte-identical output and identical teardown to master on the same database, confirming the change is behavior-neutral there. Merged tree re-verified after rebase: typecheck clean, pglite-lock + reindex-frontmatter 16 pass, llms bundle fresh, 23/23 CI green.
This commit is contained in:
@@ -2,6 +2,21 @@
|
||||
|
||||
All notable changes to GBrain will be documented in this file.
|
||||
|
||||
## [0.42.68.1] - 2026-07-30
|
||||
|
||||
**If you run `gbrain reindex-frontmatter` or `gbrain backfill` on the default embedded database, they now work. Until this release both failed every time, after waiting 30 seconds.**
|
||||
|
||||
The embedded database allows one process at a time, and holds a lock to enforce it. These two commands opened a second connection to the same database from inside the process that already held that lock, then waited for a lock that could never be released — because the thing holding it was the waiting process itself. The wait ran its full 30 seconds and the command exited with an error naming a blocking process that was, in fact, itself. Both commands now reuse the connection that is already open.
|
||||
|
||||
Nothing changes for brains on Postgres, where a second connection was always allowed.
|
||||
|
||||
## To take advantage of v0.42.68.1
|
||||
|
||||
Nothing to undo — the commands failed without writing anything. Just run whichever you needed:
|
||||
```bash
|
||||
gbrain reindex-frontmatter
|
||||
```
|
||||
|
||||
## [0.42.67.0] - 2026-07-28
|
||||
|
||||
**If you develop GBrain on Windows, the test and check commands now actually run. Until this release they were quietly doing almost nothing.**
|
||||
|
||||
File diff suppressed because one or more lines are too long
+1
-1
@@ -147,7 +147,7 @@
|
||||
"bun": ">=1.3.10"
|
||||
},
|
||||
"license": "MIT",
|
||||
"version": "0.42.67.0",
|
||||
"version": "0.42.68.1",
|
||||
"overrides": {
|
||||
"@hono/node-server": "^2.0.5",
|
||||
"fast-uri": "^3.1.4",
|
||||
|
||||
+12
-4
@@ -2253,16 +2253,24 @@ async function handleCliOnly(command: string, args: string[]) {
|
||||
//
|
||||
// v0.30.1: still works; canonical entrypoint is now `gbrain backfill
|
||||
// effective_date`. This command stays as a thin alias for back-compat.
|
||||
//
|
||||
// #1963: pass the already-connected engine. The command used to build
|
||||
// + connect its OWN engine here, which self-deadlocked on the PGLite
|
||||
// data-dir lock (this process already holds it via connectEngine
|
||||
// above) — 30s spin, then exit 1, on every PGLite invocation.
|
||||
const { reindexFrontmatterCli } = await import('./commands/reindex-frontmatter.ts');
|
||||
await reindexFrontmatterCli(args);
|
||||
return; // reindexFrontmatterCli handles its own engine lifecycle
|
||||
await reindexFrontmatterCli(engine, args);
|
||||
break;
|
||||
}
|
||||
case 'backfill': {
|
||||
// v0.30.1: first-class generic backfill command. Subcommand dispatch
|
||||
// is inside runBackfillCommand (kind | list | --help).
|
||||
// #1963: same double-connect class as reindex-frontmatter — reuse the
|
||||
// connected engine instead of building a second one on the same
|
||||
// PGLite data dir.
|
||||
const { runBackfillCommand } = await import('./commands/backfill.ts');
|
||||
await runBackfillCommand(args);
|
||||
return;
|
||||
await runBackfillCommand(engine, args);
|
||||
break;
|
||||
}
|
||||
case 'code-callers': {
|
||||
// v0.20.0 Cathedral II Layer 10 (C4): "who calls <symbol>?"
|
||||
|
||||
@@ -16,10 +16,10 @@
|
||||
* always reserving 1 connection for HNSW + heartbeat + doctor probes.
|
||||
*/
|
||||
|
||||
import type { BrainEngine } from '../core/engine.ts';
|
||||
import { resolveDirectPoolSize } from '../core/connection-manager.ts';
|
||||
import { listBackfills, getBackfill } from '../core/backfill-registry.ts';
|
||||
import { runBackfill, clearBackfillCheckpoint } from '../core/backfill-base.ts';
|
||||
import { loadConfig, toEngineConfig } from '../core/config.ts';
|
||||
|
||||
interface BackfillArgs {
|
||||
kind?: string;
|
||||
@@ -114,7 +114,14 @@ function clampConcurrency(requested: number | undefined): { effective: number; w
|
||||
return { effective: requested };
|
||||
}
|
||||
|
||||
export async function runBackfillCommand(args: string[]): Promise<void> {
|
||||
/**
|
||||
* #1963 (same class as reindex-frontmatter): takes the ALREADY-CONNECTED
|
||||
* engine from cli.ts's dispatch. Building a second engine here deadlocked on
|
||||
* the PGLite data-dir lock (cli.ts's `connectEngine()` already holds it in
|
||||
* this same process) — every `gbrain backfill <kind>` on PGLite timed out
|
||||
* after 30s. Engine lifecycle belongs to cli.ts's connect + teardown.
|
||||
*/
|
||||
export async function runBackfillCommand(engine: BrainEngine, args: string[]): Promise<void> {
|
||||
const cli = parseArgs(args);
|
||||
if (cli.help) { printHelp(); return; }
|
||||
|
||||
@@ -144,20 +151,10 @@ export async function runBackfillCommand(args: string[]): Promise<void> {
|
||||
process.exit(2);
|
||||
}
|
||||
|
||||
const config = loadConfig();
|
||||
if (!config) {
|
||||
console.error('No brain configured. Run: gbrain init');
|
||||
process.exit(2);
|
||||
}
|
||||
|
||||
// X5 admission control — clamp concurrency to direct-pool capacity.
|
||||
const { effective: concurrency, warning } = clampConcurrency(cli.concurrency);
|
||||
if (warning) console.warn(warning);
|
||||
|
||||
const { createEngine } = await import('../core/engine-factory.ts');
|
||||
const engine = await createEngine(toEngineConfig(config));
|
||||
await engine.connect(toEngineConfig(config));
|
||||
|
||||
if (cli.fresh) {
|
||||
await clearBackfillCheckpoint(engine, reg.spec.name);
|
||||
console.log(`Cleared checkpoint for backfill.${reg.spec.name}`);
|
||||
@@ -192,7 +189,6 @@ export async function runBackfillCommand(args: string[]): Promise<void> {
|
||||
if (result.cappedByMaxRows) console.log(` ⚠️ Capped by --max-rows; more remain.`);
|
||||
if (result.cappedByErrors) console.log(` ⚠️ Capped by --max-errors at ${result.errors}.`);
|
||||
|
||||
await engine.disconnect();
|
||||
if (result.cappedByErrors) process.exit(1);
|
||||
}
|
||||
|
||||
|
||||
@@ -151,8 +151,17 @@ export async function runReindexFrontmatter(
|
||||
};
|
||||
}
|
||||
|
||||
/** CLI entrypoint. Argv shape matches reindex-code for consistency. */
|
||||
export async function reindexFrontmatterCli(args: string[]): Promise<void> {
|
||||
/**
|
||||
* CLI entrypoint. Argv shape matches reindex-code for consistency.
|
||||
*
|
||||
* #1963: takes the ALREADY-CONNECTED engine from cli.ts's dispatch instead of
|
||||
* building its own. The old self-managed `createEngine()+connect()` here was a
|
||||
* same-process double-connect: cli.ts's `connectEngine()` already held the
|
||||
* PGLite data-dir lock, so the second `connect()` spun the full 30s lock
|
||||
* timeout waiting on its own process and the command always exited 1 on
|
||||
* PGLite. The engine lifecycle (connect + teardown) belongs to cli.ts.
|
||||
*/
|
||||
export async function reindexFrontmatterCli(engine: BrainEngine, args: string[]): Promise<void> {
|
||||
const opts: ReindexFrontmatterOpts = {};
|
||||
for (let i = 0; i < args.length; i++) {
|
||||
const a = args[i];
|
||||
@@ -173,37 +182,15 @@ export async function reindexFrontmatterCli(args: string[]): Promise<void> {
|
||||
}
|
||||
}
|
||||
|
||||
const { createEngine } = await import('../core/engine-factory.ts');
|
||||
const { loadConfig, toEngineConfig } = await import('../core/config.ts');
|
||||
const cfg = loadConfig();
|
||||
if (!cfg) {
|
||||
console.error('No gbrain config; run `gbrain init` first.');
|
||||
process.exit(1);
|
||||
}
|
||||
const engineConfig = toEngineConfig(cfg);
|
||||
const engine = await createEngine(engineConfig);
|
||||
// v0.37.7.0 #1225: createEngine() only constructs; callers MUST connect
|
||||
// before any executeRaw call. Pre-fix, the first query in countAffected
|
||||
// crashed with "PGLite not connected. Call connect() first." even on
|
||||
// --dry-run. initSchema is idempotent on a current schema, costs ~1ms.
|
||||
await engine.connect(engineConfig);
|
||||
await engine.initSchema();
|
||||
|
||||
try {
|
||||
const result = await runReindexFrontmatter(engine, opts);
|
||||
if (opts.json) {
|
||||
console.log(JSON.stringify(result, null, 2));
|
||||
} else {
|
||||
const noun = result.status === 'dry_run' ? 'would update' : 'updated';
|
||||
console.error(
|
||||
`\nReindex ${result.status}: examined=${result.examined} ${noun}=${result.updated} ` +
|
||||
`fallback=${result.fallback} dur=${result.durationSec.toFixed(1)}s`,
|
||||
);
|
||||
}
|
||||
if (result.status === 'cancelled') process.exit(1);
|
||||
} finally {
|
||||
if ('disconnect' in engine && typeof engine.disconnect === 'function') {
|
||||
await engine.disconnect();
|
||||
}
|
||||
const result = await runReindexFrontmatter(engine, opts);
|
||||
if (opts.json) {
|
||||
console.log(JSON.stringify(result, null, 2));
|
||||
} else {
|
||||
const noun = result.status === 'dry_run' ? 'would update' : 'updated';
|
||||
console.error(
|
||||
`\nReindex ${result.status}: examined=${result.examined} ${noun}=${result.updated} ` +
|
||||
`fallback=${result.fallback} dur=${result.durationSec.toFixed(1)}s`,
|
||||
);
|
||||
}
|
||||
if (result.status === 'cancelled') process.exit(1);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,123 @@
|
||||
/**
|
||||
* #1963 regression test: `gbrain reindex-frontmatter` (and `gbrain backfill
|
||||
* <kind>`, same class) on a PGLite brain.
|
||||
*
|
||||
* Pre-fix, cli.ts's dispatch connected the primary engine (taking the PGLite
|
||||
* data-dir lock), then `reindexFrontmatterCli` / `runBackfillCommand` built
|
||||
* and connected a SECOND engine on the same data dir. `acquireLock` never
|
||||
* reaps a live PID — and the named holder was this very process — so the
|
||||
* command spun the full 30s lock timeout and exited 1 with "Timed out waiting
|
||||
* for PGLite data-dir lock", 100% of the time on PGLite. The fix passes the
|
||||
* already-connected engine through instead of double-connecting.
|
||||
*
|
||||
* Spawn-level on purpose: the bug lives in the CLI dispatch seam, which
|
||||
* in-process unit tests of `runReindexFrontmatter` (see
|
||||
* reindex-frontmatter-connect.test.ts) can never reach.
|
||||
*
|
||||
* Single-test design mirrors apply-migrations-pglite-spawn.serial.test.ts:
|
||||
* each `bun run src/cli.ts` spawn pays a cold-start cost on CI, so one test
|
||||
* walks the whole lifecycle. Serial because it spawns subprocesses + writes a
|
||||
* tmpdir.
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'fs';
|
||||
import { join } from 'path';
|
||||
import { tmpdir } from 'os';
|
||||
|
||||
const REPO = new URL('..', import.meta.url).pathname.replace(/\/$/, '');
|
||||
|
||||
async function runCli(
|
||||
args: string[],
|
||||
env: Record<string, string>,
|
||||
timeoutMs: number,
|
||||
): Promise<{ exitCode: number; stdout: string; stderr: string }> {
|
||||
// Scrub inherited GBRAIN_* so a developer's shell config (embedding model,
|
||||
// pace mode, …) can't change what the spawned CLI does.
|
||||
const base = Object.fromEntries(
|
||||
Object.entries(process.env).filter(([k]) => !k.startsWith('GBRAIN_')),
|
||||
) as Record<string, string>;
|
||||
const proc = Bun.spawn(['bun', 'run', `${REPO}/src/cli.ts`, ...args], {
|
||||
cwd: REPO,
|
||||
env: { ...base, ...env },
|
||||
stdout: 'pipe',
|
||||
stderr: 'pipe',
|
||||
});
|
||||
const killer = setTimeout(() => {
|
||||
try { proc.kill('SIGKILL'); } catch { /* already dead */ }
|
||||
}, timeoutMs);
|
||||
try {
|
||||
const [stdout, stderr, exitCode] = await Promise.all([
|
||||
new Response(proc.stdout).text(),
|
||||
new Response(proc.stderr).text(),
|
||||
proc.exited,
|
||||
]);
|
||||
return { exitCode, stdout, stderr };
|
||||
} finally {
|
||||
clearTimeout(killer);
|
||||
}
|
||||
}
|
||||
|
||||
describe('reindex-frontmatter + backfill on PGLite (#1963 double-connect)', () => {
|
||||
test('init → import → reindex-frontmatter --yes → backfill effective_date --dry-run (all exit 0, no lock timeout)', async () => {
|
||||
const home = mkdtempSync(join(tmpdir(), 'gbrain-1963-'));
|
||||
const notes = mkdtempSync(join(tmpdir(), 'gbrain-1963-notes-'));
|
||||
try {
|
||||
mkdirSync(join(home, '.gbrain'), { recursive: true });
|
||||
writeFileSync(
|
||||
join(home, '.gbrain', 'config.json'),
|
||||
JSON.stringify({
|
||||
engine: 'pglite',
|
||||
database_path: join(home, '.gbrain', 'brain.pglite'),
|
||||
embedding_dimensions: 1536,
|
||||
}) + '\n',
|
||||
);
|
||||
for (let i = 1; i <= 3; i++) {
|
||||
writeFileSync(
|
||||
join(notes, `note${i}.md`),
|
||||
`---\neffective_date: 2025-01-0${i}\n---\n# note ${i}\n\nbody ${i}\n`,
|
||||
);
|
||||
}
|
||||
const env = { HOME: home, GBRAIN_HOME: home };
|
||||
|
||||
const init = await runCli(['init', '--migrate-only'], env, 120_000);
|
||||
if (init.exitCode !== 0) {
|
||||
console.error('--- init stdout ---\n' + init.stdout);
|
||||
console.error('--- init stderr ---\n' + init.stderr);
|
||||
}
|
||||
expect(init.exitCode).toBe(0);
|
||||
|
||||
const imp = await runCli(['import', notes, '--no-embed'], env, 120_000);
|
||||
if (imp.exitCode !== 0) {
|
||||
console.error('--- import stdout ---\n' + imp.stdout);
|
||||
console.error('--- import stderr ---\n' + imp.stderr);
|
||||
}
|
||||
expect(imp.exitCode).toBe(0);
|
||||
|
||||
// Pre-fix: exits 1 after the 30s PGLite lock timeout, naming ITSELF as
|
||||
// the holder. Post-fix: reuses cli.ts's connected engine and succeeds.
|
||||
const reindex = await runCli(['reindex-frontmatter', '--yes', '--json'], env, 120_000);
|
||||
const reindexOut = reindex.stdout + reindex.stderr;
|
||||
if (reindex.exitCode !== 0) {
|
||||
console.error('--- reindex-frontmatter stdout ---\n' + reindex.stdout);
|
||||
console.error('--- reindex-frontmatter stderr ---\n' + reindex.stderr);
|
||||
}
|
||||
expect(reindexOut).not.toMatch(/Timed out waiting for PGLite/);
|
||||
expect(reindex.exitCode).toBe(0);
|
||||
expect(reindex.stdout).toMatch(/"status":\s*"ok"/);
|
||||
|
||||
// `gbrain backfill <kind>` had the identical double-connect. dry-run
|
||||
// still connects, so pre-fix it hit the same 30s timeout.
|
||||
const backfill = await runCli(['backfill', 'effective_date', '--dry-run'], env, 120_000);
|
||||
const backfillOut = backfill.stdout + backfill.stderr;
|
||||
if (backfill.exitCode !== 0) {
|
||||
console.error('--- backfill stdout ---\n' + backfill.stdout);
|
||||
console.error('--- backfill stderr ---\n' + backfill.stderr);
|
||||
}
|
||||
expect(backfillOut).not.toMatch(/Timed out waiting for PGLite/);
|
||||
expect(backfill.exitCode).toBe(0);
|
||||
} finally {
|
||||
try { rmSync(home, { recursive: true, force: true }); } catch { /* best effort */ }
|
||||
try { rmSync(notes, { recursive: true, force: true }); } catch { /* best effort */ }
|
||||
}
|
||||
}, 480_000);
|
||||
});
|
||||
Reference in New Issue
Block a user