diff --git a/CHANGELOG.md b/CHANGELOG.md index f3545fd35..69218cce8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,52 @@ All notable changes to GBrain will be documented in this file. +## [0.31.2] - 2026-05-08 + +**`gbrain sync --strategy code` no longer hangs on big repos. Code-strategy first-sync actually indexes code files. Walker hardened. Tree-sitter is now bounded.** + +A 1500-file gstack repo could pin `gbrain sync --strategy code` at 99% CPU on a single thread for hours, with zero disk writes and zero TCP, and a `page_count` that stayed at 0. Three real defects, fixed together: tree-sitter's WASM loop had no wall-clock cap, so a single pathological file could wedge the whole sync indefinitely. Code-strategy first-sync silently fed only `.md` files into the import pipeline, so even when sync didn't hang, no code page was produced. The walker that powered sync's cost preview was weaker than the import walker for no good reason. v0.31.2 closes all three. + +### What you can now do + +**Run `gbrain sync --strategy code` on big symlink-rich repos without wedging.** Per-file tree-sitter parsing is bounded by a 30-second wall-clock cap via `parser.setTimeoutMicros`. When a file's parse exceeds the cap, the chunker logs `[gbrain chunker] timeout parsing after 30000ms; falling back to recursive chunks` to stderr and falls back to the recursive text chunker. Search quality on that one file degrades; the sync keeps going. Override the default with `GBRAIN_CHUNKER_TIMEOUT_MS=60000`. + +**Code-strategy first-sync now imports code files.** Previously `performFullSync` called `runImport(repoPath)` with no strategy, which silently fell through to a markdown-only walker. Now strategy threads end-to-end (`performSync → performFullSync → runImport`), through the full-sync write path AND the dry-run preview. `gbrain sync --strategy code --dry-run` finally reports the right number. + +**Walker survives self-referencing symlinks.** The new `collectSyncableFiles` is the single source of truth across import, full-sync dry-run, and cost preview. Three layered defenses: `lstatSync` rejects every symlink before recursion; an inode-cycle Map keyed on `${st_dev}:${st_ino}` catches non-symlink loops (bind mounts, ZFS snapshots); `MAX_WALK_DEPTH=32` is the structural backstop. Output is sorted so `runImport`'s checkpoint resume stays deterministic across runs. Override the depth cap with `GBRAIN_MAX_WALK_DEPTH=64`. + +**Phase logs surface where time goes.** Strategic stderr lines (`[gbrain phase] sync.git_pull start`, `sync.fullsync.import done 12345ms imported=602 skipped=3 errors=0`, `import.collect_files done 187ms files=1503`, `import.process_file slow 7234ms src/big.ts`) tell you which phase a stuck sync is in. The next hang reproduction will name the phase, not produce zero data. + +### Important note for upgraders + +After `gbrain upgrade`, the first sync against any source registered with `--strategy code` will now actually import code files that previously got skipped. On a 1500-file repo this means a one-time embed-cost spike proportional to your code-file count (rough order: ~1500 code files × ~1500 tokens average = 2.25M tokens × $0.13/1M = $0.30 with `text-embedding-3-large`). Run with `--dry-run` first to preview the file count. The CHANGELOG-and-actually-correct behavior is now aligned. + +### How it works under the hood + +`parser.setTimeoutMicros(timeoutMs * 1000)` is the canonical web-tree-sitter API for wall-clock parser caps; `parser.parse()` returns null when exceeded. `parseWithTimeout` (the new pure-function seam in `src/core/chunkers/code.ts`) wraps the call, throws `ChunkerTimeoutError` on null, and the caller's `try/finally` reaps the parser+tree WASM allocation. The pre-fix `chunkCodeTextFull` did manual `delete()` on success and on null, but the catch block returned without cleanup — codex caught the leak before it shipped. + +`isCollectibleForWalker(path, strategy, multimodal)` surfaces the markdown+multimodal carve-out at one site instead of leaking it across two filters. `isSyncable` (incremental diff path) admits images only on `auto`; the walker historically admitted them on markdown too when `GBRAIN_EMBEDDING_MULTIMODAL=true`. Same behavior, one source of truth. + +### Tests + +- `test/sync-walker-symlink.test.ts` (7 cases) — self-referencing symlink, symlink chain through real dirs, max-depth bailout, strategy filter, dot-dir + node_modules skip, multimodal preservation under markdown strategy, deterministic ordering. +- `test/chunker-timeout.test.ts` (7 cases) — parser-stub timeout seam, ChunkerTimeoutError shape, env wiring under `GBRAIN_CHUNKER_TIMEOUT_MS=1`, fallback-to-recursive behavior, fail-loud regression for missing `setTimeoutMicros`, repeated-timeout cleanup smoke test. + +### To take advantage of v0.31.2 + +`gbrain upgrade` then re-run any previously-stuck `gbrain sync --strategy code`. Should complete in ~25-35 minutes on a 1500-file repo. If it doesn't, the `[gbrain phase] *` log lines will name the phase that wedged — file an issue with the relevant log section. + +```bash +gbrain upgrade +cd +gbrain sync --strategy code --dry-run --source # preview the file count +gbrain sync --strategy code --source # the real run +``` + +### For contributors + +Codex's adversarial review caught five bug-grade findings in the original plan that hadn't survived eng review: parser cleanup leak under thrown timeout, an internal contradiction between `isSyncable` and the multimodal walker behavior, a missed dry-run plumbing site, deterministic-order requirement for checkpoint resume, and machine-speed-dependent timeout tests. All five folded into the shipped fix without re-litigating the bundle decision. Cross-model review on every nontrivial plan change continues to pay. + ## [0.31.1.1-fixwave] - 2026-05-09 **Security upgrade priority: closes an authorization-code scope-escalation in `gbrain serve --http`. Plus 18 community fix-wave PRs covering upgrade-path correctness, multi-source sync, takes-fence privacy, dream cycle reliability, and CLI hygiene.** diff --git a/TODOS.md b/TODOS.md index 58a32d827..3d39045de 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,51 @@ # TODOS +## v0.31.2 follow-ups + +### Investigate: `gbrain query ` infinite loop +**Priority:** P1 +**Filed:** 2026-05-08 from v0.31.2 bug report (separate from the sync hang). + +**Evidence:** Two `bun /Users/garrytan/.bun/bin/gbrain query the` processes +(PIDs 39429, 46624) on the user's Mac were pegged at 99% CPU for 7 +straight days before being killed manually. Each used 6+ GB resident +memory. Originated from the `algiers-v3` worktree. Not walker-related +(query path doesn't traverse files), so the v0.31.2 fix doesn't address +it. + +**Likely candidates:** +- Query-expansion regex catastrophic backtracking on common single words + (`src/core/search/expansion.ts` calls Haiku then post-processes with + regex; a one-token query plus an unhelpful expansion could feed a + pathological input back into the search pipeline) +- Hybrid-search RRF reciprocal-rank-fusion loop iterating over a result + set that never shrinks (`src/core/search/hybrid.ts`) +- `postgres.js` cursor that never closes when the result set is large + (the 6GB RES on `query` smells like accumulated rows in JS memory, not + WASM allocation) + +**To reproduce:** create a brain with at least a few thousand pages, run +`gbrain query the` and watch CPU + RSS. If it pegs and grows, capture +`process.report.getReport()` and a stack trace via `kill -SIGUSR2 ` +before killing. + +**Out of scope for v0.31.2** because the user's primary symptom (sync +hang) was the higher-evidence bug. Pick this up as v0.31.3 once the +sync fix is verified working in production. + +### v0.31.3: PGLite + Postgres E2E for amarillo-shape regression +**Priority:** P2 +**Filed:** 2026-05-08 from v0.31.2 plan (deferred). + +**What:** Plan called for two regression tests pinning the user's exact +repro topology: `test/sync-walker-amarillo-shape.test.ts` (PGLite, +fast-loop) and `test/e2e/sync-amarillo-shape.test.ts` (real-Postgres, +skip-on-no-DB). Unit-level walker + chunker tests landed in v0.31.2 +(`test/sync-walker-symlink.test.ts` + `test/chunker-timeout.test.ts`), +but the engine-integrated regression for the user's exact 1500-file +self-symlink topology is still pending. Add when the next sync-related +PR is in flight. + ## Thin-client mode follow-ups (v0.31.1, Issue #734) - [ ] **v0.31.x: routed-call timing telemetry.** `GBRAIN_TIMING=1` prints diff --git a/VERSION b/VERSION index 31404ff0d..d1ae0a019 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.31.1.1-fixwave +0.31.2 \ No newline at end of file diff --git a/package.json b/package.json index 4b0301bd1..41dfcc320 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.31.1.1-fixwave", + "version": "0.31.2", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", diff --git a/src/commands/import.ts b/src/commands/import.ts index b7b8fe2a4..3f6b4c4ca 100644 --- a/src/commands/import.ts +++ b/src/commands/import.ts @@ -7,6 +7,12 @@ import { importFile, importImageFile, isImageFilePath } from '../core/import-fil import { loadConfig, gbrainPath } from '../core/config.ts'; import { createProgress } from '../core/progress.ts'; import { getCliOptions, cliOptsToProgressOptions } from '../core/cli-options.ts'; +import { + isCodeFilePath, + isMarkdownFilePath, + isImageFilePath as isImageFilePathFromSync, + type SyncStrategy, +} from '../core/sync.ts'; function defaultWorkers(): number { const cpuCount = cpus().length; @@ -28,7 +34,11 @@ export interface RunImportResult { failures: Array<{ path: string; error: string }>; } -export async function runImport(engine: BrainEngine, args: string[], opts: { commit?: string; sourceId?: string } = {}): Promise { +export async function runImport( + engine: BrainEngine, + args: string[], + opts: { commit?: string; strategy?: SyncStrategy; sourceId?: string } = {}, +): Promise { const noEmbed = args.includes('--no-embed'); const fresh = args.includes('--fresh'); const jsonOutput = args.includes('--json'); @@ -61,9 +71,20 @@ export async function runImport(engine: BrainEngine, args: string[], opts: { com } const dir: string = dirArg; // narrowed; survives closure capture - // Collect all .md files - const allFiles = collectMarkdownFiles(dir); - console.log(`Found ${allFiles.length} markdown files`); + // v0.31.2: collect under the right strategy. Pre-fix this called + // collectMarkdownFiles unconditionally — code-strategy first sync + // silently no-op'd because no code file ever made it through walker + // enumeration (codex C11 confirms dispatch was correct; bug was here). + const strategy: SyncStrategy = opts.strategy ?? 'markdown'; + const _walkT0 = Date.now(); + console.error(`[gbrain phase] import.collect_files start dir=${dir} strategy=${strategy}`); + const allFiles = collectSyncableFiles(dir, { strategy }); + console.error( + `[gbrain phase] import.collect_files done ${Date.now() - _walkT0}ms files=${allFiles.length}`, + ); + const fileTypeLabel = strategy === 'code' ? 'code' + : strategy === 'auto' ? 'syncable' : 'markdown'; + console.log(`Found ${allFiles.length} ${fileTypeLabel} files`); // Resume from checkpoint if available const checkpointPath = gbrainPath('import-checkpoint.json'); @@ -109,6 +130,10 @@ export async function runImport(engine: BrainEngine, args: string[], opts: { com async function processFile(eng: BrainEngine, filePath: string) { const relativePath = relative(dir, filePath); + // v0.31.2 (D5): per-file slow-path log. Fires only when a single + // file takes >5s. The user's hang surfaces as one file taking + // forever — without this, the agent can't see which file. + const _fileT0 = Date.now(); try { // v0.27.1 (F2): dispatch image extensions to importImageFile when // multimodal is enabled. The walker (collectMarkdownFiles) only picks @@ -117,6 +142,10 @@ export async function runImport(engine: BrainEngine, args: string[], opts: { com const result = isImageFilePath(relativePath) && process.env.GBRAIN_EMBEDDING_MULTIMODAL === 'true' ? await importImageFile(eng, filePath, relativePath, { noEmbed, sourceId }) : await importFile(eng, filePath, relativePath, { noEmbed, sourceId }); + const _fileMs = Date.now() - _fileT0; + if (_fileMs > 5000) { + console.error(`[gbrain phase] import.process_file slow ${_fileMs}ms ${relativePath}`); + } if (result.status === 'imported') { imported++; chunksCreated += result.chunks; @@ -296,54 +325,131 @@ export async function runImport(engine: BrainEngine, args: string[], opts: { com return { imported, skipped, errors, chunksCreated, failures }; } -export function collectMarkdownFiles(dir: string): string[] { +/** + * v0.31.2: max walker depth before bailing out. 32 levels is more than + * any real source tree on disk; reaching it is a structural cycle the + * lstat+inode-set defenses missed (e.g., a Linux bind-mount or btrfs + * subvolume that returns a fresh inode for the same content). Override + * via `GBRAIN_MAX_WALK_DEPTH`. + */ +function resolveMaxWalkDepth(): number { + const raw = process.env.GBRAIN_MAX_WALK_DEPTH; + if (raw) { + const n = Number(raw); + if (Number.isFinite(n) && n > 0) return n; + } + return 32; +} + +interface CollectOpts { + strategy?: SyncStrategy; +} + +/** + * v0.27.1 + v0.31.2: walker-context image admission. `isSyncable` (the + * incremental-diff filter at sync.ts:213) admits images only on `auto`. + * The first-sync walker historically admitted them on markdown too when + * `GBRAIN_EMBEDDING_MULTIMODAL=true`. Codex (C5) flagged the contradiction + * — preserve the walker semantic explicitly. + */ +function isCollectibleForWalker( + path: string, + strategy: SyncStrategy, + multimodalOn: boolean, +): boolean { + switch (strategy) { + case 'code': + return isCodeFilePath(path); + case 'markdown': + return isMarkdownFilePath(path) || (multimodalOn && isImageFilePathFromSync(path)); + case 'auto': + return ( + isMarkdownFilePath(path) || + isCodeFilePath(path) || + (multimodalOn && isImageFilePathFromSync(path)) + ); + } +} + +/** + * v0.31.2 (codex C4 + C5 + C8): unified walker with five hardenings: + * + * 1. `lstatSync` + explicit `isSymbolicLink()` skip — never follow symlinks. + * Replaces the old `collectMarkdownFiles` lstat path AND the old + * `walkSyncableFiles` `statSync` path (the latter was the cost-preview + * walker, weaker than the import walker for no good reason). + * 2. Inode-set cycle detection keyed on `${st_dev}:${st_ino}` — defense in + * depth for non-symlink cycles (bind mounts, ZFS snapshots). + * 3. `MAX_WALK_DEPTH` bailout — last-line backstop if both layers above miss. + * 4. Strategy-aware filter via `isCollectibleForWalker` — single helper that + * surfaces the markdown+multimodal carve-out at one site instead of + * leaking it across two filter paths. + * 5. `.sort()` output — `runImport`'s checkpoint-resume at line 68–74 is + * index-based against a sorted list. Unstable order skips the wrong + * files on resume. + */ +export function collectSyncableFiles(dir: string, opts: CollectOpts = {}): string[] { + const strategy: SyncStrategy = opts.strategy ?? 'markdown'; + const multimodalOn = process.env.GBRAIN_EMBEDDING_MULTIMODAL === 'true'; + const maxDepth = resolveMaxWalkDepth(); + const visitedInodes = new Map(); const files: string[] = []; - function walk(d: string) { - for (const entry of readdirSync(d)) { - // Skip hidden dirs and .raw dirs + function walk(d: string, depth: number): void { + if (depth >= maxDepth) { + console.warn(`[gbrain] walker depth limit reached at ${d}; skipping`); + return; + } + let entries: string[]; + try { + entries = readdirSync(d); + } catch { + return; + } + for (const entry of entries) { + // Skip hidden dirs (.git, .claude, .raw, etc.) and `node_modules`/`ops`. + // Same set the legacy walkers honored, surfaced once at the top of + // every iteration. if (entry.startsWith('.')) continue; - // Skip node_modules - if (entry === 'node_modules') continue; + if (entry === 'node_modules' || entry === 'ops') continue; const full = join(d, entry); let stat; try { - // lstatSync, not statSync: we must NOT follow symlinks. A symlink - // inside the brain directory can point to any file the importing - // user can read, so a contributor to a shared brain could plant - // notes/innocent.md as a symlink to ~/.gbrain/config.json, /etc/passwd, - // or another sensitive file outside the brain root — and on the - // next `gbrain import` it would be read, chunked, embedded, and - // indexed, at which point a bearer-token holder could exfiltrate - // it via search/get_page. See L002 in report/findings.md. stat = lstatSync(full); } catch { - // Broken symlink or permission error — skip console.warn(`[gbrain import] Skipping unreadable path: ${full}`); continue; } - // Skip symlinks (both file and directory targets). This also blocks - // circular symlink DoS since we refuse to descend into linked dirs. if (stat.isSymbolicLink()) { console.warn(`[gbrain import] Skipping symlink: ${full}`); continue; } if (stat.isDirectory()) { - walk(full); - } else if (entry.endsWith('.md') || entry.endsWith('.mdx')) { - files.push(full); - } else if (multimodalEnabled && isImageFilePath(entry)) { - // v0.27.1 (F2): images join the walker only when multimodal is on. - // Pre-v0.27.1 brains keep their existing markdown-only walk. + const inodeKey = `${stat.dev}:${stat.ino}`; + if (visitedInodes.has(inodeKey)) { + console.warn(`[gbrain] walker cycle detected at ${full}; skipping`); + continue; + } + visitedInodes.set(inodeKey, true); + walk(full, depth + 1); + } else if (stat.isFile()) { + if (!isCollectibleForWalker(entry, strategy, multimodalOn)) continue; files.push(full); } } } - const multimodalEnabled = process.env.GBRAIN_EMBEDDING_MULTIMODAL === 'true'; - walk(dir); + walk(dir, 0); return files.sort(); } + +/** + * @deprecated v0.31.2: kept as a thin wrapper so legacy callers keep + * compiling. Prefer `collectSyncableFiles(dir, { strategy: 'markdown' })`. + */ +export function collectMarkdownFiles(dir: string): string[] { + return collectSyncableFiles(dir, { strategy: 'markdown' }); +} diff --git a/src/commands/sync.ts b/src/commands/sync.ts index c9acf0f2d..1b70629d9 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -1,8 +1,9 @@ -import { existsSync, readFileSync, writeFileSync, statSync, readdirSync } from 'fs'; +import { existsSync, readFileSync, writeFileSync, statSync } from 'fs'; import { execFileSync } from 'child_process'; import { join, relative } from 'path'; import type { BrainEngine } from '../core/engine.ts'; import { importFile } from '../core/import-file.ts'; +import { collectSyncableFiles } from './import.ts'; import { createInterface } from 'readline'; import { buildSyncManifest, @@ -74,10 +75,24 @@ function estimateSyncAllCost(sources: Array<{ local_path: string | null; config: let sourceTokens = 0; let sourceFiles = 0; try { - walkSyncableFiles(src.local_path, (filePath: string, content: string) => { - sourceTokens += estimateTokens(content); - sourceFiles++; - }, cfg.strategy ?? 'markdown'); + // v0.31.2: cost preview routed through collectSyncableFiles + // (single hardened walker; see import.ts). Previously + // walkSyncableFiles used statSync (followed symlinks). New walker + // uses lstat + inode-cycle + max-depth so the preview matches + // what the real sync will actually walk. + const files = collectSyncableFiles(src.local_path, { strategy: cfg.strategy ?? 'markdown' }); + for (const fullPath of files) { + try { + const stat = statSync(fullPath); + if (stat.size > 5_000_000) continue; // skip large binaries + const content = readFileSync(fullPath, 'utf-8'); + sourceTokens += estimateTokens(content); + sourceFiles++; + } catch { + // Best-effort per file. Skip unreadable files silently; + // sync itself tolerates the same. + } + } } catch { // Best-effort: a source whose local_path is gone or unreadable just // contributes 0. The sync itself would have failed anyway; no point @@ -91,48 +106,6 @@ function estimateSyncAllCost(sources: Array<{ local_path: string | null; config: return { totalTokens, totalFiles, activeSources, perSource }; } -/** - * Walk a repo's working tree and invoke `cb(path, content)` for each - * syncable file. Honors the same strategy as `isSyncable` so the preview - * and the real sync agree on what's in scope. - */ -function walkSyncableFiles( - repoRoot: string, - cb: (path: string, content: string) => void, - strategy: 'markdown' | 'code' | 'auto', -): void { - const stack: string[] = [repoRoot]; - while (stack.length > 0) { - const dir = stack.pop()!; - let entries: import('fs').Dirent[]; - try { - entries = readdirSync(dir, { withFileTypes: true }) as unknown as import('fs').Dirent[]; - } catch { - continue; - } - for (const entry of entries) { - const name = typeof entry.name === 'string' ? entry.name : String(entry.name); - // Skip hidden dirs, .git, node_modules (same rules isSyncable applies). - if (name.startsWith('.') || name === 'node_modules' || name === 'ops') continue; - const fullPath = `${dir}/${name}`; - if (entry.isDirectory()) { - stack.push(fullPath); - } else if (entry.isFile()) { - const relativePath = fullPath.slice(repoRoot.length + 1); - if (!isSyncable(relativePath, { strategy })) continue; - try { - const stat = statSync(fullPath); - if (stat.size > 5_000_000) continue; // skip large binaries - const content = readFileSync(fullPath, 'utf-8'); - cb(fullPath, content); - } catch { - // Ignore files we can't read; consistent with sync's own tolerance. - } - } - } - } -} - /** Interactive [y/N] prompt. Resolves false on non-y answers or EOF. */ async function promptYesNo(question: string): Promise { return new Promise((resolve) => { @@ -417,11 +390,15 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise relative(repoPath, abs)) - .filter(rel => isSyncable(rel)); + const allFiles = collectSyncableFiles(repoPath, { strategy: opts.strategy ?? 'markdown' }); console.log( - `Full-sync dry run: ${syncableRelPaths.length} file(s) would be imported ` + + `Full-sync dry run (strategy=${opts.strategy ?? 'markdown'}): ` + + `${allFiles.length} file(s) would be imported ` + `from ${repoPath} @ ${headCommit.slice(0, 8)}.`, ); return { status: 'dry_run', fromCommit: null, toCommit: headCommit, - added: syncableRelPaths.length, + added: allFiles.length, modified: 0, deleted: 0, renamed: 0, @@ -965,10 +945,21 @@ async function performFullSync( const importArgs = [repoPath]; if (opts.noEmbed) importArgs.push('--no-embed'); if (fullConcurrency > 1) importArgs.push('--workers', String(fullConcurrency)); - // v0.30.x follow-up to PR #707: thread sourceId through runImport's opts - // so performFullSync routes pages to the named source (the incremental - // sync path in this same file already does this on lines 581/641). - const result = await runImport(engine, importArgs, { commit: headCommit, sourceId: opts.sourceId }); + // v0.31.2: thread strategy through so code-strategy first sync + // actually enumerates code files (closes bug 1). + // v0.30.x: thread sourceId so performFullSync routes pages to the named + // source (incremental path already does this). + const _fullImportT0 = Date.now(); + console.error(`[gbrain phase] sync.fullsync.import start strategy=${opts.strategy ?? 'markdown'}`); + const result = await runImport(engine, importArgs, { + commit: headCommit, + strategy: opts.strategy, + sourceId: opts.sourceId, + }); + console.error( + `[gbrain phase] sync.fullsync.import done ${Date.now() - _fullImportT0}ms ` + + `imported=${result.imported} skipped=${result.skipped} errors=${result.errors}`, + ); // Bug 9 — gate the full-sync bookmark on success. runImport already // writes its own sync.last_commit conditionally (import.ts), but diff --git a/src/core/chunkers/code.ts b/src/core/chunkers/code.ts index f2451b1d5..9468c6310 100644 --- a/src/core/chunkers/code.ts +++ b/src/core/chunkers/code.ts @@ -475,6 +475,77 @@ export interface ChunkAndEdgeResult { edges: import('./edge-extractor.ts').ExtractedEdge[]; } +/** + * Thrown when tree-sitter's wall-clock cap (set via setTimeoutMicros) + * fires and `parser.parse(source)` returns null. The caller is expected + * to fall back to recursive chunking and continue. v0.31.2 closes the + * 99%-CPU-no-I/O hang class where a single pathological file wedged + * the entire sync because tree-sitter's WASM loop is opaque to JS. + */ +export class ChunkerTimeoutError extends Error { + readonly filePath: string; + readonly timeoutMs: number; + constructor(filePath: string, timeoutMs: number) { + super(`Tree-sitter parse timeout on ${filePath} after ${timeoutMs}ms`); + this.name = 'ChunkerTimeoutError'; + this.filePath = filePath; + this.timeoutMs = timeoutMs; + } +} + +interface ParserLike { + setTimeoutMicros(t: number): void; + parse(source: string): unknown; +} + +/** + * Parse `source` with a wall-clock cap, throwing `ChunkerTimeoutError` + * when the parser returns null. Pure function — caller owns parser + * construction AND parser/tree cleanup. Callers MUST wrap the + * parser+tree lifecycle in try/finally so a thrown timeout still + * reaps the WASM allocation. + * + * Test seam: `parser` is `ParserLike` so unit tests can pass a stub + * whose `parse()` returns null deterministically without depending on + * machine speed. The runtime path always passes a real + * web-tree-sitter Parser instance. + * + * @internal exported for tests; production callers go through + * chunkCodeTextFull. + */ +export function parseWithTimeout( + parser: ParserLike, + source: string, + timeoutMs: number, + filePath: string, +): unknown { + if (typeof parser.setTimeoutMicros !== 'function') { + // Fail loud at the seam if a future web-tree-sitter upgrade drops + // the API — better than silently regressing to no-timeout behavior. + throw new Error( + `web-tree-sitter Parser is missing setTimeoutMicros (required for chunker timeout). ` + + `Pin in package.json may be too new (deprecated 0.25.0+) or too old.`, + ); + } + parser.setTimeoutMicros(timeoutMs * 1000); + const tree = parser.parse(source); + if (tree === null || tree === undefined) { + throw new ChunkerTimeoutError(filePath, timeoutMs); + } + return tree; +} + +const DEFAULT_CHUNKER_TIMEOUT_MS = 30_000; + +function resolveChunkerTimeoutMs(): number { + const raw = process.env.GBRAIN_CHUNKER_TIMEOUT_MS; + if (raw) { + const n = Number(raw); + if (Number.isFinite(n) && n > 0) return n; + } + return DEFAULT_CHUNKER_TIMEOUT_MS; +} + export async function chunkCodeTextFull( source: string, filePath: string, @@ -489,29 +560,41 @@ export async function chunkCodeTextFull( const largeThreshold = opts.largeChunkThresholdTokens ?? 1000; const chunkTarget = opts.chunkSizeTokens ?? 300; + const timeoutMs = resolveChunkerTimeoutMs(); + // v0.31.2: parser + tree are always reaped via finally. Pre-fix, the + // catch block returned without delete() — a leak Codex flagged + // (C4) as soon as the timeout path could throw before the manual + // mid-function deletes ran. + let parser: any = null; + let tree: any = null; try { await ensureInit(); const P = await getParser(); - const parser = new (P as any)(); + parser = new (P as any)(); const grammar = await loadLanguage(language); parser.setLanguage(grammar); - const tree = parser.parse(source); - if (!tree) { - parser.delete(); - return { chunks: fallbackChunks(source, filePath, language, opts), edges: [] }; + try { + tree = parseWithTimeout(parser as ParserLike, source, timeoutMs, filePath); + } catch (e: unknown) { + if (e instanceof ChunkerTimeoutError) { + console.warn( + `[gbrain chunker] timeout parsing ${filePath} after ${timeoutMs}ms; ` + + `falling back to recursive chunks`, + ); + return { chunks: fallbackChunks(source, filePath, language, opts), edges: [] }; + } + throw e; } - const root = tree.rootNode; + const root = (tree as any).rootNode; const topLevelTypes = TOP_LEVEL_TYPES[language]; const semanticNodes = topLevelTypes ? root.namedChildren.filter((n: any) => topLevelTypes.has(n.type)) : []; if (semanticNodes.length === 0) { - tree.delete(); - parser.delete(); return { chunks: fallbackChunks(source, filePath, language, opts), edges: [] }; } @@ -599,15 +682,20 @@ export async function chunkCodeTextFull( rawEdges = []; } - tree.delete(); - parser.delete(); - if (chunks.length === 0) { return { chunks: fallbackChunks(source, filePath, language, opts), edges: rawEdges }; } return { chunks: mergeSmallSiblings(chunks, chunkTarget), edges: rawEdges }; } catch { return { chunks: fallbackChunks(source, filePath, language, opts), edges: [] }; + } finally { + // v0.31.2 (codex C4): single cleanup site so a thrown + // ChunkerTimeoutError, edge-extraction failure, or any other + // exception still reaps parser+tree WASM objects. Pre-fix, the + // catch block returned without delete() — a guaranteed leak + // whenever a code file failed to parse. + try { tree?.delete?.(); } catch { /* ignore double-delete */ } + try { parser?.delete?.(); } catch { /* ignore double-delete */ } } } diff --git a/src/core/sync.ts b/src/core/sync.ts index 60a9fca80..cb44c8eff 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -130,10 +130,6 @@ export function isCodeFilePath(path: string): boolean { return false; } -function isMarkdownFilePath(path: string): boolean { - return path.endsWith('.md') || path.endsWith('.mdx'); -} - /** * v0.27.1: image extensions are admitted only when the multimodal config * gate is on. The runtime gate flips through `process.env.GBRAIN_EMBEDDING_MULTIMODAL` @@ -141,7 +137,7 @@ function isMarkdownFilePath(path: string): boolean { * (or env directly when the operator overrides). When the gate is off, * existing brains keep their current "markdown + code only" sync behavior. */ -function isImageFilePath(path: string): boolean { +export function isImageFilePath(path: string): boolean { const lower = path.toLowerCase(); return ( lower.endsWith('.png') || @@ -155,6 +151,10 @@ function isImageFilePath(path: string): boolean { ); } +export function isMarkdownFilePath(path: string): boolean { + return path.endsWith('.md') || path.endsWith('.mdx'); +} + function isMultimodalEnabled(): boolean { return process.env.GBRAIN_EMBEDDING_MULTIMODAL === 'true'; } diff --git a/test/chunker-timeout.test.ts b/test/chunker-timeout.test.ts new file mode 100644 index 000000000..daed2bce5 --- /dev/null +++ b/test/chunker-timeout.test.ts @@ -0,0 +1,117 @@ +/** + * v0.31.2 chunker timeout regression tests. + * + * Closes the bug class where a single pathological code file could + * wedge the entire sync inside tree-sitter WASM at 99% CPU with no + * I/O and no observable progress. The fix bounds parser.parse() with + * setTimeoutMicros and falls back to recursive chunks on timeout. + * + * Test design (per codex C9): the runtime parser is non-deterministic + * about how long it takes to parse arbitrary input, so "force timeout + * via huge input" is machine-speed dependent and flaky on slow CI. We + * stub the ParserLike seam directly to assert the timeout contract, + * then verify the env wiring with a 1ms cap that reliably loses. + */ +import { describe, test, expect } from 'bun:test'; +import { + parseWithTimeout, + ChunkerTimeoutError, + chunkCodeTextFull, +} from '../src/core/chunkers/code.ts'; +import { withEnv } from './helpers/with-env.ts'; + +const REAL_TS = `export function add(a: number, b: number): number { return a + b; } +export class Counter { + private n = 0; + increment(): void { this.n++; } + get value(): number { return this.n; } +} +`; + +describe('parseWithTimeout — pure-function seam', () => { + test('1. throws ChunkerTimeoutError when parser.parse returns null', () => { + const stub = { + _timeoutCalls: 0, + _parseCalls: 0, + setTimeoutMicros(_t: number) { this._timeoutCalls++; }, + parse(_s: string) { this._parseCalls++; return null; }, + }; + + expect(() => parseWithTimeout(stub, 'x', 50, 'foo.ts')).toThrow(ChunkerTimeoutError); + expect(stub._timeoutCalls).toBe(1); + expect(stub._parseCalls).toBe(1); + }); + + test('2. throws clear error if setTimeoutMicros API is missing', () => { + // A future web-tree-sitter that drops the API must NOT silently + // regress to no-timeout behavior. + const stub = { + parse(_s: string) { return { rootNode: null, delete: () => {} }; }, + } as any; + + expect(() => parseWithTimeout(stub, 'x', 50, 'foo.ts')) + .toThrow(/setTimeoutMicros/); + }); + + test('3. returns the tree on success; calls setTimeoutMicros once', () => { + const stub = { + _timeout: 0, + setTimeoutMicros(t: number) { this._timeout = t; }, + parse(_s: string) { return { rootNode: { type: 'program' }, delete: () => {} }; }, + }; + + const tree = parseWithTimeout(stub, 'x', 50, 'foo.ts') as { rootNode: { type: string } }; + expect(tree.rootNode.type).toBe('program'); + expect(stub._timeout).toBe(50_000); // microseconds = ms × 1000 + }); + + test('4. ChunkerTimeoutError carries filePath + timeoutMs for actionable logs', () => { + const stub = { setTimeoutMicros() {}, parse() { return null; } }; + try { + parseWithTimeout(stub, 'x', 30_000, 'src/big.ts'); + throw new Error('should have thrown'); + } catch (e: unknown) { + expect(e).toBeInstanceOf(ChunkerTimeoutError); + expect((e as ChunkerTimeoutError).filePath).toBe('src/big.ts'); + expect((e as ChunkerTimeoutError).timeoutMs).toBe(30_000); + } + }); +}); + +describe('chunkCodeTextFull — integration with real parser', () => { + test('5. default-timeout path on real code completes well under cap', async () => { + const result = await chunkCodeTextFull(REAL_TS, 'sample.ts'); + expect(result.chunks.length).toBeGreaterThan(0); + }); + + test('6. GBRAIN_CHUNKER_TIMEOUT_MS=1 reliably forces fallback', async () => { + // 1ms is below any plausible real-parser wall-clock. The parser + // returns null; chunkCodeTextFull catches the ChunkerTimeoutError + // and falls back to fallbackChunks (recursive text chunker). + await withEnv({ GBRAIN_CHUNKER_TIMEOUT_MS: '1' }, async () => { + const result = await chunkCodeTextFull(REAL_TS, 'sample.ts'); + // Recursive fallback still produces chunks; assertion is that the + // call returned cleanly instead of hanging. + expect(Array.isArray(result.chunks)).toBe(true); + expect(result.edges).toEqual([]); // edges only emitted on tree-sitter path + }); + }); +}); + +describe('cleanup contract under exception', () => { + test('7. parser.delete() still fires when timeout throws (codex C4)', async () => { + // chunkCodeTextFull's finally must reap parser+tree even when + // parseWithTimeout throws ChunkerTimeoutError. We can't directly + // observe parser.delete() in a real WASM run, but we can test the + // shape: forcing the env-timeout path through chunkCodeTextFull a + // few times must not leak (smoke test — Bun GC won't catch a real + // WASM leak but if cleanup were missing on the throw path, repeated + // calls would visibly degrade). + await withEnv({ GBRAIN_CHUNKER_TIMEOUT_MS: '1' }, async () => { + for (let i = 0; i < 5; i++) { + const result = await chunkCodeTextFull(REAL_TS, `sample-${i}.ts`); + expect(Array.isArray(result.chunks)).toBe(true); + } + }); + }); +}); diff --git a/test/sync-walker-symlink.test.ts b/test/sync-walker-symlink.test.ts new file mode 100644 index 000000000..47fe5e83d --- /dev/null +++ b/test/sync-walker-symlink.test.ts @@ -0,0 +1,164 @@ +/** + * v0.31.2 walker hardening regression tests. + * + * Closes the bug class where `gbrain sync --strategy code` could hang on + * repos with self-referencing symlinks (the gstack `.claude/skills/gstack + * -> repo-root` dev pattern was the trigger). Pins: + * + * 1. Self-referencing symlink does not loop. + * 2. Symlink chain through real dirs does not loop (inode-set defense). + * 3. MAX_WALK_DEPTH=32 bailout is structural backstop. + * 4. Strategy filter (code/markdown/auto) admits the right files. + * 5. Dot-prefixed dirs (.git/.claude/.raw) and node_modules still skipped. + * 6. Multimodal preservation under markdown-strategy (codex C5). + * 7. Deterministic ordering — runImport's index-based resume depends on it + * (codex C8). + */ +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { collectSyncableFiles } from '../src/commands/import.ts'; +import { withEnv } from './helpers/with-env.ts'; + +let tmp: string; + +beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), 'gbrain-walker-')); +}); + +afterEach(() => { + rmSync(tmp, { recursive: true, force: true }); +}); + +describe('collectSyncableFiles symlink + cycle hardening', () => { + test('1. self-referencing symlink does not loop', async () => { + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + writeFileSync(join(tmp, 'README.md'), '# top\n'); + // Symlink "loop" inside tempdir pointing back to itself. + symlinkSync(tmp, join(tmp, 'loop')); + + const t0 = Date.now(); + const files = collectSyncableFiles(tmp, { strategy: 'markdown' }); + const ms = Date.now() - t0; + + expect(ms).toBeLessThan(1000); // would hang if walker followed the loop + expect(files).toContain(join(tmp, 'README.md')); + expect(files.every(f => !f.includes('/loop/'))).toBe(true); + }); + }); + + test('2. symlink chain through real dirs does not loop', async () => { + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + // a/ contains a real subdir b/, which contains a symlink "back" -> a. + // The lstat skip catches "back" before recursion. If somehow it + // missed, the inode-cycle Map catches the second visit to a/. + mkdirSync(join(tmp, 'a/b'), { recursive: true }); + writeFileSync(join(tmp, 'a/b/leaf.md'), 'leaf\n'); + symlinkSync(join(tmp, 'a'), join(tmp, 'a/b/back')); + + const t0 = Date.now(); + const files = collectSyncableFiles(tmp, { strategy: 'markdown' }); + const ms = Date.now() - t0; + + expect(ms).toBeLessThan(2000); + expect(files).toContain(join(tmp, 'a/b/leaf.md')); + }); + }); + + test('3. max-depth bailout terminates pathological deep trees', async () => { + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + // Build a 40-level real directory tree. 32 is the default cap; the + // file at the deepest level is intentionally past the bailout so we + // can assert it is NOT collected (and the walker still terminates). + let cur = tmp; + for (let i = 0; i < 40; i++) { + cur = join(cur, `d${i}`); + mkdirSync(cur); + } + writeFileSync(join(cur, 'deep.md'), 'deep\n'); + writeFileSync(join(tmp, 'shallow.md'), 'shallow\n'); + + const files = collectSyncableFiles(tmp, { strategy: 'markdown' }); + + expect(files).toContain(join(tmp, 'shallow.md')); + expect(files.every(f => !f.includes('/d35/'))).toBe(true); // past depth 32 + }); + }); + + test('4. strategy filter admits the right files', async () => { + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + writeFileSync(join(tmp, 'README.md'), '# r\n'); + writeFileSync(join(tmp, 'foo.ts'), '// f\n'); + writeFileSync(join(tmp, 'bar.py'), '# b\n'); + + const code = collectSyncableFiles(tmp, { strategy: 'code' }); + const markdown = collectSyncableFiles(tmp, { strategy: 'markdown' }); + const auto = collectSyncableFiles(tmp, { strategy: 'auto' }); + + expect(code.map(f => f.split('/').pop()).sort()).toEqual(['bar.py', 'foo.ts']); + expect(markdown.map(f => f.split('/').pop())).toEqual(['README.md']); + expect(auto.map(f => f.split('/').pop()).sort()).toEqual(['README.md', 'bar.py', 'foo.ts']); + }); + }); + + test('5. dot-prefixed dirs and node_modules still skipped', async () => { + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + writeFileSync(join(tmp, 'real.md'), 'r\n'); + mkdirSync(join(tmp, '.git')); + writeFileSync(join(tmp, '.git/HEAD'), 'ref: refs/heads/main\n'); + mkdirSync(join(tmp, '.claude/skills'), { recursive: true }); + writeFileSync(join(tmp, '.claude/skills/SKILL.md'), 's\n'); + mkdirSync(join(tmp, 'node_modules/foo'), { recursive: true }); + writeFileSync(join(tmp, 'node_modules/foo/index.md'), 'no\n'); + + const files = collectSyncableFiles(tmp, { strategy: 'markdown' }); + const names = files.map(f => f.replace(tmp, '')); + + expect(names).toContain('/real.md'); + expect(names.every(n => !n.startsWith('/.git'))).toBe(true); + expect(names.every(n => !n.startsWith('/.claude'))).toBe(true); + expect(names.every(n => !n.startsWith('/node_modules'))).toBe(true); + }); + }); + + test('6. multimodal preservation under markdown strategy (codex C5)', async () => { + writeFileSync(join(tmp, 'r.md'), 'r\n'); + writeFileSync(join(tmp, 'p.png'), Buffer.from([0x89, 0x50, 0x4e, 0x47])); + writeFileSync(join(tmp, 'j.jpg'), Buffer.from([0xff, 0xd8, 0xff])); + + // Off → markdown only. + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + const off = collectSyncableFiles(tmp, { strategy: 'markdown' }); + expect(off.map(f => f.split('/').pop()).sort()).toEqual(['r.md']); + }); + + // On → markdown + images (preserves v0.27.1 F2 collectMarkdownFiles + // behavior; codex C5 carve-out). + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: 'true' }, () => { + const on = collectSyncableFiles(tmp, { strategy: 'markdown' }); + expect(on.map(f => f.split('/').pop()).sort()).toEqual(['j.jpg', 'p.png', 'r.md']); + }); + }); + + test('7. deterministic ordering — two walks return identical arrays (codex C8)', async () => { + await withEnv({ GBRAIN_EMBEDDING_MULTIMODAL: undefined }, () => { + // runImport's checkpoint resume at import.ts:68-74 is index-based + // against a sorted file list. Unstable order skips the wrong files + // on resume. + writeFileSync(join(tmp, 'b.md'), 'b\n'); + writeFileSync(join(tmp, 'a.md'), 'a\n'); + mkdirSync(join(tmp, 'sub')); + writeFileSync(join(tmp, 'sub/c.md'), 'c\n'); + + const first = collectSyncableFiles(tmp, { strategy: 'markdown' }); + const second = collectSyncableFiles(tmp, { strategy: 'markdown' }); + + expect(first).toEqual(second); + // Sorted: a.md, b.md, sub/c.md (lexicographic on absolute paths). + expect(first.map(f => f.replace(tmp, ''))).toEqual([ + '/a.md', '/b.md', '/sub/c.md', + ]); + }); + }); +});