From 5cd0d6eddd66095ea7c2922037c85a40ec725acd Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 28 Jul 2026 16:47:15 -0700 Subject: [PATCH] fix(reindex-frontmatter): reuse the connected engine instead of self-deadlocking on the PGLite lock (#1963) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cli.ts's dispatch connects the primary engine (taking the PGLite data-dir lock), then reindexFrontmatterCli built and connected a SECOND engine on the same data dir. acquireLock never reaps a live PID — and the recorded holder was this very process — so every 'gbrain reindex-frontmatter' on PGLite spun the full 30s lock timeout and exited 1, reporting itself as the blocking process. (The issue's statement_timeout theory was a red herring: the backfill itself finishes in milliseconds.) 'gbrain backfill ' had the identical double-connect and is fixed the same way: both commands now take the already-connected engine from the dispatch switch, and cli.ts's shared finally owns teardown. Deliberately NOT changed: pglite-lock.ts. Making the lock reentrant (or special-casing our own PID) would contradict the pinned #2348 semantics — a live-PID holder is never stolen, PID-reuse included — and the root cause is the second connect, not the lock. Regression test spawns the real CLI against a fresh PGLite home (init → import → reindex-frontmatter → backfill effective_date); on master it fails after the 30s self-deadlock, with the fix it passes in seconds. Fixes #1963 Co-Authored-By: Claude Opus 5 --- docs/architecture/KEY_FILES.md | 4 +- src/cli.ts | 16 ++- src/commands/backfill.ts | 22 ++-- src/commands/reindex-frontmatter.ts | 55 +++----- ...ex-frontmatter-pglite-spawn.serial.test.ts | 123 ++++++++++++++++++ 5 files changed, 167 insertions(+), 53 deletions(-) create mode 100644 test/reindex-frontmatter-pglite-spawn.serial.test.ts diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index e9c1f4552..3268a48df 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -32,7 +32,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/doctor.ts` extension — `graph_signals_coverage` check wired into both `runDoctor` (local) and `doctorReportRemote` (HTTP/JSON thin-client path). Reads `search.graph_signals` config first, falls back to mode default; silent `ok` when disabled. Computes inbound link coverage on the page set; warns at <10% with `gbrain extract all` fix hint; `ok` at ≥30% ("fire on most queries") and 10-29% ("fire occasionally"), each with the percentage embedded. Pinned by cases in `test/doctor.test.ts`. - `src/core/engine-factory.ts` — Engine factory with dynamic imports (`'pglite'` | `'postgres'`). - `src/core/pglite-engine.ts` — PGLite (embedded Postgres 17.5 via WASM) implementation, all BrainEngine methods. `listLinkSources({sourceId?, sourceIds?})` returns distinct `link_source` provenances + counts (`ORDER BY count DESC, link_source ASC NULLS LAST`; scalar + federated scoped; parity with postgres-engine.ts) powering `gbrain link-sources`. `addLinksBatch`/`addTimelineEntriesBatch`/`addTakesBatch` pass the whole batch as one JSONB document via `jsonb_to_recordset(($1::jsonb)->'rows')` (bound through `executeRawJsonb` with a `{ rows }` wrapper; rows built by the shared `src/core/batch-rows.ts` helpers, NUL-stripped), and are `batchRetry`-wrapped. `connect()` wraps `PGlite.create()` in a try/catch that emits an actionable error (macOS 26.3 WASM bug #223, points at `gbrain doctor`); the lock is released on failure so the next process can retry cleanly. `searchKeyword`/`searchKeywordChunks` multiply `ts_rank` by the source-factor CASE at chunk grain; `searchVector` is a two-stage CTE — inner CTE keeps `ORDER BY cc.embedding <=> vec` so HNSW stays usable, outer SELECT re-ranks by `raw_score * source_factor`, inner LIMIT scales with offset to preserve pagination. `searchTakes`/`searchTakesVector` take full `SearchOpts` and apply the standard source-scope predicates (federated `sourceIds[]` wins over scalar `sourceId`, via the joined page's `source_id`) alongside the holder allow-list — parity SQL in postgres-engine.ts; pinned by `test/e2e/think-source-isolation-pglite.test.ts`. `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL — probes for forward-referenced state the embedded blob needs (`pages.source_id`, `links.link_source`, `links.origin_page_id`, `content_chunks.symbol_name`, `content_chunks.language`, `sources` FK target, plus `files.source_id`, `files.page_id`, `oauth_clients.source_id`, `oauth_clients.federated_read`, `sources.archived`, `sources.archived_at`, `sources.archive_expires_at`, `timeline_entries.event_page_id` — column-only, migration v121 stays the source of truth for its FK + indexes) and adds only what's missing; threads the DDL connection from `initSchema` so probes run inside the advisory-lock scope; no-op on fresh installs and modern brains (closes the upgrade-wedge bug class #239/#243/#266/#357/#366/#374/#375/#378/#395/#396/#1018/#974/#820). `getBrainScore` returns 100/100 with full breakdown (35/25/15/15/10) when `pageCount === 0` (vacuous truth — empty brain has no coverage problem); Pinned by `test/brain-score-breakdown.test.ts` empty-brain assertion + `test/doctor-report-remote.serial.test.ts`. `disconnect()` uses snapshot+early-null (snapshot `_db`/`_lock`, null instance fields BEFORE any `await` so a concurrent `connect()` can't see a partial mid-close state) wrapped in try/finally guaranteeing lock-release even if `db.close()` throws; KEEPS close-then-release order (release-then-close was rejected: it would widen the window where a sibling process connects to a still-closing brain); Pinned by `test/pglite-engine-disconnect.serial.test.ts`. `PGlite.create()` runs inside `preservingProcessExitCode` (#2084): PGLite's Emscripten runtime writes its own status into `process.exitCode` (99 at create; in-memory brains run initdb whose status lands on a later tick; the exit status at close — and assigning `undefined` cannot clear a prior value), which would otherwise silently clobber an errored op's exit 1 back to 0. The wrapper keeps the global tidy for external readers; `db.close()` stays unwrapped (its 0-write is baseline behavior test runners depend on). The CLI's exit verdict is immune either way — it lives in the gbrain-owned channel in `cli-force-exit.ts` and never reads `process.exitCode` back. Exports `classifyPgliteInitError(message): 'bunfs' | 'macos-26-3' | 'unknown'` + `buildPgliteInitErrorMessage(verdict, original)` routing the catch-block hint by failure shape (`bunfs` matches literal `$$bunfs` OR `ENOENT[\s\S]*pglite\.data` co-occurrence, surfaces a paste-ready `bun upgrade` + Node fallback; `macos-26-3` keeps the #223 link; `unknown` falls through); Pinned by `test/pglite-init-classifier.test.ts`. Implements `deletePages(slugs, {sourceId})` + `resolveSlugsByPaths(paths, {sourceId})` via `slug = ANY($1::text[])` array-param binding, caller-chunking primitive throwing when input exceeds `DELETE_BATCH_SIZE`, `deletePages` returns `RETURNING slug` rows so callers filter `pagesAffected` to confirmed deletes. Implements the embedding-signature stale-detection quartet — `sumStaleChunkChars({sourceId?, signature?})`, `setPageEmbeddingSignature(slug, {sourceId?, signature})`, `invalidateStaleSignatureEmbeddings({signature, sourceId?})`, widened `countStaleChunks({sourceId?, signature?})` (the `signature` opt widens via `JOIN pages p ... WHERE cc.embedding IS NULL OR (p.embedding_signature IS NOT NULL AND p.embedding_signature <> $signature)`, NULL grandfathered); parity SQL with postgres-engine.ts. PGLite-specific DDL (pgvector, pg_trgm, triggers). -- `src/core/pglite-lock.ts` — advisory data-dir lock so two processes never open the same embedded PGLite (single-connection WASM Postgres) at once. Atomic `mkdir` of `.gbrain-lock/` + a `lock` file carrying `{pid, acquired_at, refreshed_at, command, subcommand}`. A held lock HEARTBEATS its `refreshed_at` every 30s (`.unref()`ed timer; informational). A waiting acquirer reaps a holder ONLY when its PID is dead — a LIVE holder is NEVER stolen, regardless of how stale its heartbeat is (#2348). A live `gbrain serve` holder is identified from the parsed `subcommand` and reported immediately with separate CLI-retry and MCP-tool choices; other live holders keep the bounded wait. The heartbeat runs on the JS event loop, which is blocked during long synchronous WASM imports/CHECKPOINTs, so a working `dream`/`embed` holder can look stale while alive; the old steal-on-stale-heartbeat grace let a second OS process open the same data dir and corrupt the catalog + pgvector extension (58P01 / `internal_load_library` / `type "vector" does not exist`), recoverable only by wipe+restore. A wedged-but-alive or PID-reused holder is never stolen: serve-tagged holders report immediately, while other holders time out with a message naming the PID. Each holder carries an ownership token (`:`); the heartbeat and `releaseLock` verify the on-disk lock is STILL theirs before touching it. In-memory engines take no lock. Pinned by `test/pglite-lock.test.ts`. A corrupted store surfaces a `reinit-pglite` recovery hint via `classifyPgliteInitError`'s `corrupt` verdict in `pglite-engine.ts`. +- `src/core/pglite-lock.ts` — advisory data-dir lock so two processes never open the same embedded PGLite (single-connection WASM Postgres) at once. Atomic `mkdir` of `.gbrain-lock/` + a `lock` file carrying `{pid, acquired_at, refreshed_at, command, subcommand}`. A held lock HEARTBEATS its `refreshed_at` every 30s (`.unref()`ed timer; informational). A waiting acquirer reaps a holder ONLY when its PID is dead — a LIVE holder is NEVER stolen, regardless of how stale its heartbeat is (#2348). A live `gbrain serve` holder is identified from the parsed `subcommand` and reported immediately with separate CLI-retry and MCP-tool choices; other live holders keep the bounded wait. The heartbeat runs on the JS event loop, which is blocked during long synchronous WASM imports/CHECKPOINTs, so a working `dream`/`embed` holder can look stale while alive; the old steal-on-stale-heartbeat grace let a second OS process open the same data dir and corrupt the catalog + pgvector extension (58P01 / `internal_load_library` / `type "vector" does not exist`), recoverable only by wipe+restore. A wedged-but-alive or PID-reused holder is never stolen: serve-tagged holders report immediately, while other holders time out with a message naming the PID. Each holder carries an ownership token (`:`); the heartbeat and `releaseLock` verify the on-disk lock is STILL theirs before touching it. In-memory engines take no lock. There is deliberately NO same-process reentrancy or same-PID special case: a second `acquireLock` from the process that already holds the lock waits out the timeout like any other live holder (#1963 was this shape — a command double-connecting a second engine on the same data dir; the fix is to reuse the connected engine at the dispatch layer, never to soften the lock). Pinned by `test/pglite-lock.test.ts`. A corrupted store surfaces a `reinit-pglite` recovery hint via `classifyPgliteInitError`'s `corrupt` verdict in `pglite-engine.ts`. - `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch`/`addTimelineEntriesBatch`/`addTakesBatch` pass the batch as one JSONB document — `INSERT ... SELECT FROM jsonb_to_recordset(($1::jsonb)->'rows') AS v(...) JOIN pages ...` bound through `executeRawJsonb({ rows })` — which encodes arbitrary free text safely (the old `unnest(${arr}::text[])` array-literal path crashed Postgres with "malformed array literal" on calendar/Zoom context, gbrain#1861) and sidesteps the 65535-parameter cap; takes declares native recordset column types (`page_id int, weight real, active boolean, …`) so no per-element casts; all three are `batchRetry`-wrapped. `searchKeyword`/`searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection. `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. `searchKeyword`/`searchKeywordChunks`/`searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude from `src/core/search/sql-ranking.ts`; `searchVector` is a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in outer SELECT) carrying `p.source_id` inner→outer. `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures, and by `batchRetry` on a retryable connection error). Concurrent callers share one in-flight `_reconnectPromise` (they await the single reconnect rather than racing a half-rebuilt pool); ownership re-samples through the atomic `db.connect()` token on the connect leg. `reconnect(ctx?)` accepts the triggering error and records a pool-recovery audit event (`reap_detected`/`reconnect_other`/`reconnect_succeeded`/`reconnect_failed`) for the `pool_reap_health` doctor check. `executeRaw` is a single-statement passthrough — no per-call retry (unsound for non-idempotent statements; recovery is supervisor-driven). `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. `countStaleChunks()`+`listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale` (eliminates ~76 MB/call client-side pull); `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding. `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same probe set as PGLite (extended for column-only forward-reference cases: `files.source_id`, `files.page_id`, `oauth_clients.source_id`, `oauth_clients.federated_read`, `sources.archived`/`archived_at`/`archive_expires_at`, `timeline_entries.event_page_id`); the entire probe path runs on the DDL connection threaded from `initSchema` (closing a concurrent-bootstrap race for Supabase pooler users); closes #1018/#974/#820. `disconnect()` is idempotent — `_connectionStyle` tracks whether the engine owns its pool (worker engines) or shares the module-level singleton; second call on an instance-pool engine is a no-op rather than clobbering the singleton; and a module-style engine only calls `db.disconnect()` when it owns the singleton (`_ownsModuleSingleton`, set from the `db.connect()` creation token), so a borrower probe engine's teardown leaves the cycle owner's connection intact. Pinned by `test/e2e/postgres-engine-disconnect-idempotency.test.ts` + `test/postgres-engine-singleton-ownership.test.ts`. `getBrainScore` empty-brain parity with PGLite — 100/100 with breakdown 35/25/15/15/10 when `pageCount === 0` (both engines must agree to keep `doctor-report-remote.serial.test.ts` deterministic). Implements `deletePages(slugs, {sourceId}): Promise` via `DELETE FROM pages WHERE slug = ANY($1::text[]) AND source_id = $2 RETURNING slug` (single round-trip; caller chunks); `resolveSlugsByPaths` does `SELECT slug, source_path FROM pages WHERE source_path = ANY($1::text[]) AND source_id = $2`; FK cascades through `content_chunks`/`links`/`tags`/`raw_data`/`timeline_entries`/`page_versions`, `files.page_id`+`links.origin_page_id` go SET NULL; throws when input exceeds `DELETE_BATCH_SIZE` (from `src/core/engine-constants.ts`); both short-circuit on empty input. Implements the embedding-signature stale-detection quartet (`sumStaleChunkChars`, `setPageEmbeddingSignature`, `invalidateStaleSignatureEmbeddings`, widened `countStaleChunks`, all accept optional `signature` extending "stale" to model/dims-swap drift via the `pages.embedding_signature` JOIN, NULL grandfathered; the `embedding IS NULL` server-side filter is preserved as the no-signature fast path); Pinned by `test/e2e/engine-parity.test.ts`. - `src/core/cjk.ts` — Single source of truth for CJK detection. Exports `CJK_RANGES_REGEX`, `CJK_SLUG_CHARS` (character-class fragment for embedding inside other regexes), `CJK_SENTENCE_DELIMITERS` (`。！？`), `CJK_CLAUSE_DELIMITERS` (`；：，、`), `CJK_DENSITY_THRESHOLD = 0.30`, `hasCJK(s)`, `countCJKAwareWords(s)` (30% density threshold — English docs with one Japanese term stay whitespace-tokenized; Chinese-dominant docs get char-counted), and `escapeLikePattern(s)` (escapes `%`, `_`, `\\` for `ILIKE ... ESCAPE '\\'`). BMP-only ranges (Han / Hiragana / Katakana / Hangul Syllables). Consumers: `expansion.ts`, `sync.ts:slugifySegment`, `operations.ts:validatePageSlug + validateFilename`, `chunkers/recursive.ts:countWords + DELIMITERS`, `pglite-engine.ts:searchKeyword + searchKeywordChunks`. - `src/core/audit-slug-fallback.ts` — Weekly ISO-week-rotated audit JSONL at `~/.gbrain/audit/slug-fallback-YYYY-Www.jsonl`. `logSlugFallback(slug, sourcePath)` fires when `importFromFile` falls back to a frontmatter slug because `slugifyPath` returned empty (emoji / Thai / Arabic / non-CJK exotic-script filenames). `readRecentSlugFallbacks(days)` reads the last N days for `gbrain doctor`'s `slug_fallback_audit` check. Honors `GBRAIN_AUDIT_DIR` via the shared `resolveAuditDir()`. Separate surface from `sync-failures.jsonl` — that file carries bookmark-gating semantics that info events shouldn't trigger. @@ -205,7 +205,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/import.ts` — `gbrain import [--source-id ]`: page import with the path-set checkpoint. `--source-id ` routes pages to the named source (resolved via `resolveSourceWithTier()` at the boundary; consistent across `import`, `extract`, `graph-query`, `sources current`). Pinned by `test/import-source-id.test.ts`. - `src/commands/graph-query.ts` — `gbrain graph-query [--type T] [--depth N] [--direction in|out|both] [--include-foreign]`: typed-edge relationship traversal (renders indented tree). Foreign-edge footer always present (`X foreign edges (use --include-foreign to traverse)`) so cross-source edges never disappear silently; `--include-foreign` widens the SQL filter to walk them. Pinned by `test/graph-query.test.ts`. - `src/commands/sources.ts` — `gbrain sources {list,add,remove,archive,restore,archived,purge,current,status,audit}`. `current [--json]` calls `resolveSourceWithTier()` and prints `source_id`, `tier` (`flag | env | dotfile | local_path | brain_default | seed_default`), and optional `detail` (decision table in `skills/conventions/brain-routing.md`). `status [--json]` — read-only per-source dashboard (last sync, staleness, page count, embedding coverage, unacked failures); thin wrapper around `buildSyncStatusReport` + `printSyncStatusReport` from `src/commands/sync.ts`; `--json` emits stable `{schema_version: 1, sources, ...}` on stdout; filters input to `local_path IS NOT NULL AND archived IS NOT TRUE`. `audit [--json]` — read-only dry-run disk scan for size distribution + would-blocks + junk-pattern hits WITHOUT touching the DB; walks `sources.local_path`, reads each markdown file, runs `assessContent()` from `src/core/content-sanity.ts`, aggregates by verdict (`ok | warn_oversize | hard_block_junk_pattern`). The live `runStatus` health table gains a `BACKFILL` column between `EMBED` and `FAILS` (`active(N)` beats `queued(N)` beats `idle`, from `SourceMetrics.backfill_active` / `backfill_queued` in `src/core/source-health.ts`) so operators see deferred `embed-backfill` minion work after `sync --all` exits 0; `jobCountsBySource` in `source-health.ts` widens its `minion_jobs` SQL with two `COUNT(*) FILTER (WHERE name = 'embed-backfill' AND ...)` aggregates (best-effort, all-0 on pre-minions brains). Pinned by `test/content-sanity.test.ts`, `test/import-file-content-sanity.test.ts`, `test/source-health.test.ts`. -- `src/commands/reindex-frontmatter.ts` — `gbrain reindex-frontmatter`. Query path wrapped in the standard `withEngine(...)` lifecycle so `engine.connect()` runs before the first SQL call. Pinned by `test/reindex-frontmatter-connect.test.ts`. +- `src/commands/reindex-frontmatter.ts` — `gbrain reindex-frontmatter`. `reindexFrontmatterCli(engine, args)` takes the ALREADY-CONNECTED engine from cli.ts's dispatch (#1963); it must never build/connect its own engine — a second connect on the same PGLite data dir self-deadlocks on the data-dir lock (this process already holds it) and timed out 100% of the time on PGLite. Same rule applies to `runBackfillCommand(engine, args)` in `src/commands/backfill.ts` and any future command dispatched from cli.ts's engine-connected switch. Pinned by `test/reindex-frontmatter-connect.test.ts` (library path) and `test/reindex-frontmatter-pglite-spawn.serial.test.ts` (CLI dispatch seam, both commands). - `src/core/source-config-sql.ts` + `src/core/sources-load.ts` — canonical recovery for historical non-object `sources.config` values. The application reader unwraps nested JSON strings and merges recoverable array fragments left-to-right; the shared SQL expression mirrors that policy atomically for both engines, source config updates, archive/restore, and the paste-ready `source_config_shape` doctor repair. `localFederatedSourceIds` reads config through the same parser so stdio/CLI federation cannot silently disagree with `sources list`. Invalid fragments degrade to `{}` rather than throwing. Pinned by `test/sources-load.test.ts`, `test/list-all-sources.test.ts`, `test/local-federated-search-scope.test.ts`, `test/destructive-guard.test.ts`, and `test/doctor-source-config-shape.test.ts`. - `src/core/source-resolver.ts` — 6-tier source resolution. `resolveSourceWithTier(engine, explicit, cwd)` returns `{ source_id, tier: SourceTier, detail? }` alongside `resolveSourceId()` (unchanged). `SOURCE_TIER_NAMES = ['flag', 'env', 'dotfile', 'local_path', 'sole_non_default', 'brain_default', 'seed_default']` (7 entries; order matches priority). Tier `sole_non_default` slots between `local_path` and `brain_default`: when NO `sources.default` config is set AND exactly one registered source has `local_path` AND isn't `'default'`, auto-route to it; archived sources excluded (try/catch for pre-v34 brains); private `pickSoleNonDefaultSource(engine)` shared by both resolver entry points so they cannot drift. Exported `formatSoleNonDefaultNudge(sourceId): string | null` builds the user-facing stderr nudge (null when `GBRAIN_NO_SOLE_NON_DEFAULT_NUDGE=1`). `src/commands/sync.ts:1497-1519` calls `resolveSourceWithTier` unconditionally so the tier fires; `src/commands/import.ts:96-128` mirrors with the tier-gated nudge. Consumed by `gbrain sources current`, `import --source-id`, `extract --source-id`, and the `source_routing_health` doctor check. Pinned by `test/source-resolver-with-tier.test.ts` (`withEnv()` per test-isolation lint), `test/source-resolver-sole-non-default.test.ts` (14 cases), `test/sync-sole-non-default-routing.test.ts` (3 PGLite cases driving real `runSync`). - `src/core/sync.ts` extension — `isSyncable` factored through private `classifySync(path, opts): SyncableReason | null`; exported companion `unsyncableReason(path, opts)` returns the same tagged reason or null when syncable. `SYNC_SKIP_FILES` is a named export (the four canonical metafile basenames `schema.md`, `index.md`, `log.md`, `README.md`). `SyncableReason` union: `'metafile' | 'strategy' | 'pruned-dir' | 'include-glob-miss' | 'exclude-glob-hit'`. The `commands/sync.ts` cleanup loop guards on `unsyncableReason(path)` being `'metafile'` OR `'pruned-dir'` (#2404) so previously-indexed metafile pages AND deliberately put-created pages under pruned dirs survive every re-sync. Does NOT cover `manifest.deleted` (the upstream filter already strips metafiles). Pinned by `test/sync-isSyncable-shape.test.ts` (15 cases, duality contract) + `test/sync-metafile-skip.serial.test.ts` (3 PGLite cases incl. the renamed `.md → .txt` negative). diff --git a/src/cli.ts b/src/cli.ts index 8965a2d70..a044e171a 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -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 ?" diff --git a/src/commands/backfill.ts b/src/commands/backfill.ts index 1d07d4623..b13b73e68 100644 --- a/src/commands/backfill.ts +++ b/src/commands/backfill.ts @@ -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 { +/** + * #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 ` on PGLite timed out + * after 30s. Engine lifecycle belongs to cli.ts's connect + teardown. + */ +export async function runBackfillCommand(engine: BrainEngine, args: string[]): Promise { const cli = parseArgs(args); if (cli.help) { printHelp(); return; } @@ -144,20 +151,10 @@ export async function runBackfillCommand(args: string[]): Promise { 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 { 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); } diff --git a/src/commands/reindex-frontmatter.ts b/src/commands/reindex-frontmatter.ts index 0571731ae..7d83a4995 100644 --- a/src/commands/reindex-frontmatter.ts +++ b/src/commands/reindex-frontmatter.ts @@ -151,8 +151,17 @@ export async function runReindexFrontmatter( }; } -/** CLI entrypoint. Argv shape matches reindex-code for consistency. */ -export async function reindexFrontmatterCli(args: string[]): Promise { +/** + * 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 { 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 { } } - 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); } diff --git a/test/reindex-frontmatter-pglite-spawn.serial.test.ts b/test/reindex-frontmatter-pglite-spawn.serial.test.ts new file mode 100644 index 000000000..5458c757e --- /dev/null +++ b/test/reindex-frontmatter-pglite-spawn.serial.test.ts @@ -0,0 +1,123 @@ +/** + * #1963 regression test: `gbrain reindex-frontmatter` (and `gbrain backfill + * `, 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, + 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; + 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 ` 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); +});